Re: [PATCH v2 2/5] binder: Make shrinker rely solely on per-VMA lock

[Carlos Llamas]

On Fri, Jun 12, 2026 at 09:54:58AM -0700, Dave Hansen wrote:
> On 6/12/26 09:41, Suren Baghdasaryan wrote:
> >> I think the key to distinguishing between:
> >>
> >>         vma==NULL because there's no VMA
> >> and
> >>         vma==NULL because of a trylock failure
> >>
> >> is binder_alloc_is_mapped(). It won't return false until vm_ops->close()
> >> finishes. vm_ops->close() shouldn't be able to happen while
> >> lock_vma_under_rcu() is held. So if you've got a non-NULL VMA, you've
> >> also got a stable is binder_alloc_is_mapped().
> > By "stable binder_alloc_is_mapped()" do you mean it would always be
> > true?
> 
> By stable, I meant that it can't change.
> 
> 	vma = lock_vma_under_rcu()
> 	mapped = binder_alloc_is_mapped();
> 	<window>
> 	vma_end_read(vma);
> 
> During <window> it can't go from true=>false or false=>true.
> 
> false=>true never happens from what I can tell. It's just plain
> impossible given the current code.
> 
> true=>false is locked out because when lock_vma_under_rcu() is held.
> 
> > Asking because in your patch you removed this condition:
> > 
> > -         if (vma && !binder_alloc_is_mapped(alloc))
> > -                  goto err_invalid_vma;
> > 
> > So, previously if we found the VMA but binder_alloc_is_mapped()==false
> > we would bail out and now we don't. Are you reasoning that this
> > combination is impossible?
> 
> It's not impossible, but I do think it is irrelevant. Or at least that
> the *VMA* is irrelevant in this case. binder_alloc_is_mapped()==false
> means that the binder VMA is gone. It's not in the maple tree, and it's
> not coming back. If a VMA is found, it's an impostor.
> 
> That's why I did:
> 
> -        if (vma) {
> +        if (mapped) {
> 
> The question isn't whether a VMA was found. The question is whether the
> binder VMA is still mapped at page_addr. *That* is best inferred from
> binder_alloc_is_mapped(), not the VMA lookup.
> 
> At least that's what I decided after staring at it for far too long.

Yes, I _think_ binder_alloc_is_mapped() can help distinguish between the
two scenarios (contention vs vma-close). However, I think it would be
simpler and safe to do an early exit:

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 88c3e1667d5b..9dd7d927249d 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1149,6 +1149,8 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	 * for 'page_addr'.
 	 */
 	vma = lock_vma_under_rcu(mm, page_addr);
+	if (!vma && binder_alloc_is_mapped(alloc))
+		goto err_vma_lock_failed;
 
 	if (!mutex_trylock(&alloc->mutex))
 		goto err_get_alloc_mutex_failed;