Re: [PATCH v4 3/5] mm: Add RCU-based VMA lookup helper that waits for writers

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Sat, Aug 08, 2026 at 06:07:41PM -0700, Suren Baghdasaryan wrote:
> On Sat, Aug 8, 2026 at 12:24 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Thu, Aug 06, 2026 at 01:05:46PM -0700, Suren Baghdasaryan wrote:
> > > From: Dave Hansen <dave.hansen@linux.intel.com>
> > >
> > > == Background ==
> >
> > I think we can do without the headings?
> >
> > > There are basically two parallel ways to look up a VMA: the
> > > traditional way, which is protected by mmap_read_lock, and the RCU-based
> > > per-VMA lock way which is based on RCU and refcounts.
> > >
> > > == Problem ==
> > >
> > > The mmap_lock one is more straightforward to use but it has a big
> > > disadvantage in that it can not be mixed with page faults since those
> > > can take mmap_lock for read, which can deadlock when mixed with nested
> > > page faults and parallel writers.
> > > For example:
> > >
> > >       mmap_read_lock(mm);
> > >       // Another thread does mmap_write_lock().
> > >       // New mmap_lock readers are blocked.
> > >       vma = vma_lookup(mm, address);
> > >       // This deadlocks on mmap_read_lock() if it faults:
> > >       copy_from_user(address);
> > >       mmap_read_unlock(mm);
> > >
> > > The per-VMA lock can be mixed with faults, but they can fail and need to
> > > be able to fall back to the traditional way.
> >
> > Umm.  I don't see how this avoids deadlock.  Assuming the next patch
> > converts copy_from_user() to use the VMA lock, surely the following
> > situation would obtain:
> >
> > A takes mmap_read_lock
> > B tries to take mmap_write_lock, blocks
> > A calls copy_from_user()
> > A calls vma_start_read_unlocked() (because it doesn't know A actually
> > holds the mmap_read_lock() already)
> > A does a lookup under RCU, but gets NULL back (maybe it's calling
> > c_f_u() with an invalid address?)
> > A tries to take the mmap_read_lock again to make sure.  Deadlock
> > because B is waiting for A to release the mmap_read_lock.
> >
> > Am I missing something?
>
> I think Dave's idea was that A would take a VMA lock (which might
> require taking mmap read lock temporarily) and then call
> copy_from_user() with VMA lock only, which allows copy_from_user() to
> take the mmap lock. This was used in the last patch [1] of the
> original RFC. That patch is dropped from the series because this
> approach has a conceptual lock ordering problem - it takes VMA lock
> and then mmap_lock (see explanation at [2]).

Yeah I wonder about the value of this. Dave can write all this up when he makes
the change to use this in the relevant shadow stack (I think it was?) code.

Having it here when just adding the function is not really helpful I don't think.

Other use cases are stronger arguments for it anyway - we are essentially doing
stuff like this in places and falling back to mmap when we don't need to - when
we have the option to 'twiddle' the mmap lock and get a VMA lock.

>
> [1] https://lore.kernel.org/all/20260429182005.00BF70D8@davehans-spike.ostc.intel.com/
> [2] https://lore.kernel.org/all/CAJuCfpHBzYUNEe_LBpeERf68_fso=Hy+UZWtqejTUkm1O4NLfA@mail.gmail.com/
>
> So, yeah. I need to rewrite this commit message.
>
> >
> > > +/**
> > > + * vma_start_read_unlocked() - Find the VMA covering 'address' and read-lock it.
> > > + * @mm: the mm_struct of the address space to search
> > > + * @address: address that the vma should contain
> > > + *
> > > + * The fast path does not take mmap_lock. Waits for writers to finish if the
> > > + * VMA is being modified by taking mmap_lock.
> > > + * Use when mmap_lock is not held, otherwise use vma_start_read_locked().
> > > + * Nothing prevents VMAs being unmapped/mapped before or after the VMA is
> > > + * looked up, if a stronger guarantee is required, take an mmap_lock.
> > > + *
> > > + * Return: If a VMA exists which spans @address, return that VMA, read-locked.
> > > + * If no VMA is mapped there or, very unlikely, a reference count overflow
> > > + * occurred, return NULL.
> > > + */
> > > +struct vm_area_struct *vma_start_read_unlocked(struct mm_struct *mm,
> > > +                                            unsigned long address)
> > > +{
> > > +     struct vm_area_struct *vma;
> > > +
> > > +     /* Fast path: return stable VMA covering 'address': */
> > > +     vma = lock_vma_under_rcu(mm, address);
> > > +     if (vma)
> > > +             return vma;
> > > +
> > > +     /* Slow path: preclude VMA writers by temporarily getting mmap read lock. */
> > > +     mmap_read_lock(mm);
> > > +     vma = vma_lookup(mm, address);
> > > +     if (vma && !vma_start_read_locked(vma))
> > > +             vma = NULL;
> > > +     mmap_read_unlock(mm);
> > > +
> > > +     return vma;
> > > +}

--
Cheers, Lorenzo