Re: [RFC PATCH 0/1] binder: switch alloc->mutex back to spinlock

[Bo Zhang]

On Thu, Aug 06, 2026 at 09:01:19AM +0000, Alice Ryhl wrote:
> This will not work due to the following race condition between T1 and T2:
> 
> T1 binder_alloc_free_page() removes the page from alloc->pages
> T2 binder_alloc_new_buf_locked() runs
> T2 binder_install_single_page() gets -EBUSY
> T2 binder_page_lookup() looks up the page in the vma
> T1 binder_alloc_free_page() calls zap_vma_range()
> 
> In this scenario, T2 looks up a page that it expects to be "pinned" and
> not removable by the shrinker, but the shrinker zaps it anyway.
> 
> Today this cannot happen because binder_alloc_new_buf_locked() runs
> under the mutex, which ensures that zap_vma_range() finishes removing
> the page before binder_install_single_page() runs.

Hi Alice,

Thanks for pointing out this race. You're right that simply moving
spin_unlock() before zap_vma_range() opens a window where the install
side could GUP the old page that is about to be freed.

However, I think it could be resolved without keeping the lock across
zap_vma_range(). The solution might be:

The install side only calls vm_insert_page() when pages[index] == NULL.
And the shrinker sets pages[index] = NULL before zapping. So if
vm_insert_page() returns -EBUSY, we can check pages[index] to determine
which scenario we're in:

  1. pages[index] != NULL: Another installer won the race. It's safe to
     GUP the page, which is the normal concurrent install case.

  2. pages[index] == NULL: The shrinker cleared it but hasn't zapped
     the PTE yet. We must NOT GUP the old page. Return -EAGAIN and
     let the caller retry after the shrinker finishes.

Case 2 is safe because: the install side only attempts installation
when pages[index] == NULL, so if EBUSY occurs with pages[index] == NULL,
it must be a PTE left over from the page that the shrinker is currently
reclaiming. After the shrinker zaps and frees, the retry will find
pages[index] still NULL, call vm_insert_page() again, and succeed since
the PTE is now clear.

Case 1 cannot race with the shrinker because: if pages[index] != NULL
at the time of the EBUSY check, the page was just installed by another
thread and is not on the LRU freelist, so the shrinker won't touch it.

The change would look like:

  case -EBUSY:
      binder_free_page(page);
      if (!binder_get_installed_page(alloc, index)) {
          /* shrinker is reclaiming, retry */
          ret = -EAGAIN;
          break;
      }
      /* normal concurrent install, look up the winner's page */
      page = binder_page_lookup(alloc, addr);
      ...

And the caller retries on -EAGAIN with a cond_resched().

This also should not introduce any performance regression since the
-EAGAIN retry only triggers in the rare case where EBUSY coincides with
an active shrinker reclaim on the same page index, a window that lasts
only for the duration of a single zap_vma_range() call.

Does this approach look reasonable to you?

Bo