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

[Bo Zhang]

Thanks for the detailed review. Replying to the three points below.

1) ABBA deadlock between install_mutex and mmap_lock

Sashiko says

"Can this locking order cause an ABBA deadlock with the shrinker path?
Here in binder_install_single_page(), alloc->install_mutex is acquired
first, followed by memory locks (per-VMA read lock or mmap_read_lock)
inside binder_page_insert(). However, in the shrinker callback
binder_alloc_free_page(), the locks are acquired in the reverse order.
If a concurrent writer is waiting on mmap_write_lock (which blocks new
readers), could this inversion result in a deadlock?"

This is a real issue and I'll fix it in v3. The install side holds
install_mutex and then falls back to a blocking mmap_read_lock(), while
the shrinker holds mmap_lock and then takes install_mutex. With a writer
pending on mmap_write_lock, the rwsem's writer-priority can turn this
inversion into a deadlock.

The fix is to never block on mmap_lock while holding install_mutex. The
mmap_lock fallback in binder_page_insert() becomes mmap_read_trylock(),
and on contention it returns -EAGAIN. The caller then drops install_mutex
and retries after waiting for mmap_lock to become available (without
holding any binder lock), so the install side never holds install_mutex
while blocked on mmap_lock. The per-VMA lock path is unaffected.

2) Potential infinite loop on -EBUSY

Sashiko says

"If vm_insert_page() persistently returns -EBUSY ... will this result in
an infinite loop? Because the -EAGAIN return from
binder_install_single_page() unconditionally jumps back to the retry
label without sleeping or limiting the number of attempts, could this
lead to a CPU lockup and local denial of service?"

Good catch. Under install_mutex the PTE should never be already
populated, so -EBUSY is not expected. In v3 I'll stop treating -EBUSY as
retryable: it returns an error instead of -EAGAIN, so it can never loop.
The -EAGAIN retry path is then only reached for genuine mmap_lock
contention, where the retry waits on mmap_lock rather than busy-looping.

3) Use-after-free of the preallocated next buffer on retry

Sashiko says

"Does retrying on -EAGAIN here result in a use-after-free of the next
buffer? ... the error path in binder_alloc_new_buf_locked() calls
kfree(new_buffer) ... If we jump back to the retry label and pass the
exact same freed pointer back into binder_alloc_new_buf_locked() ...
Additionally, if the buffer was split prior to the -EAGAIN failure, the
split is not rolled back."

You're right on both counts, thanks for catching this. In v3:

- binder_alloc_new_buf_locked() rolls back the split on the -EAGAIN
  path (rb_erase() + list_del() of the newly inserted free buffer),
  restoring the original free block.

- The preallocated 'next' buffer is (re)allocated inside the retry loop
  in binder_alloc_new_buf(), so a freed pointer is never reused.

I'll send v3 with all three fixes.

Bo