commit 9a9ab0d963621d9d12199df9817e66982582d5a5 upstream.
Task A calls binder_update_page_range() to allocate and insert pages on a remote address space from Task B. For this, Task A pins the remote mm via mmget_not_zero() first. This can race with Task B do_exit() and the final mmput() refcount decrement will come from Task A.
Task A | Task B ------------------+------------------ mmget_not_zero() | | do_exit() | exit_mm() | mmput() mmput() | exit_mmap() | remove_vma() | fput() |
In this case, the work of ____fput() from Task B is queued up in Task A as TWA_RESUME. So in theory, Task A returns to userspace and the cleanup work gets executed. However, Task A instead sleep, waiting for a reply from Task B that never comes (it's dead).
This means the binder_deferred_release() is blocked until an unrelated binder event forces Task A to go back to userspace. All the associated death notifications will also be delayed until then.
In order to fix this use mmput_async() that will schedule the work in the corresponding mm->async_put_work WQ instead of Task A.
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver") Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Carlos Llamas cmllamas@google.com Link: https://lore.kernel.org/r/20231201172212.1813387-4-cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org [cmllamas: fix trivial conflict with missing d8ed45c5dcd4.] Signed-off-by: Carlos Llamas cmllamas@google.com --- drivers/android/binder_alloc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 27950f901595..79cac74f0f1a 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -290,7 +290,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, } if (mm) { up_read(&mm->mmap_sem); - mmput(mm); + mmput_async(mm); } return 0;
@@ -325,7 +325,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, err_no_vma: if (mm) { up_read(&mm->mmap_sem); - mmput(mm); + mmput_async(mm); } return vma ? -ENOMEM : -ESRCH; }
base-commit: abc2dd6a248d443c27888aee13cfefd94c3f7407 prerequisite-patch-id: 040c7991c3b5fb63a9d12350a1400c91a057f7d5
commit c6d05e0762ab276102246d24affd1e116a46aa0c upstream.
Each transaction is associated with a 'struct binder_buffer' that stores the metadata about its buffer area. Since commit 74310e06be4d ("android: binder: Move buffer out of area shared with user space") this struct is no longer embedded within the buffer itself but is instead allocated on the heap to prevent userspace access to this driver-exclusive info.
Unfortunately, the space of this struct is still being accounted for in the total buffer size calculation, specifically for async transactions. This results in an additional 104 bytes added to every async buffer request, and this area is never used.
This wasted space can be substantial. If we consider the maximum mmap buffer space of SZ_4M, the driver will reserve half of it for async transactions, or 0x200000. This area should, in theory, accommodate up to 262,144 buffers of the minimum 8-byte size. However, after adding the extra 'sizeof(struct binder_buffer)', the total number of buffers drops to only 18,724, which is a sad 7.14% of the actual capacity.
This patch fixes the buffer size calculation to enable the utilization of the entire async buffer space. This is expected to reduce the number of -ENOSPC errors that are seen on the field.
Fixes: 74310e06be4d ("android: binder: Move buffer out of area shared with user space") Signed-off-by: Carlos Llamas cmllamas@google.com Reviewed-by: Alice Ryhl aliceryhl@google.com Link: https://lore.kernel.org/r/20231201172212.1813387-6-cmllamas@google.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org [cmllamas: fix trivial conflict with missing 261e7818f06e.] Signed-off-by: Carlos Llamas cmllamas@google.com ---
Notes: This patch depends on commit 3091c21d3e93 ("binder: fix async space check for 0-sized buffers") to apply cleanly which is currently queued up for stable trees.
drivers/android/binder_alloc.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 79cac74f0f1a..6eb5be798d55 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -402,8 +402,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked( /* Pad 0-size buffers so they get assigned unique addresses */ size = max(size, sizeof(void *));
- if (is_async && - alloc->free_async_space < size + sizeof(struct binder_buffer)) { + if (is_async && alloc->free_async_space < size) { binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: binder_alloc_buf size %zd failed, no async space left\n", alloc->pid, size); @@ -509,7 +508,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked( buffer->async_transaction = is_async; buffer->extra_buffers_size = extra_buffers_size; if (is_async) { - alloc->free_async_space -= size + sizeof(struct binder_buffer); + alloc->free_async_space -= size; binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC_ASYNC, "%d: binder_alloc_buf size %zd async free %zd\n", alloc->pid, size, alloc->free_async_space); @@ -631,8 +630,7 @@ static void binder_free_buf_locked(struct binder_alloc *alloc, BUG_ON(buffer->data > alloc->buffer + alloc->buffer_size);
if (buffer->async_transaction) { - alloc->free_async_space += buffer_size + sizeof(struct binder_buffer); - + alloc->free_async_space += buffer_size; binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC_ASYNC, "%d: binder_free_buf size %zd async free %zd\n", alloc->pid, size, alloc->free_async_space);
linux-stable-mirror@lists.linaro.org
-
Carlos Llamas