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

On Mon, Aug 3, 2026 at 4:33=E2=80=AFAM Lorenzo Stoakes (ARM) <[email protected]=
g> 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=C3=B8nnev=C3=A5g <[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/bin=
der/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 =3D MmWithUser::into_mmput_async(self.mm.mmget_not_zero=
().ok_or(ESRCH)?);
> >          {
> > -            let vma_read;
> > -            let mmap_read;
> > -            let vma =3D if let Some(ret) =3D mm.lock_vma_under_rcu(vma=
_addr) {
> > -                vma_read =3D ret;
> > -                check_vma(&vma_read, self)
> > -            } else {
> > -                mmap_read =3D mm.mmap_read_lock();
> > -                mmap_read
> > -                    .vma_lookup(vma_addr)
> > -                    .and_then(|vma| check_vma(vma, self))
> > -            };
> > -
> > -            match vma {
> > -                Some(vma) =3D> vma.vm_insert_page(user_page_addr, &new=
_page)?,
> > -                None =3D> return Err(ESRCH),
> > -            }
> > +            let vma_read_guard =3D mm.vma_start_read_unlocked(vma_addr=
).ok_or(ESRCH)?;
> > +            let vma =3D check_vma(&vma_read_guard, self).ok_or(ESRCH)?=
;
> > +            vma.vm_insert_page(user_page_addr, &new_page)?;
> >          }
> >
> >          let inner =3D self.lock.lock();
> > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_al=
loc.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 =3D -ESRCH;
> >
> > -     /* attempt per-vma lock first */
> > -     vma =3D lock_vma_under_rcu(mm, addr);
> > -     if (vma) {
> > -             if (binder_alloc_is_mapped(alloc))
> > -                     ret =3D vm_insert_page(vma, addr, page);
> > -             vma_end_read(vma);
> > +     vma =3D vma_start_read_unlocked(mm, addr);
> > +     if (!vma)
> >               return ret;
> > -     }
> >
> > -     /* fall back to mmap_lock */
> > -     mmap_read_lock(mm);
> > -     vma =3D vma_lookup(mm, addr);
> > -     if (vma && binder_alloc_is_mapped(alloc))
> > +     if (binder_alloc_is_mapped(alloc))
> >               ret =3D 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 the=
y not be the same?

Ack. Will change.

>
> > +    #[inline]
> > +    pub fn vma_start_read_unlocked(&self, vma_addr: usize) -> Option<V=
maReadGuard<'_>> {
> > +        // SAFETY: We may invoke `vma_start_read_unlocked` because we =
know this `mm` has non-zero
> > +        // `mm_users`.
> > +        let vma =3D 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 rea=
d 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