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

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

On Mon, Aug 03, 2026 at 06:24:34PM +0200, Vlastimil Babka (SUSE) wrote:
> On 8/3/26 17:00, Lorenzo Stoakes (ARM) wrote:
> > On Mon, Aug 03, 2026 at 04:55:19PM +0200, Vlastimil Babka (SUSE) wrote:
> >> On 8/2/26 23:54, Suren Baghdasaryan wrote:
> >> > From: Dave Hansen <dave.hansen@linux.intel.com>
> >> >
> >> > == Background ==
> >> >
> >> > 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.
> >> >
> >> > == Solution ==
> >> >
> >> > Add a variant of the RCU-based lookup that waits for writers. This is
> >> > basically the same as the existing RCU-based lookup, but on a failure to
> >> > lock it temporarily takes mmap_lock for read and waits for writers
> >> > to finish before locking the VMA, dropping the mmap_lock and returning
> >> > the locked VMA. This has some advantages:
> >>
> >> Maybe mention that the helper is called vma_start_read_unlocked()?
> >>
> >> >
> >> >  1. Callers do not need to have a fallback path for when they
> >> >     collide with writers.
> >> >  2. It can be used in contexts where page faults can happen because
> >> >     it can take the mmap_lock for read but never *holds* it.
> >> >  3. Its fast path does not require taking mmap_lock for read.
> >> >
> >> > Basically, when applied correctly, this approach results in faster
> >> > *and* simpler code.
> >> >
> >> > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >> > Cc: Suren Baghdasaryan <surenb@google.com>
> >> > Cc: Andrew Morton <akpm@linux-foundation.org>
> >> > Cc: "Liam R. Howlett" <Liam.Howlett@oracle.com>
> >> > Cc: Lorenzo Stoakes <ljs@kernel.org>
> >> > Cc: Vlastimil Babka <vbabka@kernel.org>
> >> > Cc: Shakeel Butt <shakeel.butt@linux.dev>
> >> > Cc: linux-mm@kvack.org
> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Cc: Arve Hjønnevåg <arve@android.com>
> >> > Cc: Todd Kjos <tkjos@android.com>
> >> > Cc: Christian Brauner <christian@brauner.io>
> >> > Cc: Carlos Llamas <cmllamas@google.com>
> >> > Cc: Alice Ryhl <aliceryhl@google.com>
> >> > Cc: "David S. Miller" <davem@davemloft.net>
> >> > Cc: David Ahern <dsahern@kernel.org>
> >> > Cc: netdev@vger.kernel.org
> >> > ---
> >> >  include/linux/mmap_lock.h | 15 +++++++++++----
> >> >  mm/mmap_lock.c            | 29 +++++++++++++++++++++++++++++
> >> >  mm/userfaultfd.c          |  6 ++++--
> >> >  3 files changed, 44 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> >> > index eb32b482434e..fdd8f5cf5722 100644
> >> > --- a/include/linux/mmap_lock.h
> >> > +++ b/include/linux/mmap_lock.h
> >> > @@ -228,10 +228,12 @@ static inline void vma_refcount_put(struct vm_area_struct *vma)
> >> >  }
> >> >
> >> >  /*
> >> > - * Use only while holding mmap read lock which guarantees that locking will not
> >> > - * fail (nobody can concurrently write-lock the vma). vma_start_read() should
> >> > + * Use only while holding mmap read lock which guarantees that vma lock is not
> >> > + * contended (nobody can concurrently write-lock the vma). vma_start_read() should
> >> >   * not be used in such cases because it might fail due to mm_lock_seq overflow.
> >> >   * This functionality is used to obtain vma read lock and drop the mmap read lock.
> >> > + * VMA can't be detached while we are holding mmap lock, therefore in practice this
> >> > + * function can fail only when there are so many readers that vm_refcnt overflows.
> >> >   */
> >> >  static inline bool vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass)
> >> >  {
> >> > @@ -247,16 +249,21 @@ static inline bool vma_start_read_locked_nested(struct vm_area_struct *vma, int
> >> >  }
> >> >
> >> >  /*
> >> > - * Use only while holding mmap read lock which guarantees that locking will not
> >> > - * fail (nobody can concurrently write-lock the vma). vma_start_read() should
> >> > + * Use only while holding mmap read lock which guarantees that vma lock is not
> >> > + * contended (nobody can concurrently write-lock the vma). vma_start_read() should
> >> >   * not be used in such cases because it might fail due to mm_lock_seq overflow.
> >> >   * This functionality is used to obtain vma read lock and drop the mmap read lock.
> >> > + * VMA can't be detached while we are holding mmap lock, therefore in practice this
> >> > + * function can fail only when there are so many readers that vm_refcnt overflows.
> >> >   */
> >> >  static inline bool vma_start_read_locked(struct vm_area_struct *vma)
> >> >  {
> >> >  	return vma_start_read_locked_nested(vma, 0);
> >> >  }
> >> >
> >> > +struct vm_area_struct *vma_start_read_unlocked(struct mm_struct *mm,
> >> > +					       unsigned long address);
> >> > +
> >> >  static inline void vma_end_read(struct vm_area_struct *vma)
> >> >  {
> >> >  	vma_refcount_put(vma);
> >> > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> >> > index e20d01e8d38f..6ff05e68e61b 100644
> >> > --- a/mm/mmap_lock.c
> >> > +++ b/mm/mmap_lock.c
> >> > @@ -338,6 +338,35 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >> >  	return NULL;
> >> >  }
> >> >
> >> > +/*
> >> > + * Find the VMA covering 'address' and lock it for reading. Waits for writers to
> >> > + * finish if the VMA is being modified. Returns NULL if there is no VMA covering
> >> > + * 'address'.
> >>
> >> Hm but it can also return NULL when vm_refcnt overflows, in theory.
> >> Should we also return -EAGAIN (like uffd_lock_vma() below), or just retry in
> >> here and hope for the best? The latter would be simpler for the users.
> >> (AFAICS due to VM_REFCNT_LIMIT we never end up triggering the refcount
> >> saturation)
> >
> > The problem is everything's unlocked so 'didn't find a VMA' doesn't really mean
> > much more than 'something went wrong' because hey maybe if you check again now
> > you'll find something :)
>
> Well there might be use cases where you know that either there's a vma with
> your address and then you need to do something with it, or there's not and
> then you don't. And it can't suddenly appear after you check.

You don't hold a lock that prevents new VMAs appearing/disappearing
spontaneously at the point you call lock_vma_under_rcu(), or after you drop the
mmap read lock, only that at the point of checking a VMA spans address, so
there's nothing preventing a VMA suddenly appearing after you check right? Or it
not being the one you wanted?

And checking to see if it's 'really the one you meant' is itself fraught (see
the whole uffd saga on that).

Point I'm making is that in any case where you'd actually care you'd need to
take a stronger lock anyway, so it's actually potentially dangerous to
differentiate between the two.

Given the overflow is very very unlikely I think it's also not a big deal to not
differentiate anyway.

>
> So in that case treating that spurious NULL as "there's no vma so I don't
> need to do anything" would be wrong.
>
> The usages in 4/5 and 5/5 seem like they are not this case though. So it's
> fine. But perhaps worth just mentioning it in the comment then.

Agree this is worth spelling out in the comment (I raised similarly).

Maybe something like:

	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.

	Nothing prevents VMAs being unmapped/mapped before or after the VMA is
	looked up, if a stronger guarantee is required, take an mmap lock.

>
> > So I think this might be a feature more than a bug, especially given overflow is
> > not exactly likely.
> >
> >>
> >> > + *
> >> > + * Use only in code paths where no mmap_lock and no VMA lock is held.
> >> > + *
> >> > + * The fast path does not take mmap_lock.
> >> > + */
> >> > +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;
> >> > +}
> >> > +
> >> >  static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm,
> >> >  							    struct vma_iterator *vmi,
> >> >  							    unsigned long from_addr)
> >> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> >> > index edd90892f8cc..c3a0c38a3dc3 100644
> >> > --- a/mm/userfaultfd.c
> >> > +++ b/mm/userfaultfd.c
> >> > @@ -129,8 +129,10 @@ struct vm_area_struct *find_vma_and_prepare_anon(struct mm_struct *mm,
> >> >   *
> >> >   * Should be called without holding mmap_lock.
> >> >   *
> >> > - * Return: A locked vma containing @address, -ENOENT if no vma is found, or
> >> > - * -ENOMEM if anon_vma couldn't be allocated.
> >> > + * Return: A locked vma containing @address, -ENOENT if no vma is found,
> >> > + * -ENOMEM if anon_vma couldn't be allocated, or -EAGAIN if vma refcount
> >> > + * overflow happened due to high number of readers and the caller should
> >> > + * retry later.
> >> >   */
> >> >  static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm,
> >> >  				       unsigned long address)
> >>
> >
> > --
> > Cheers, Lorenzo
>

--
Cheers, Lorenzo