Re: [RFC PATCH v4 1/2] binder: switch alloc->mutex to spinlock for buffer metadata

[Bo Zhang]

Thanks for the review. Both are real regressions introduced by this
patch: the current tree uses a mutex here, and neither problem exists
under the mutex. Converting to a spinlock is what introduces them, so
they must be fixed rather than left as-is.

The root cause is that this patch (spinlock only) cannot fix either
problem on its own, because the fixes rely on the install_mutex added in
patch 2. I will therefore fold the two patches into one in v5, so the
spinlock and install_mutex land together and the intermediate state is
never reached.

1) Soft lockup holding the spinlock across cleanup

Sashiko says
"this loop iterates over all allocated buffers and pages, it can execute
up to 4MB of memset operations and 1024 calls to binder_free_page() while
preemption is disabled by alloc->lock."

Correct. Under the mutex this loop is preemptible; under the spinlock it
is not, so unprivileged userspace can keep a CPU with preemption disabled.
In v5, binder_alloc_deferred_release() drops alloc->lock around the
sleeping/long-running work: the clear-on-free memset and binder_free_page()
run outside the spinlock, while alloc->lock only covers the rb-tree and
LRU bookkeeping.

2) Use-after-free from the early spin_unlock() in the shrinker

Sashiko says
"By dropping alloc->lock here, the shrinker allows a concurrent
binder_alloc_deferred_release() ... to acquire the lock ... The release
function can then complete its cleanup ... and eventually free the
binder_alloc structure. When the shrinker resumes execution, it accesses
the freed alloc structure when calling trace_binder_unmap_user_end()."

Correct. Under the mutex the shrinker's zap and trace ran inside
alloc->mutex, which deferred_release() also took, so release waited for
the shrinker. Dropping the spinlock early breaks that. In v5 the shrinker
already holds install_mutex across the zap/trace (from the folded patch 2),
so binder_alloc_deferred_release() takes install_mutex too and waits for
the shrinker to finish before freeing the alloc.

Note that deferred_release() runs from binder_free_proc(), after all
threads are released and there are no in-flight transactions, so no page
install can run concurrently; the only concurrent writer to pages[] is the
shrinker, which install_mutex now serializes against.

v5 will fold the two patches and carry both fixes.

Bo