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

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Aug 06, 2026 at 10:23:01PM +0800, Bo Zhang wrote:
> 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?

No, this will deadlock if the thread installing the page is SCHED_FIFO.

Waiting for the shrinker by repeatedly checking whether the shrinker is
done in a loop does not work.

As an aside, your solution also adds similar waiting for concurrent
installers. If the successful installer gets preempted between
binder_page_insert() and binder_set_installed_page(), then your code
will also repeatedly retry until said thread gets scheduled in again. So
there is a similar deadlock if the inserter that failed the race is
SCHED_FIFO.

Alice