Re: [PATCH v7 0/6] Per-VMA lock support for swap and userfaults

[David Hildenbrand]

On 10.08.23 08:24, Suren Baghdasaryan wrote:
> On Wed, Aug 9, 2023 at 10:29 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> > On Wed, Aug 9, 2023 at 11:31 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> > > On Wed, Aug 9, 2023 at 11:08 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> > > > On Wed, Aug 9, 2023 at 11:04 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > > > > > > > > Which ends up being
> > > > > > > > >
> > > > > > > > > VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
> > > > > > > > >
> > > > > > > > > I did not check if this is also the case on mainline, and if this series is responsible.
> > > > > > > >
> > > > > > > > Thanks for reporting! I'm checking it now.
> > > > > > >
> > > > > > > Hmm. From the code it's not obvious how lock_mm_and_find_vma() ends up
> > > > > > > calling find_vma() without mmap_lock after successfully completing
> > > > > > > get_mmap_lock_carefully(). lock_mm_and_find_vma+0x3f/0x270 points to
> > > > > > > the first invocation of find_vma(), so this is not even the lock
> > > > > > > upgrade path... I'll try to reproduce this issue and dig up more but
> > > > > > > from the information I have so far this issue does not seem to be
> > > > > > > related to this series.
> > > > >
> > > > > I just checked on mainline and it does not fail there.
> > >
> > > Thanks. Just to eliminate the possibility, I'll try reverting my
> > > patchset in mm-unstable and will try the test again. Will do that in
> > > the evening once I'm home.
> > >
> > > > > > This is really weird. I added mmap_assert_locked(mm) calls into
> > > > > > get_mmap_lock_carefully() right after we acquire mmap_lock read lock
> > > > > > and one of them triggers right after successful
> > > > > > mmap_read_lock_killable(). Here is my modified version of
> > > > > > get_mmap_lock_carefully():
> > > > > >
> > > > > > static inline bool get_mmap_lock_carefully(struct mm_struct *mm,
> > > > > > struct pt_regs *regs) {
> > > > > >        /* Even if this succeeds, make it clear we might have slept */
> > > > > >        if (likely(mmap_read_trylock(mm))) {
> > > > > >            might_sleep();
> > > > > >            mmap_assert_locked(mm);
> > > > > >            return true;
> > > > > >        }
> > > > > >        if (regs && !user_mode(regs)) {
> > > > > >            unsigned long ip = instruction_pointer(regs);
> > > > > >            if (!search_exception_tables(ip))
> > > > > >                return false;
> > > > > >        }
> > > > > >        if (!mmap_read_lock_killable(mm)) {
> > > > > >            mmap_assert_locked(mm);                     <---- generates a BUG
> > > > > >            return true;
> > > > > >        }
> > > > > >        return false;
> > > > > > }
> > > > >
> > > > > Ehm, that's indeed weird.
> > > > >
> > > > > > AFAIKT conditions for mmap_read_trylock() and
> > > > > > mmap_read_lock_killable() are checked correctly. Am I missing
> > > > > > something?
> > > > >
> > > > > Weirdly enough, it only triggers during that specific uffd test, right?
> > > >
> > > > Yes, uffd-unit-tests. I even ran it separately to ensure it's not some
> > > > fallback from a previous test and I'm able to reproduce this
> > > > consistently.
> >
> > Yeah, it is somehow related to per-vma locking. Unfortunately I can't
> > reproduce the issue on my VM, so I have to use my host and bisection
> > is slow. I think I'll get to the bottom of this tomorrow.
>
> Ok, I think I found the issue. 

Nice!

> wp_page_shared() ->
> fault_dirty_shared_page() can drop mmap_lock (see the comment saying
> "Drop the mmap_lock before waiting on IO, if we can...", therefore we
> have to ensure we are not doing this under per-VMA lock.
> I think what happens is that this path is racing with another page
> fault which took mmap_lock for read. fault_dirty_shared_page()
> releases this lock which was taken by another page faulting thread and
> that thread generates an assertion when it finds out the lock it just
> took got released from under it.

I wonder if we could detect that someone releases the mmap lock that was 
not taken by that person, to bail out early at the right place when 
debugging such issues. Only with certain config knobs enabled, of course.

> The following crude change fixed the issue for me but there might be a
> more granular way to deal with this:
>
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3293,18 +3293,18 @@ static vm_fault_t wp_page_shared(struct
> vm_fault *vmf, struct folio *folio)
>           struct vm_area_struct *vma = vmf->vma;
>           vm_fault_t ret = 0;
>
> +        if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
> +                pte_unmap_unlock(vmf->pte, vmf->ptl);
> +                vma_end_read(vmf->vma);
> +                return VM_FAULT_RETRY;
> +        }
> +

I won't lie: all of these locking checks are a bit hard to get and 
possibly even harder to maintain.

Maybe better mmap unlock sanity checks as spelled out above might help 
improve part of the situation.


And maybe some comments regarding the placement might help as well ;)

--
Cheers,

David / dhildenb