Re: [PATCH v3 4/5] binder: Remove mmap_lock fallback

On Mon, Aug 3, 2026 at 4:33 AM Lorenzo Stoakes (ARM) <[email protected]> wrote:
>
> On Sun, Aug 02, 2026 at 02:54:58PM -0700, Suren Baghdasaryan wrote:
> > From: Dave Hansen <[email protected]>
> >
> > Previously, the per-VMA locking could fail in the face of writers
> > which necessitate a fallback to mmap_lock. The new
> > vma_start_read_unlocked() will wait for writers instead of failing.
> >
> > Use the new helper. Wait for writers. Remove the fallback to mmap_lock.
> >
> > Signed-off-by: Dave Hansen <[email protected]>
> > Signed-off-by: Suren Baghdasaryan <[email protected]>
>
> LGTM, just a nit below.
>
> Acked-by: Lorenzo Stoakes (ARM) <[email protected]>

Thanks!

>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Liam R. Howlett <[email protected]>
> > Cc: Vlastimil Babka <[email protected]>
> > Cc: Shakeel Butt <[email protected]>
> > Cc: [email protected]
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Arve Hjønnevåg <[email protected]>
> > Cc: Todd Kjos <[email protected]>
> > Cc: Christian Brauner <[email protected]>
> > Cc: Carlos Llamas <[email protected]>
> > Cc: Alice Ryhl <[email protected]>
> > Cc: David S. Miller <[email protected]>
> > Cc: David Ahern <[email protected]>
> > Cc: [email protected]
> > ---
> >  drivers/android/binder/page_range.rs | 19 +++----------------
> >  drivers/android/binder_alloc.c       | 17 +++++------------
> >  rust/kernel/mm.rs                    | 18 ++++++++++++++++++
> >  3 files changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
> > index e82a5523804f..f7ad88a0d806 100644
> > --- a/drivers/android/binder/page_range.rs
> > +++ b/drivers/android/binder/page_range.rs
> > @@ -439,22 +439,9 @@ unsafe fn use_page_slow(&self, i: usize) -> Result<()> {
> >          // workqueue.
> >          let mm = MmWithUser::into_mmput_async(self.mm.mmget_not_zero().ok_or(ESRCH)?);
> >          {
> > -            let vma_read;
> > -            let mmap_read;
> > -            let vma = if let Some(ret) = mm.lock_vma_under_rcu(vma_addr) {
> > -                vma_read = ret;
> > -                check_vma(&vma_read, self)
> > -            } else {
> > -                mmap_read = mm.mmap_read_lock();
> > -                mmap_read
> > -                    .vma_lookup(vma_addr)
> > -                    .and_then(|vma| check_vma(vma, self))
> > -            };
> > -
> > -            match vma {
> > -                Some(vma) => vma.vm_insert_page(user_page_addr, &new_page)?,
> > -                None => return Err(ESRCH),
> > -            }
> > +            let vma_read_guard = mm.vma_start_read_unlocked(vma_addr).ok_or(ESRCH)?;
> > +            let vma = check_vma(&vma_read_guard, self).ok_or(ESRCH)?;
> > +            vma.vm_insert_page(user_page_addr, &new_page)?;
> >          }
> >
> >          let inner = self.lock.lock();
> > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> > index 84104ba04e30..519dcded19b2 100644
> > --- a/drivers/android/binder_alloc.c
> > +++ b/drivers/android/binder_alloc.c
> > @@ -259,21 +259,14 @@ static int binder_page_insert(struct binder_alloc *alloc,
> >       struct vm_area_struct *vma;
> >       int ret = -ESRCH;
> >
> > -     /* attempt per-vma lock first */
> > -     vma = lock_vma_under_rcu(mm, addr);
> > -     if (vma) {
> > -             if (binder_alloc_is_mapped(alloc))
> > -                     ret = vm_insert_page(vma, addr, page);
> > -             vma_end_read(vma);
> > +     vma = vma_start_read_unlocked(mm, addr);
> > +     if (!vma)
> >               return ret;
> > -     }
> >
> > -     /* fall back to mmap_lock */
> > -     mmap_read_lock(mm);
> > -     vma = vma_lookup(mm, addr);
> > -     if (vma && binder_alloc_is_mapped(alloc))
> > +     if (binder_alloc_is_mapped(alloc))
> >               ret = vm_insert_page(vma, addr, page);
> > -     mmap_read_unlock(mm);
> > +
> > +     vma_end_read(vma);
>
> Nice cleanup :)
>
> >
> >       return ret;
> >  }
> > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> > index 2633e704c83d..877fad68be9c 100644
> > --- a/rust/kernel/mm.rs
> > +++ b/rust/kernel/mm.rs
> > @@ -190,6 +190,24 @@ pub fn lock_vma_under_rcu(&self, vma_addr: usize) -> Option<VmaReadGuard<'_>> {
> >          }
> >      }
> >
> > +    /// Find the VMA covering 'address' and lock it for reading. Waits for writers to finish if the
> > +    /// VMA is being modified.
>
> This seems a little inconsistent with the C version's comment, should they not be the same?

Ack. Will change.

>
> > +    #[inline]
> > +    pub fn vma_start_read_unlocked(&self, vma_addr: usize) -> Option<VmaReadGuard<'_>> {
> > +        // SAFETY: We may invoke `vma_start_read_unlocked` because we know this `mm` has non-zero
> > +        // `mm_users`.
> > +        let vma = unsafe { bindings::vma_start_read_unlocked(self.as_raw(), vma_addr) };
> > +        if vma.is_null() {
> > +            return None;
> > +        }
> > +        Some(VmaReadGuard {
> > +            // SAFETY: If `vma_start_read_unlocked` returns a non-null ptr, then it points at a
> > +            // valid vma. The vma is stable for as long as the vma read lock is held.
> > +            vma: unsafe { VmaRef::from_raw(vma) },
> > +            _nts: NotThreadSafe,
> > +        })
> > +    }
> > +
> >      /// Lock the mmap read lock.
> >      #[inline]
> >      pub fn mmap_read_lock(&self) -> MmapReadGuard<'_> {
> > --
> > 2.55.0.508.g3f0d502094-goog
> >
>
> --
> Cheers, Lorenzo