Re: [RFC PATCH v3 2/2] binder: add install_mutex to serialize page install and shrinker zap

[Bo Zhang]

Thanks for the review. All three are valid; I'll address them together
in v4 by tightening the roles of the two locks: install_mutex only
serializes page install vs shrinker zap, while alloc->lock (spinlock)
exclusively owns pages[] and the LRU. Details per point below.

1) AA self-deadlock via direct reclaim

Sashiko says
"binder_install_single_page() acquires alloc->install_mutex here, then
calls binder_page_insert() which invokes vm_insert_page(). vm_insert_page()
can trigger page table allocations using GFP_KERNEL semantics, which may
enter direct reclaim. If direct reclaim iterates over list_lru shrinkers
and invokes binder_alloc_free_page() for this same allocator on the same
thread, the shrinker callback will unconditionally try to lock the
already-held install_mutex."

Correct. In v4 the shrinker uses mutex_trylock(&install_mutex) and skips
the page (LRU_SKIP) on failure, so a reclaim recursion from the install
side's vm_insert_page() cannot deadlock on the same thread. The original
shrinker already used trylock on alloc->mutex; restoring trylock here also
removes the ABBA concern with mmap_lock, since a trylock does not
participate in a blocking lock cycle.

2) RT task livelock on the -EAGAIN retry

Sashiko says
"a race window exists where the shrinker has isolated a page from the
buffer's range (causing list_lru_del to fail and return -EAGAIN) but is
preempted before acquiring install_mutex. Because the mutex is uncontended,
the allocating thread acquires and drops it instantly, thinks the shrinker
is done, and retries. It will again find the page installed, fail
list_lru_del, and spin in a tight loop."

Correct. The root cause is that the current approach splits the
shrinker's pages[index]=NULL and list_lru_isolate() such that
binder_lru_freelist_del() can observe an inconsistent state. In v4 the
shrinker performs pages[index]=NULL and list_lru_isolate() together under
alloc->lock, so binder_lru_freelist_del() always sees a consistent
pages[]/LRU state and list_lru_del() never fails. This removes the
-EAGAIN retry path entirely, so the livelock cannot occur.

3) Use-after-free from clearing pages[index] outside alloc->lock

Sashiko says
"By moving binder_set_installed_page(alloc, index, NULL) outside the
alloc->lock critical section, a concurrent allocating thread holding
alloc->lock in binder_lru_freelist_del() can read the stale pointer:
page = alloc->pages[index] (which is not yet NULL). ... When the allocating
thread resumes, it will pass the dangling pointer to page_to_lru(page) and
page_to_nid(page), dereferencing the freed page_private."

Correct. In v4 pages[index]=NULL is moved back inside alloc->lock (together
with list_lru_isolate), so binder_lru_freelist_del() reading pages[index]
under alloc->lock can no longer observe a pointer that the shrinker is
about to free.

v4 will carry all three fixes.

Bo