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

[Lorenzo Stoakes (ARM)]

On Sun, Aug 02, 2026 at 02:54:58PM -0700, Suren Baghdasaryan wrote:
> From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>
> 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 <dave.hansen@xxxxxxxxxxxxxxx>
> Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

LGTM, just a nit below.

Acked-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> Cc: Vlastimil Babka <vbabka@xxxxxxxxxx>
> Cc: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> Cc: linux-mm@xxxxxxxxx
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Arve Hjønnevåg <arve@xxxxxxxxxxx>
> Cc: Todd Kjos <tkjos@xxxxxxxxxxx>
> Cc: Christian Brauner <christian@xxxxxxxxxx>
> Cc: Carlos Llamas <cmllamas@xxxxxxxxxx>
> Cc: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> Cc: David S. Miller <davem@xxxxxxxxxxxxx>
> Cc: David Ahern <dsahern@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx
> ---
>  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?

> +    #[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