[PATCH v2 00/28] binder: convert alloc->mutex to spinlock

[PATCH v2 00/28] binder: convert alloc->mutex to spinlock

[Carlos Llamas]

This series refactors the binder buffer allocation space to be able to
convert the alloc->mutex into a spinlock. Doing so decreases the latency
of binder transactions seen under worst-case scenarios.

I attempted to organize and improve readability of things that were
touched during this process. I also include a couple of fixes (unrelated
to the lock conversion) for issues I found along the way.

Regards,
Carlos Llamas

Cc: Todd Kjos <tkjos@google.com>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: John Stultz <jstultz@google.com>
Cc: Suren Baghdasaryan <surenb@google.com> 
Cc: Tim Murray <timmurray@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

v2:
 - New fix for wasted alloc->free_sync_space calculation
 - Fix issues pointed out by Alice: incorrect size overflow check, data
   race in buffer->async_transaction, reorder of __free_page(page) and
   new_buffer leak in some error paths.
 - Better naming and comments to accommodate the new scope of things
   e.g. binder_lru_freelist_add() and binder_install_single_page().
 - Dropped patch ("binder: do not add pages to LRU in release path")
   since it is not worth the added complexity.
 - Avoid _new_ logs of unhashed userspace addresses.
 - More minor cosmeting changes.
 - Add Reviewed-by tags.

v1:
   https://lore.kernel.org/all/20231102185934.773885-1-cmllamas@google.com/

Carlos Llamas (28):
  binder: use EPOLLERR from eventpoll.h
  binder: fix use-after-free in shinker's callback
  binder: fix race between mmput() and do_exit()
  binder: fix async space check for 0-sized buffers
  binder: fix unused alloc->free_async_space
  binder: fix trivial typo of binder_free_buf_locked()
  binder: fix comment on binder_alloc_new_buf() return value
  binder: remove extern from function prototypes
  binder: keep vma addresses type as unsigned long
  binder: split up binder_update_page_range()
  binder: do unlocked work in binder_alloc_new_buf()
  binder: remove pid param in binder_alloc_new_buf()
  binder: separate the no-space debugging logic
  binder: relocate low space calculation
  binder: relocate binder_alloc_clear_buf()
  binder: refactor page range allocation
  binder: malloc new_buffer outside of locks
  binder: initialize lru pages in mmap callback
  binder: perform page installation outside of locks
  binder: remove redundant debug log
  binder: make oversized buffer code more readable
  binder: rename lru shrinker utilities
  binder: document the final page calculation
  binder: collapse print_binder_buffer() into caller
  binder: refactor binder_delete_free_buffer()
  binder: avoid user addresses in debug logs
  binder: reverse locking order in shrinker callback
  binder: switch alloc->mutex to spinlock_t

 drivers/android/binder.c                |  27 +-
 drivers/android/binder_alloc.c          | 858 ++++++++++++------------
 drivers/android/binder_alloc.h          |  61 +-
 drivers/android/binder_alloc_selftest.c |  14 +-
 drivers/android/binder_trace.h          |   2 +-
 5 files changed, 489 insertions(+), 473 deletions(-)


base-commit: 994d5c58e50e91bb02c7be4a91d5186292a895c8
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 01/28] binder: use EPOLLERR from eventpoll.h

[Carlos Llamas]

Use EPOLLERR instead of POLLERR to make sure it is cast to the correct
__poll_t type. This fixes the following sparse issue:

  drivers/android/binder.c:5030:24: warning: incorrect type in return expression (different base types)
  drivers/android/binder.c:5030:24:    expected restricted __poll_t
  drivers/android/binder.c:5030:24:    got int

Fixes: f88982679f54 ("binder: check for binder_thread allocation failure in binder_poll()")
Cc: stable@vger.kernel.org
Cc: Eric Biggers <ebiggers@google.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 92128aae2d06..71a40a4c546f 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5030,7 +5030,7 @@ static __poll_t binder_poll(struct file *filp,
 
 	thread = binder_get_thread(proc);
 	if (!thread)
-		return POLLERR;
+		return EPOLLERR;
 
 	binder_inner_proc_lock(thread->proc);
 	thread->looper |= BINDER_LOOPER_STATE_POLL;
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 02/28] binder: fix use-after-free in shinker's callback

[Carlos Llamas]

The mmap read lock is used during the shrinker's callback, which means
that using alloc->vma pointer isn't safe as it can race with munmap().
As of commit dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in
munmap") the mmap lock is downgraded after the vma has been isolated.

I was able to reproduce this issue by manually adding some delays and
triggering page reclaiming through the shrinker's debug sysfs. The
following KASAN report confirms the UAF:

  ==================================================================
  BUG: KASAN: slab-use-after-free in zap_page_range_single+0x470/0x4b8
  Read of size 8 at addr ffff356ed50e50f0 by task bash/478

  CPU: 1 PID: 478 Comm: bash Not tainted 6.6.0-rc5-00055-g1c8b86a3799f-dirty #70
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   zap_page_range_single+0x470/0x4b8
   binder_alloc_free_page+0x608/0xadc
   __list_lru_walk_one+0x130/0x3b0
   list_lru_walk_node+0xc4/0x22c
   binder_shrink_scan+0x108/0x1dc
   shrinker_debugfs_scan_write+0x2b4/0x500
   full_proxy_write+0xd4/0x140
   vfs_write+0x1ac/0x758
   ksys_write+0xf0/0x1dc
   __arm64_sys_write+0x6c/0x9c

  Allocated by task 492:
   kmem_cache_alloc+0x130/0x368
   vm_area_alloc+0x2c/0x190
   mmap_region+0x258/0x18bc
   do_mmap+0x694/0xa60
   vm_mmap_pgoff+0x170/0x29c
   ksys_mmap_pgoff+0x290/0x3a0
   __arm64_sys_mmap+0xcc/0x144

  Freed by task 491:
   kmem_cache_free+0x17c/0x3c8
   vm_area_free_rcu_cb+0x74/0x98
   rcu_core+0xa38/0x26d4
   rcu_core_si+0x10/0x1c
   __do_softirq+0x2fc/0xd24

  Last potentially related work creation:
   __call_rcu_common.constprop.0+0x6c/0xba0
   call_rcu+0x10/0x1c
   vm_area_free+0x18/0x24
   remove_vma+0xe4/0x118
   do_vmi_align_munmap.isra.0+0x718/0xb5c
   do_vmi_munmap+0xdc/0x1fc
   __vm_munmap+0x10c/0x278
   __arm64_sys_munmap+0x58/0x7c

Fix this issue by performing instead a vma_lookup() which will fail to
find the vma that was isolated before the mmap lock downgrade. Note that
this option has better performance than upgrading to a mmap write lock
which would increase contention. Plus, mmap_write_trylock() has been
recently removed anyway.

Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
Cc: stable@vger.kernel.org
Cc: Liam Howlett <liam.howlett@oracle.com>
Cc: Minchan Kim <minchan@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 138f6d43d13b..9d2eff70c3ba 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1005,7 +1005,9 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 		goto err_mmget;
 	if (!mmap_read_trylock(mm))
 		goto err_mmap_read_lock_failed;
-	vma = binder_alloc_get_vma(alloc);
+	vma = vma_lookup(mm, page_addr);
+	if (vma && vma != binder_alloc_get_vma(alloc))
+		goto err_invalid_vma;
 
 	list_lru_isolate(lru, item);
 	spin_unlock(lock);
@@ -1031,6 +1033,8 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	mutex_unlock(&alloc->mutex);
 	return LRU_REMOVED_RETRY;
 
+err_invalid_vma:
+	mmap_read_unlock(mm);
 err_mmap_read_lock_failed:
 	mmput_async(mm);
 err_mmget:
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 03/28] binder: fix race between mmput() and do_exit()

[Carlos Llamas]

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>
---
 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 9d2eff70c3ba..adcec5ec0959 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -271,7 +271,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 	}
 	if (mm) {
 		mmap_write_unlock(mm);
-		mmput(mm);
+		mmput_async(mm);
 	}
 	return 0;
 
@@ -304,7 +304,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 err_no_vma:
 	if (mm) {
 		mmap_write_unlock(mm);
-		mmput(mm);
+		mmput_async(mm);
 	}
 	return vma ? -ENOMEM : -ESRCH;
 }
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 03/28] binder: fix race between mmput() and do_exit()

[Carlos Llamas]

On Fri, Dec 01, 2023 at 05:21:32PM +0000, Carlos Llamas wrote:
> 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>
> ---

Sorry, I forgot to Cc: stable@vger.kernel.org.

--
Carlos Llamas

Re: [PATCH v2 03/28] binder: fix race between mmput() and do_exit()

[Greg Kroah-Hartman]

On Thu, Jan 18, 2024 at 07:29:07PM +0000, Carlos Llamas wrote:
> On Fri, Dec 01, 2023 at 05:21:32PM +0000, Carlos Llamas wrote:
> > 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>
> > ---
> 
> Sorry, I forgot to Cc: stable@vger.kernel.org.

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH v2 03/28] binder: fix race between mmput() and do_exit()

[Carlos Llamas]

On Fri, Jan 19, 2024 at 06:48:43AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 18, 2024 at 07:29:07PM +0000, Carlos Llamas wrote:
> > On Fri, Dec 01, 2023 at 05:21:32PM +0000, Carlos Llamas wrote:
> > > 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>
> > > ---
> > 
> > Sorry, I forgot to Cc: stable@vger.kernel.org.
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>

Oops, here is the complete info:

Commit ID: 9a9ab0d963621d9d12199df9817e66982582d5a5
Subject:   "binder: fix race between mmput() and do_exit()"
Reason:    Fixes a race condition in binder.
Versions:  v4.19+

Note this will have a trivial conflict in v4.19 and v5.10 kernels as
commit d8ed45c5dcd4 is not there. Please let me know if I should send
those patches separately.

Thanks,
--
Carlos Llamas

Re: [PATCH v2 03/28] binder: fix race between mmput() and do_exit()

[Carlos Llamas]

On Fri, Jan 19, 2024 at 05:06:13PM +0000, Carlos Llamas wrote:
> 
> Oops, here is the complete info:
> 
> Commit ID: 9a9ab0d963621d9d12199df9817e66982582d5a5
> Subject:   "binder: fix race between mmput() and do_exit()"
> Reason:    Fixes a race condition in binder.
> Versions:  v4.19+
> 
> Note this will have a trivial conflict in v4.19 and v5.10 kernels as
> commit d8ed45c5dcd4 is not there. Please let me know if I should send
> those patches separately.
> 
> Thanks,
> --
> Carlos Llamas

Sigh, I meant to type "conflict in v4.19 and v5.4". The patch applies
cleanly in v5.10+.

Re: [PATCH v2 03/28] binder: fix race between mmput() and do_exit()

[Greg Kroah-Hartman]

On Fri, Jan 19, 2024 at 05:37:22PM +0000, Carlos Llamas wrote:
> On Fri, Jan 19, 2024 at 05:06:13PM +0000, Carlos Llamas wrote:
> > 
> > Oops, here is the complete info:
> > 
> > Commit ID: 9a9ab0d963621d9d12199df9817e66982582d5a5
> > Subject:   "binder: fix race between mmput() and do_exit()"
> > Reason:    Fixes a race condition in binder.
> > Versions:  v4.19+
> > 
> > Note this will have a trivial conflict in v4.19 and v5.10 kernels as
> > commit d8ed45c5dcd4 is not there. Please let me know if I should send
> > those patches separately.
> > 
> > Thanks,
> > --
> > Carlos Llamas
> 
> Sigh, I meant to type "conflict in v4.19 and v5.4". The patch applies
> cleanly in v5.10+.

Yes, I need backported patches please.

thanks,

greg k-h

Re: [PATCH v2 03/28] binder: fix race between mmput() and do_exit()

[Carlos Llamas]

On Sat, Jan 20, 2024 at 07:37:25AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Jan 19, 2024 at 05:37:22PM +0000, Carlos Llamas wrote:
> > On Fri, Jan 19, 2024 at 05:06:13PM +0000, Carlos Llamas wrote:
> > > 
> > > Oops, here is the complete info:
> > > 
> > > Commit ID: 9a9ab0d963621d9d12199df9817e66982582d5a5
> > > Subject:   "binder: fix race between mmput() and do_exit()"
> > > Reason:    Fixes a race condition in binder.
> > > Versions:  v4.19+
> > > 
> > > Note this will have a trivial conflict in v4.19 and v5.10 kernels as
> > > commit d8ed45c5dcd4 is not there. Please let me know if I should send
> > > those patches separately.
> > > 
> > > Thanks,
> > > --
> > > Carlos Llamas
> > 
> > Sigh, I meant to type "conflict in v4.19 and v5.4". The patch applies
> > cleanly in v5.10+.
> 
> Yes, I need backported patches please.
> 
> thanks,
> 
> greg k-h

Backports have been sent.

linux-4.19.y:
https://lore.kernel.org/all/20240122174250.2123854-1-cmllamas@google.com/

linux-5.4.y:
https://lore.kernel.org/all/20240122175751.2214176-1-cmllamas@google.com/

The patch should apply cleanly in remaining stable branches.

Thanks,
--
Carlos Llamas

[PATCH v2 04/28] binder: fix async space check for 0-sized buffers

[Carlos Llamas]

Move the padding of 0-sized buffers to an earlier stage to account for
this round up during the alloc->free_async_space check.

Fixes: 74310e06be4d ("android: binder: Move buffer out of area shared with user space")
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index adcec5ec0959..abff1bafcc43 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -407,6 +407,10 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 				alloc->pid, extra_buffers_size);
 		return ERR_PTR(-EINVAL);
 	}
+
+	/* 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)) {
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
@@ -415,9 +419,6 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		return ERR_PTR(-ENOSPC);
 	}
 
-	/* Pad 0-size buffers so they get assigned unique addresses */
-	size = max(size, sizeof(void *));
-
 	while (n) {
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
 		BUG_ON(!buffer->free);
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 04/28] binder: fix async space check for 0-sized buffers

[Carlos Llamas]

On Fri, Dec 01, 2023 at 05:21:33PM +0000, Carlos Llamas wrote:
> Move the padding of 0-sized buffers to an earlier stage to account for
> this round up during the alloc->free_async_space check.
> 
> Fixes: 74310e06be4d ("android: binder: Move buffer out of area shared with user space")
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---

Sorry, I forgot to Cc: stable@vger.kernel.org.

--
Carlos Llamas

Re: [PATCH v2 04/28] binder: fix async space check for 0-sized buffers

[Greg Kroah-Hartman]

On Thu, Jan 18, 2024 at 07:32:08PM +0000, Carlos Llamas wrote:
> On Fri, Dec 01, 2023 at 05:21:33PM +0000, Carlos Llamas wrote:
> > Move the padding of 0-sized buffers to an earlier stage to account for
> > this round up during the alloc->free_async_space check.
> > 
> > Fixes: 74310e06be4d ("android: binder: Move buffer out of area shared with user space")
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> 
> Sorry, I forgot to Cc: stable@vger.kernel.org.


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH v2 04/28] binder: fix async space check for 0-sized buffers

[Carlos Llamas]

On Fri, Jan 19, 2024 at 06:48:53AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 18, 2024 at 07:32:08PM +0000, Carlos Llamas wrote:
> > On Fri, Dec 01, 2023 at 05:21:33PM +0000, Carlos Llamas wrote:
> > > Move the padding of 0-sized buffers to an earlier stage to account for
> > > this round up during the alloc->free_async_space check.
> > > 
> > > Fixes: 74310e06be4d ("android: binder: Move buffer out of area shared with user space")
> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > ---
> > 
> > Sorry, I forgot to Cc: stable@vger.kernel.org.
> 
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>

Oops, here is the complete info:

Commit ID: 3091c21d3e9322428691ce0b7a0cfa9c0b239eeb
Subject:   "binder: fix async space check for 0-sized buffers"
Reason:    Fixes an incorrect calculation of available space.
Versions:  v4.19+

Thanks,
--
Carlos Llamas

Re: [PATCH v2 04/28] binder: fix async space check for 0-sized buffers

[Greg Kroah-Hartman]

On Fri, Jan 19, 2024 at 05:11:26PM +0000, Carlos Llamas wrote:
> On Fri, Jan 19, 2024 at 06:48:53AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 18, 2024 at 07:32:08PM +0000, Carlos Llamas wrote:
> > > On Fri, Dec 01, 2023 at 05:21:33PM +0000, Carlos Llamas wrote:
> > > > Move the padding of 0-sized buffers to an earlier stage to account for
> > > > this round up during the alloc->free_async_space check.
> > > > 
> > > > Fixes: 74310e06be4d ("android: binder: Move buffer out of area shared with user space")
> > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > > ---
> > > 
> > > Sorry, I forgot to Cc: stable@vger.kernel.org.
> > 
> > 
> > <formletter>
> > 
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read:
> >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> > 
> > </formletter>
> 
> Oops, here is the complete info:
> 
> Commit ID: 3091c21d3e9322428691ce0b7a0cfa9c0b239eeb
> Subject:   "binder: fix async space check for 0-sized buffers"
> Reason:    Fixes an incorrect calculation of available space.
> Versions:  v4.19+

Now queued up, thanks.

greg k-h

[PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Carlos Llamas]

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>
---
 drivers/android/binder_alloc.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index abff1bafcc43..9b5c4d446efa 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -344,8 +344,7 @@ static bool debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
 			continue;
 		if (!buffer->async_transaction)
 			continue;
-		total_alloc_size += binder_alloc_buffer_size(alloc, buffer)
-			+ sizeof(struct binder_buffer);
+		total_alloc_size += binder_alloc_buffer_size(alloc, buffer);
 		num_buffers++;
 	}
 
@@ -411,8 +410,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);
@@ -520,7 +518,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	buffer->pid = pid;
 	buffer->oneway_spam_suspect = false;
 	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);
@@ -658,8 +656,7 @@ static void binder_free_buf_locked(struct binder_alloc *alloc,
 	BUG_ON(buffer->user_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);
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Alice Ryhl]

> 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>

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Carlos Llamas]

On Fri, Dec 01, 2023 at 05:21:34PM +0000, Carlos Llamas wrote:
> 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>
> ---

Sorry, I forgot to Cc: stable@vger.kernel.org.

--
Carlos Llamas

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Greg Kroah-Hartman]

On Thu, Jan 18, 2024 at 07:33:48PM +0000, Carlos Llamas wrote:
> On Fri, Dec 01, 2023 at 05:21:34PM +0000, Carlos Llamas wrote:
> > 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>
> > ---
> 
> Sorry, I forgot to Cc: stable@vger.kernel.org.


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Carlos Llamas]

On Fri, Jan 19, 2024 at 06:49:00AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 18, 2024 at 07:33:48PM +0000, Carlos Llamas wrote:
> > On Fri, Dec 01, 2023 at 05:21:34PM +0000, Carlos Llamas wrote:
> > > 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>
> > > ---
> > 
> > Sorry, I forgot to Cc: stable@vger.kernel.org.
> 
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>

Oops, here is the complete info:

Commit ID: c6d05e0762ab276102246d24affd1e116a46aa0c
Subject:   "binder: fix unused alloc->free_async_space"
Reason:    Fixes an incorrect calculation of available space.
Versions:  v4.19+

Note this patch will also have trivial conflicts in v4.19 and v5.4
kernels as commit 261e7818f06e is missing there. Please let me know and
I can send the corresponding patches separately.

Thanks,
--
Carlos Llamas

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Greg Kroah-Hartman]

On Fri, Jan 19, 2024 at 05:27:18PM +0000, Carlos Llamas wrote:
> On Fri, Jan 19, 2024 at 06:49:00AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 18, 2024 at 07:33:48PM +0000, Carlos Llamas wrote:
> > > On Fri, Dec 01, 2023 at 05:21:34PM +0000, Carlos Llamas wrote:
> > > > 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>
> > > > ---
> > > 
> > > Sorry, I forgot to Cc: stable@vger.kernel.org.
> > 
> > 
> > <formletter>
> > 
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read:
> >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> > 
> > </formletter>
> 
> Oops, here is the complete info:
> 
> Commit ID: c6d05e0762ab276102246d24affd1e116a46aa0c
> Subject:   "binder: fix unused alloc->free_async_space"
> Reason:    Fixes an incorrect calculation of available space.
> Versions:  v4.19+
> 
> Note this patch will also have trivial conflicts in v4.19 and v5.4
> kernels as commit 261e7818f06e is missing there. Please let me know and
> I can send the corresponding patches separately.

It doesn't even apply to 6.7.y either, so we need backports for all
affected trees, thanks.

greg k-h

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Greg Kroah-Hartman]

On Mon, Jan 22, 2024 at 07:04:20AM -0800, Greg Kroah-Hartman wrote:
> On Fri, Jan 19, 2024 at 05:27:18PM +0000, Carlos Llamas wrote:
> > On Fri, Jan 19, 2024 at 06:49:00AM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Jan 18, 2024 at 07:33:48PM +0000, Carlos Llamas wrote:
> > > > On Fri, Dec 01, 2023 at 05:21:34PM +0000, Carlos Llamas wrote:
> > > > > 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>
> > > > > ---
> > > > 
> > > > Sorry, I forgot to Cc: stable@vger.kernel.org.
> > > 
> > > 
> > > <formletter>
> > > 
> > > This is not the correct way to submit patches for inclusion in the
> > > stable kernel tree.  Please read:
> > >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > > for how to do this properly.
> > > 
> > > </formletter>
> > 
> > Oops, here is the complete info:
> > 
> > Commit ID: c6d05e0762ab276102246d24affd1e116a46aa0c
> > Subject:   "binder: fix unused alloc->free_async_space"
> > Reason:    Fixes an incorrect calculation of available space.
> > Versions:  v4.19+
> > 
> > Note this patch will also have trivial conflicts in v4.19 and v5.4
> > kernels as commit 261e7818f06e is missing there. Please let me know and
> > I can send the corresponding patches separately.
> 
> It doesn't even apply to 6.7.y either, so we need backports for all
> affected trees, thanks.

Now I got it to apply, but we need backports for 5.4.y and 4.19.y,
thanks.

greg k-h

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Carlos Llamas]

On Mon, Jan 22, 2024 at 07:05:29AM -0800, Greg Kroah-Hartman wrote:
> On Mon, Jan 22, 2024 at 07:04:20AM -0800, Greg Kroah-Hartman wrote:
> > On Fri, Jan 19, 2024 at 05:27:18PM +0000, Carlos Llamas wrote:
> > > On Fri, Jan 19, 2024 at 06:49:00AM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Jan 18, 2024 at 07:33:48PM +0000, Carlos Llamas wrote:
> > > > > On Fri, Dec 01, 2023 at 05:21:34PM +0000, Carlos Llamas wrote:
> > > > > > 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>
> > > > > > ---
> > > > > 
> > > > > Sorry, I forgot to Cc: stable@vger.kernel.org.
> > > > 
> > > > 
> > > > <formletter>
> > > > 
> > > > This is not the correct way to submit patches for inclusion in the
> > > > stable kernel tree.  Please read:
> > > >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > > > for how to do this properly.
> > > > 
> > > > </formletter>
> > > 
> > > Oops, here is the complete info:
> > > 
> > > Commit ID: c6d05e0762ab276102246d24affd1e116a46aa0c
> > > Subject:   "binder: fix unused alloc->free_async_space"
> > > Reason:    Fixes an incorrect calculation of available space.
> > > Versions:  v4.19+
> > > 
> > > Note this patch will also have trivial conflicts in v4.19 and v5.4
> > > kernels as commit 261e7818f06e is missing there. Please let me know and
> > > I can send the corresponding patches separately.
> > 
> > It doesn't even apply to 6.7.y either, so we need backports for all
> > affected trees, thanks.
> 
> Now I got it to apply, but we need backports for 5.4.y and 4.19.y,
> thanks.
> 
> greg k-h

Backports sent.

linux-4.19.y:
https://lore.kernel.org/all/20240122174250.2123854-2-cmllamas@google.com/

linux-5.4.y:
https://lore.kernel.org/all/20240122175751.2214176-2-cmllamas@google.com/

Thanks,
Carlos Llamas

Re: [PATCH v2 05/28] binder: fix unused alloc->free_async_space

[Greg Kroah-Hartman]

On Mon, Jan 22, 2024 at 06:08:36PM +0000, Carlos Llamas wrote:
> On Mon, Jan 22, 2024 at 07:05:29AM -0800, Greg Kroah-Hartman wrote:
> > On Mon, Jan 22, 2024 at 07:04:20AM -0800, Greg Kroah-Hartman wrote:
> > > On Fri, Jan 19, 2024 at 05:27:18PM +0000, Carlos Llamas wrote:
> > > > On Fri, Jan 19, 2024 at 06:49:00AM +0100, Greg Kroah-Hartman wrote:
> > > > > On Thu, Jan 18, 2024 at 07:33:48PM +0000, Carlos Llamas wrote:
> > > > > > On Fri, Dec 01, 2023 at 05:21:34PM +0000, Carlos Llamas wrote:
> > > > > > > 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>
> > > > > > > ---
> > > > > > 
> > > > > > Sorry, I forgot to Cc: stable@vger.kernel.org.
> > > > > 
> > > > > 
> > > > > <formletter>
> > > > > 
> > > > > This is not the correct way to submit patches for inclusion in the
> > > > > stable kernel tree.  Please read:
> > > > >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > > > > for how to do this properly.
> > > > > 
> > > > > </formletter>
> > > > 
> > > > Oops, here is the complete info:
> > > > 
> > > > Commit ID: c6d05e0762ab276102246d24affd1e116a46aa0c
> > > > Subject:   "binder: fix unused alloc->free_async_space"
> > > > Reason:    Fixes an incorrect calculation of available space.
> > > > Versions:  v4.19+
> > > > 
> > > > Note this patch will also have trivial conflicts in v4.19 and v5.4
> > > > kernels as commit 261e7818f06e is missing there. Please let me know and
> > > > I can send the corresponding patches separately.
> > > 
> > > It doesn't even apply to 6.7.y either, so we need backports for all
> > > affected trees, thanks.
> > 
> > Now I got it to apply, but we need backports for 5.4.y and 4.19.y,
> > thanks.
> > 
> > greg k-h
> 
> Backports sent.
> 
> linux-4.19.y:
> https://lore.kernel.org/all/20240122174250.2123854-2-cmllamas@google.com/
> 
> linux-5.4.y:
> https://lore.kernel.org/all/20240122175751.2214176-2-cmllamas@google.com/

All now queued up, thanks!

greg k-h

[PATCH v2 06/28] binder: fix trivial typo of binder_free_buf_locked()

[Carlos Llamas]

Fix minor misspelling of the function in the comment section.

No functional changes in this patch.

Cc: stable@vger.kernel.org
Fixes: 0f966cba95c7 ("binder: add flag to clear buffer on txn complete")
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 9b5c4d446efa..a124d2743c69 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -704,7 +704,7 @@ void binder_alloc_free_buf(struct binder_alloc *alloc,
 	/*
 	 * We could eliminate the call to binder_alloc_clear_buf()
 	 * from binder_alloc_deferred_release() by moving this to
-	 * binder_alloc_free_buf_locked(). However, that could
+	 * binder_free_buf_locked(). However, that could
 	 * increase contention for the alloc mutex if clear_on_free
 	 * is used frequently for large buffers. The mutex is not
 	 * needed for correctness here.
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 07/28] binder: fix comment on binder_alloc_new_buf() return value

[Carlos Llamas]

Update the comments of binder_alloc_new_buf() to reflect that the return
value of the function is now ERR_PTR(-errno) on failure.

No functional changes in this patch.

Cc: stable@vger.kernel.org
Fixes: 57ada2fb2250 ("binder: add log information for binder transaction failures")
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index a124d2743c69..a56cbfd9ba44 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -556,7 +556,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
  * is the sum of the three given sizes (each rounded up to
  * pointer-sized boundary)
  *
- * Return:	The allocated buffer or %NULL if error
+ * Return:	The allocated buffer or %ERR_PTR(-errno) if error
  */
 struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 					   size_t data_size,
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 08/28] binder: remove extern from function prototypes

[Carlos Llamas]

The kernel coding style does not require 'extern' in function prototypes
in .h files, so remove them from drivers/android/binder_alloc.h as they
are not needed.

No functional changes in this patch.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.h | 38 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index dc1e2b01dd64..82380febdd85 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -121,27 +121,27 @@ static inline void binder_selftest_alloc(struct binder_alloc *alloc) {}
 enum lru_status binder_alloc_free_page(struct list_head *item,
 				       struct list_lru_one *lru,
 				       spinlock_t *lock, void *cb_arg);
-extern struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
-						  size_t data_size,
-						  size_t offsets_size,
-						  size_t extra_buffers_size,
-						  int is_async,
-						  int pid);
-extern void binder_alloc_init(struct binder_alloc *alloc);
-extern int binder_alloc_shrinker_init(void);
-extern void binder_alloc_shrinker_exit(void);
-extern void binder_alloc_vma_close(struct binder_alloc *alloc);
-extern struct binder_buffer *
+struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
+					   size_t data_size,
+					   size_t offsets_size,
+					   size_t extra_buffers_size,
+					   int is_async,
+					   int pid);
+void binder_alloc_init(struct binder_alloc *alloc);
+int binder_alloc_shrinker_init(void);
+void binder_alloc_shrinker_exit(void);
+void binder_alloc_vma_close(struct binder_alloc *alloc);
+struct binder_buffer *
 binder_alloc_prepare_to_free(struct binder_alloc *alloc,
 			     uintptr_t user_ptr);
-extern void binder_alloc_free_buf(struct binder_alloc *alloc,
-				  struct binder_buffer *buffer);
-extern int binder_alloc_mmap_handler(struct binder_alloc *alloc,
-				     struct vm_area_struct *vma);
-extern void binder_alloc_deferred_release(struct binder_alloc *alloc);
-extern int binder_alloc_get_allocated_count(struct binder_alloc *alloc);
-extern void binder_alloc_print_allocated(struct seq_file *m,
-					 struct binder_alloc *alloc);
+void binder_alloc_free_buf(struct binder_alloc *alloc,
+			   struct binder_buffer *buffer);
+int binder_alloc_mmap_handler(struct binder_alloc *alloc,
+			      struct vm_area_struct *vma);
+void binder_alloc_deferred_release(struct binder_alloc *alloc);
+int binder_alloc_get_allocated_count(struct binder_alloc *alloc);
+void binder_alloc_print_allocated(struct seq_file *m,
+				  struct binder_alloc *alloc);
 void binder_alloc_print_pages(struct seq_file *m,
 			      struct binder_alloc *alloc);
 
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 09/28] binder: keep vma addresses type as unsigned long

[Carlos Llamas]

The vma addresses in binder are currently stored as void __user *. This
requires casting back and forth between the mm/ api which uses unsigned
long. Since we also do internal arithmetic on these addresses we end up
having to cast them _again_ to an integer type.

Lets stop all the unnecessary casting which kills code readability and
store the virtual addresses as the native unsigned long from mm/. Note
that this approach is preferred over uintptr_t as Linus explains in [1].

Opportunistically add a few cosmetic touchups.

Link: https://lore.kernel.org/all/CAHk-=wj2OHy-5e+srG1fy+ZU00TmZ1NFp6kFLbVLMXHe7A1d-g@mail.gmail.com/ [1]
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c                | 21 +++---
 drivers/android/binder_alloc.c          | 91 +++++++++++--------------
 drivers/android/binder_alloc.h          |  6 +-
 drivers/android/binder_alloc_selftest.c |  6 +-
 drivers/android/binder_trace.h          |  2 +-
 5 files changed, 57 insertions(+), 69 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 71a40a4c546f..437d1097b118 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2077,9 +2077,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 			 * Convert the address to an offset relative to
 			 * the base of the transaction buffer.
 			 */
-			fda_offset =
-			    (parent->buffer - (uintptr_t)buffer->user_data) +
-			    fda->parent_offset;
+			fda_offset = parent->buffer - buffer->user_data +
+				fda->parent_offset;
 			for (fd_index = 0; fd_index < fda->num_fds;
 			     fd_index++) {
 				u32 fd;
@@ -2597,7 +2596,7 @@ static int binder_translate_fd_array(struct list_head *pf_head,
 	 * Convert the address to an offset relative to
 	 * the base of the transaction buffer.
 	 */
-	fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) +
+	fda_offset = parent->buffer - t->buffer->user_data +
 		fda->parent_offset;
 	sender_ufda_base = (void __user *)(uintptr_t)sender_uparent->buffer +
 				fda->parent_offset;
@@ -2672,8 +2671,9 @@ static int binder_fixup_parent(struct list_head *pf_head,
 				  proc->pid, thread->pid);
 		return -EINVAL;
 	}
-	buffer_offset = bp->parent_offset +
-			(uintptr_t)parent->buffer - (uintptr_t)b->user_data;
+
+	buffer_offset = bp->parent_offset + parent->buffer - b->user_data;
+
 	return binder_add_fixup(pf_head, buffer_offset, bp->buffer, 0);
 }
 
@@ -3250,7 +3250,7 @@ static void binder_transaction(struct binder_proc *proc,
 				    ALIGN(extra_buffers_size, sizeof(void *)) -
 				    ALIGN(secctx_sz, sizeof(u64));
 
-		t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
+		t->security_ctx = t->buffer->user_data + buf_offset;
 		err = binder_alloc_copy_to_buffer(&target_proc->alloc,
 						  t->buffer, buf_offset,
 						  secctx, secctx_sz);
@@ -3527,8 +3527,7 @@ static void binder_transaction(struct binder_proc *proc,
 				goto err_translate_failed;
 			}
 			/* Fixup buffer pointer to target proc address space */
-			bp->buffer = (uintptr_t)
-				t->buffer->user_data + sg_buf_offset;
+			bp->buffer = t->buffer->user_data + sg_buf_offset;
 			sg_buf_offset += ALIGN(bp->length, sizeof(u64));
 
 			num_valid = (buffer_offset - off_start_offset) /
@@ -4698,7 +4697,7 @@ static int binder_thread_read(struct binder_proc *proc,
 		}
 		trd->data_size = t->buffer->data_size;
 		trd->offsets_size = t->buffer->offsets_size;
-		trd->data.ptr.buffer = (uintptr_t)t->buffer->user_data;
+		trd->data.ptr.buffer = t->buffer->user_data;
 		trd->data.ptr.offsets = trd->data.ptr.buffer +
 					ALIGN(t->buffer->data_size,
 					    sizeof(void *));
@@ -5981,7 +5980,7 @@ static void print_binder_transaction_ilocked(struct seq_file *m,
 	}
 	if (buffer->target_node)
 		seq_printf(m, " node %d", buffer->target_node->debug_id);
-	seq_printf(m, " size %zd:%zd data %pK\n",
+	seq_printf(m, " size %zd:%zd data %lx\n",
 		   buffer->data_size, buffer->offsets_size,
 		   buffer->user_data);
 }
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index a56cbfd9ba44..179b67a3ef70 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -125,23 +125,20 @@ static void binder_insert_allocated_buffer_locked(
 
 static struct binder_buffer *binder_alloc_prepare_to_free_locked(
 		struct binder_alloc *alloc,
-		uintptr_t user_ptr)
+		unsigned long user_ptr)
 {
 	struct rb_node *n = alloc->allocated_buffers.rb_node;
 	struct binder_buffer *buffer;
-	void __user *uptr;
-
-	uptr = (void __user *)user_ptr;
 
 	while (n) {
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
 		BUG_ON(buffer->free);
 
-		if (uptr < buffer->user_data)
+		if (user_ptr < buffer->user_data) {
 			n = n->rb_left;
-		else if (uptr > buffer->user_data)
+		} else if (user_ptr > buffer->user_data) {
 			n = n->rb_right;
-		else {
+		} else {
 			/*
 			 * Guard against user threads attempting to
 			 * free the buffer when in use by kernel or
@@ -168,7 +165,7 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked(
  * Return:	Pointer to buffer or NULL
  */
 struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
-						   uintptr_t user_ptr)
+						   unsigned long user_ptr)
 {
 	struct binder_buffer *buffer;
 
@@ -179,18 +176,17 @@ struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
 }
 
 static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
-				    void __user *start, void __user *end)
+				    unsigned long start, unsigned long end)
 {
-	void __user *page_addr;
-	unsigned long user_page_addr;
-	struct binder_lru_page *page;
 	struct vm_area_struct *vma = NULL;
+	struct binder_lru_page *page;
 	struct mm_struct *mm = NULL;
+	unsigned long page_addr;
 	bool need_mm = false;
 
 	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-		     "%d: %s pages %pK-%pK\n", alloc->pid,
-		     allocate ? "allocate" : "free", start, end);
+			   "%d: %s allocate pages %lx-%lx\n", alloc->pid,
+			   allocate ? "allocate" : "free", start, end);
 
 	if (end <= start)
 		return 0;
@@ -249,18 +245,17 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 					    __GFP_HIGHMEM |
 					    __GFP_ZERO);
 		if (!page->page_ptr) {
-			pr_err("%d: binder_alloc_buf failed for page at %pK\n",
-				alloc->pid, page_addr);
+			pr_err("%d: binder_alloc_buf failed for page at %lx\n",
+			       alloc->pid, page_addr);
 			goto err_alloc_page_failed;
 		}
 		page->alloc = alloc;
 		INIT_LIST_HEAD(&page->lru);
 
-		user_page_addr = (uintptr_t)page_addr;
-		ret = vm_insert_page(vma, user_page_addr, page[0].page_ptr);
+		ret = vm_insert_page(vma, page_addr, page->page_ptr);
 		if (ret) {
 			pr_err("%d: binder_alloc_buf failed to map page at %lx in userspace\n",
-			       alloc->pid, user_page_addr);
+			       alloc->pid, page_addr);
 			goto err_vm_insert_page_failed;
 		}
 
@@ -377,9 +372,9 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	struct binder_buffer *buffer;
 	size_t buffer_size;
 	struct rb_node *best_fit = NULL;
-	void __user *has_page_addr;
-	void __user *end_page_addr;
 	size_t size, data_offsets_size;
+	unsigned long has_page_addr;
+	unsigned long end_page_addr;
 	int ret;
 
 	/* Check binder_alloc is fully initialized */
@@ -477,15 +472,13 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		     "%d: binder_alloc_buf size %zd got buffer %pK size %zd\n",
 		      alloc->pid, size, buffer, buffer_size);
 
-	has_page_addr = (void __user *)
-		(((uintptr_t)buffer->user_data + buffer_size) & PAGE_MASK);
+	has_page_addr = (buffer->user_data + buffer_size) & PAGE_MASK;
 	WARN_ON(n && buffer_size != size);
-	end_page_addr =
-		(void __user *)PAGE_ALIGN((uintptr_t)buffer->user_data + size);
+	end_page_addr = PAGE_ALIGN(buffer->user_data + size);
 	if (end_page_addr > has_page_addr)
 		end_page_addr = has_page_addr;
-	ret = binder_update_page_range(alloc, 1, (void __user *)
-		PAGE_ALIGN((uintptr_t)buffer->user_data), end_page_addr);
+	ret = binder_update_page_range(alloc, 1, PAGE_ALIGN(buffer->user_data),
+				       end_page_addr);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -498,7 +491,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 			       __func__, alloc->pid);
 			goto err_alloc_buf_struct_failed;
 		}
-		new_buffer->user_data = (u8 __user *)buffer->user_data + size;
+		new_buffer->user_data = buffer->user_data + size;
 		list_add(&new_buffer->entry, &buffer->entry);
 		new_buffer->free = 1;
 		binder_insert_free_buffer(alloc, new_buffer);
@@ -536,8 +529,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	return buffer;
 
 err_alloc_buf_struct_failed:
-	binder_update_page_range(alloc, 0, (void __user *)
-				 PAGE_ALIGN((uintptr_t)buffer->user_data),
+	binder_update_page_range(alloc, 0, PAGE_ALIGN(buffer->user_data),
 				 end_page_addr);
 	return ERR_PTR(-ENOMEM);
 }
@@ -574,15 +566,14 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	return buffer;
 }
 
-static void __user *buffer_start_page(struct binder_buffer *buffer)
+static unsigned long buffer_start_page(struct binder_buffer *buffer)
 {
-	return (void __user *)((uintptr_t)buffer->user_data & PAGE_MASK);
+	return buffer->user_data & PAGE_MASK;
 }
 
-static void __user *prev_buffer_end_page(struct binder_buffer *buffer)
+static unsigned long prev_buffer_end_page(struct binder_buffer *buffer)
 {
-	return (void __user *)
-		(((uintptr_t)(buffer->user_data) - 1) & PAGE_MASK);
+	return (buffer->user_data - 1) & PAGE_MASK;
 }
 
 static void binder_delete_free_buffer(struct binder_alloc *alloc,
@@ -597,7 +588,7 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc,
 	if (prev_buffer_end_page(prev) == buffer_start_page(buffer)) {
 		to_free = false;
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				   "%d: merge free, buffer %pK share page with %pK\n",
+				   "%d: merge free, buffer %lx share page with %lx\n",
 				   alloc->pid, buffer->user_data,
 				   prev->user_data);
 	}
@@ -607,7 +598,7 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc,
 		if (buffer_start_page(next) == buffer_start_page(buffer)) {
 			to_free = false;
 			binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-					   "%d: merge free, buffer %pK share page with %pK\n",
+					   "%d: merge free, buffer %lx share page with %lx\n",
 					   alloc->pid,
 					   buffer->user_data,
 					   next->user_data);
@@ -616,17 +607,17 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc,
 
 	if (PAGE_ALIGNED(buffer->user_data)) {
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				   "%d: merge free, buffer start %pK is page aligned\n",
+				   "%d: merge free, buffer start %lx is page aligned\n",
 				   alloc->pid, buffer->user_data);
 		to_free = false;
 	}
 
 	if (to_free) {
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				   "%d: merge free, buffer %pK do not share page with %pK or %pK\n",
+				   "%d: merge free, buffer %lx do not share page with %lx or %lx\n",
 				   alloc->pid, buffer->user_data,
 				   prev->user_data,
-				   next ? next->user_data : NULL);
+				   next ? next->user_data : 0);
 		binder_update_page_range(alloc, 0, buffer_start_page(buffer),
 					 buffer_start_page(buffer) + PAGE_SIZE);
 	}
@@ -662,10 +653,8 @@ static void binder_free_buf_locked(struct binder_alloc *alloc,
 			      alloc->pid, size, alloc->free_async_space);
 	}
 
-	binder_update_page_range(alloc, 0,
-		(void __user *)PAGE_ALIGN((uintptr_t)buffer->user_data),
-		(void __user *)(((uintptr_t)
-			  buffer->user_data + buffer_size) & PAGE_MASK));
+	binder_update_page_range(alloc, 0, PAGE_ALIGN(buffer->user_data),
+				 (buffer->user_data + buffer_size) & PAGE_MASK);
 
 	rb_erase(&buffer->rb_node, &alloc->allocated_buffers);
 	buffer->free = 1;
@@ -754,7 +743,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 				   SZ_4M);
 	mutex_unlock(&binder_alloc_mmap_lock);
 
-	alloc->buffer = (void __user *)vma->vm_start;
+	alloc->buffer = vma->vm_start;
 
 	alloc->pages = kcalloc(alloc->buffer_size / PAGE_SIZE,
 			       sizeof(alloc->pages[0]),
@@ -787,7 +776,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 	kfree(alloc->pages);
 	alloc->pages = NULL;
 err_alloc_pages_failed:
-	alloc->buffer = NULL;
+	alloc->buffer = 0;
 	mutex_lock(&binder_alloc_mmap_lock);
 	alloc->buffer_size = 0;
 err_already_mapped:
@@ -840,7 +829,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 		int i;
 
 		for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
-			void __user *page_addr;
+			unsigned long page_addr;
 			bool on_lru;
 
 			if (!alloc->pages[i].page_ptr)
@@ -850,7 +839,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 					      &alloc->pages[i].lru);
 			page_addr = alloc->buffer + i * PAGE_SIZE;
 			binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				     "%s: %d: page %d at %pK %s\n",
+				     "%s: %d: page %d at %lx %s\n",
 				     __func__, alloc->pid, i, page_addr,
 				     on_lru ? "on lru" : "active");
 			__free_page(alloc->pages[i].page_ptr);
@@ -870,7 +859,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 static void print_binder_buffer(struct seq_file *m, const char *prefix,
 				struct binder_buffer *buffer)
 {
-	seq_printf(m, "%s %d: %pK size %zd:%zd:%zd %s\n",
+	seq_printf(m, "%s %d: %lx size %zd:%zd:%zd %s\n",
 		   prefix, buffer->debug_id, buffer->user_data,
 		   buffer->data_size, buffer->offsets_size,
 		   buffer->extra_buffers_size,
@@ -984,7 +973,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 						    struct binder_lru_page,
 						    lru);
 	struct binder_alloc *alloc;
-	uintptr_t page_addr;
+	unsigned long page_addr;
 	size_t index;
 	struct vm_area_struct *vma;
 
@@ -996,7 +985,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 		goto err_page_already_freed;
 
 	index = page - alloc->pages;
-	page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE;
+	page_addr = alloc->buffer + index * PAGE_SIZE;
 
 	mm = alloc->mm;
 	if (!mmget_not_zero(mm))
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 82380febdd85..cb19677a5c15 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -56,7 +56,7 @@ struct binder_buffer {
 	size_t data_size;
 	size_t offsets_size;
 	size_t extra_buffers_size;
-	void __user *user_data;
+	unsigned long user_data;
 	int    pid;
 };
 
@@ -101,7 +101,7 @@ struct binder_alloc {
 	struct mutex mutex;
 	struct vm_area_struct *vma;
 	struct mm_struct *mm;
-	void __user *buffer;
+	unsigned long buffer;
 	struct list_head buffers;
 	struct rb_root free_buffers;
 	struct rb_root allocated_buffers;
@@ -133,7 +133,7 @@ void binder_alloc_shrinker_exit(void);
 void binder_alloc_vma_close(struct binder_alloc *alloc);
 struct binder_buffer *
 binder_alloc_prepare_to_free(struct binder_alloc *alloc,
-			     uintptr_t user_ptr);
+			     unsigned long user_ptr);
 void binder_alloc_free_buf(struct binder_alloc *alloc,
 			   struct binder_buffer *buffer);
 int binder_alloc_mmap_handler(struct binder_alloc *alloc,
diff --git a/drivers/android/binder_alloc_selftest.c b/drivers/android/binder_alloc_selftest.c
index c2b323bc3b3a..341c73b4a807 100644
--- a/drivers/android/binder_alloc_selftest.c
+++ b/drivers/android/binder_alloc_selftest.c
@@ -93,11 +93,11 @@ static bool check_buffer_pages_allocated(struct binder_alloc *alloc,
 					 struct binder_buffer *buffer,
 					 size_t size)
 {
-	void __user *page_addr;
-	void __user *end;
+	unsigned long page_addr;
+	unsigned long end;
 	int page_index;
 
-	end = (void __user *)PAGE_ALIGN((uintptr_t)buffer->user_data + size);
+	end = PAGE_ALIGN(buffer->user_data + size);
 	page_addr = buffer->user_data;
 	for (; page_addr < end; page_addr += PAGE_SIZE) {
 		page_index = (page_addr - alloc->buffer) / PAGE_SIZE;
diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
index 8cc07e6a4273..fe38c6fc65d0 100644
--- a/drivers/android/binder_trace.h
+++ b/drivers/android/binder_trace.h
@@ -317,7 +317,7 @@ DEFINE_EVENT(binder_buffer_class, binder_transaction_update_buffer_release,
 
 TRACE_EVENT(binder_update_page_range,
 	TP_PROTO(struct binder_alloc *alloc, bool allocate,
-		 void __user *start, void __user *end),
+		 unsigned long start, unsigned long end),
 	TP_ARGS(alloc, allocate, start, end),
 	TP_STRUCT__entry(
 		__field(int, proc)
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 09/28] binder: keep vma addresses type as unsigned long

[Alice Ryhl]

> The vma addresses in binder are currently stored as void __user *. This
> requires casting back and forth between the mm/ api which uses unsigned
> long. Since we also do internal arithmetic on these addresses we end up
> having to cast them _again_ to an integer type.
> 
> Lets stop all the unnecessary casting which kills code readability and
> store the virtual addresses as the native unsigned long from mm/. Note
> that this approach is preferred over uintptr_t as Linus explains in [1].
> 
> Opportunistically add a few cosmetic touchups.
> 
> Link: https://lore.kernel.org/all/CAHk-=wj2OHy-5e+srG1fy+ZU00TmZ1NFp6kFLbVLMXHe7A1d-g@mail.gmail.com/ [1]
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

With the discussion in [1] and addition of patch 26, I am happy with this.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[1]: https://lore.kernel.org/all/ZWmEv6-OBEZ08vy5@google.com/

[PATCH v2 10/28] binder: split up binder_update_page_range()

[Carlos Llamas]

The binder_update_page_range() function performs both allocation and
freeing of binder pages. However, these two operations are unrelated and
have no common logic. In fact, when a free operation is requested, the
allocation logic is skipped entirely. This behavior makes the error path
unnecessarily complex. To improve readability of the code, this patch
splits the allocation and freeing operations into separate functions.

No functional changes are introduced by this patch.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 79 +++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 179b67a3ef70..3051ea7ca44f 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -175,8 +175,32 @@ struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
 	return buffer;
 }
 
-static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
-				    unsigned long start, unsigned long end)
+static void binder_free_page_range(struct binder_alloc *alloc,
+				   unsigned long start, unsigned long end)
+{
+	struct binder_lru_page *page;
+	unsigned long page_addr;
+
+	trace_binder_update_page_range(alloc, false, start, end);
+
+	for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
+		size_t index;
+		int ret;
+
+		index = (page_addr - alloc->buffer) / PAGE_SIZE;
+		page = &alloc->pages[index];
+
+		trace_binder_free_lru_start(alloc, index);
+
+		ret = list_lru_add(&binder_alloc_lru, &page->lru);
+		WARN_ON(!ret);
+
+		trace_binder_free_lru_end(alloc, index);
+	}
+}
+
+static int binder_allocate_page_range(struct binder_alloc *alloc,
+				      unsigned long start, unsigned long end)
 {
 	struct vm_area_struct *vma = NULL;
 	struct binder_lru_page *page;
@@ -185,16 +209,13 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 	bool need_mm = false;
 
 	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-			   "%d: %s allocate pages %lx-%lx\n", alloc->pid,
-			   allocate ? "allocate" : "free", start, end);
+			   "%d: allocate pages %lx-%lx\n",
+			   alloc->pid, start, end);
 
 	if (end <= start)
 		return 0;
 
-	trace_binder_update_page_range(alloc, allocate, start, end);
-
-	if (allocate == 0)
-		goto free_range;
+	trace_binder_update_page_range(alloc, true, start, end);
 
 	for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
 		page = &alloc->pages[(page_addr - alloc->buffer) / PAGE_SIZE];
@@ -270,32 +291,12 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 	}
 	return 0;
 
-free_range:
-	for (page_addr = end - PAGE_SIZE; 1; page_addr -= PAGE_SIZE) {
-		bool ret;
-		size_t index;
-
-		index = (page_addr - alloc->buffer) / PAGE_SIZE;
-		page = &alloc->pages[index];
-
-		trace_binder_free_lru_start(alloc, index);
-
-		ret = list_lru_add(&binder_alloc_lru, &page->lru);
-		WARN_ON(!ret);
-
-		trace_binder_free_lru_end(alloc, index);
-		if (page_addr == start)
-			break;
-		continue;
-
 err_vm_insert_page_failed:
-		__free_page(page->page_ptr);
-		page->page_ptr = NULL;
+	__free_page(page->page_ptr);
+	page->page_ptr = NULL;
 err_alloc_page_failed:
 err_page_ptr_cleared:
-		if (page_addr == start)
-			break;
-	}
+	binder_free_page_range(alloc, start, page_addr);
 err_no_vma:
 	if (mm) {
 		mmap_write_unlock(mm);
@@ -477,8 +478,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	end_page_addr = PAGE_ALIGN(buffer->user_data + size);
 	if (end_page_addr > has_page_addr)
 		end_page_addr = has_page_addr;
-	ret = binder_update_page_range(alloc, 1, PAGE_ALIGN(buffer->user_data),
-				       end_page_addr);
+	ret = binder_allocate_page_range(alloc, PAGE_ALIGN(buffer->user_data),
+					 end_page_addr);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -529,8 +530,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	return buffer;
 
 err_alloc_buf_struct_failed:
-	binder_update_page_range(alloc, 0, PAGE_ALIGN(buffer->user_data),
-				 end_page_addr);
+	binder_free_page_range(alloc, PAGE_ALIGN(buffer->user_data),
+			       end_page_addr);
 	return ERR_PTR(-ENOMEM);
 }
 
@@ -618,8 +619,8 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc,
 				   alloc->pid, buffer->user_data,
 				   prev->user_data,
 				   next ? next->user_data : 0);
-		binder_update_page_range(alloc, 0, buffer_start_page(buffer),
-					 buffer_start_page(buffer) + PAGE_SIZE);
+		binder_free_page_range(alloc, buffer_start_page(buffer),
+				       buffer_start_page(buffer) + PAGE_SIZE);
 	}
 	list_del(&buffer->entry);
 	kfree(buffer);
@@ -653,8 +654,8 @@ static void binder_free_buf_locked(struct binder_alloc *alloc,
 			      alloc->pid, size, alloc->free_async_space);
 	}
 
-	binder_update_page_range(alloc, 0, PAGE_ALIGN(buffer->user_data),
-				 (buffer->user_data + buffer_size) & PAGE_MASK);
+	binder_free_page_range(alloc, PAGE_ALIGN(buffer->user_data),
+			       (buffer->user_data + buffer_size) & PAGE_MASK);
 
 	rb_erase(&buffer->rb_node, &alloc->allocated_buffers);
 	buffer->free = 1;
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 11/28] binder: do unlocked work in binder_alloc_new_buf()

[Carlos Llamas]

Extract non-critical sections from binder_alloc_new_buf_locked() that
don't require holding the alloc->mutex. While we are here, consolidate
the checks for size overflow and zero-sized padding into a separate
sanitized_size() helper function.

Also add a few touchups to follow the coding guidelines.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 95 ++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 39 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 3051ea7ca44f..40a2ca0c0dea 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -363,9 +363,7 @@ static bool debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
 
 static struct binder_buffer *binder_alloc_new_buf_locked(
 				struct binder_alloc *alloc,
-				size_t data_size,
-				size_t offsets_size,
-				size_t extra_buffers_size,
+				size_t size,
 				int is_async,
 				int pid)
 {
@@ -373,39 +371,10 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	struct binder_buffer *buffer;
 	size_t buffer_size;
 	struct rb_node *best_fit = NULL;
-	size_t size, data_offsets_size;
 	unsigned long has_page_addr;
 	unsigned long end_page_addr;
 	int ret;
 
-	/* Check binder_alloc is fully initialized */
-	if (!binder_alloc_get_vma(alloc)) {
-		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
-				   "%d: binder_alloc_buf, no vma\n",
-				   alloc->pid);
-		return ERR_PTR(-ESRCH);
-	}
-
-	data_offsets_size = ALIGN(data_size, sizeof(void *)) +
-		ALIGN(offsets_size, sizeof(void *));
-
-	if (data_offsets_size < data_size || data_offsets_size < offsets_size) {
-		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				"%d: got transaction with invalid size %zd-%zd\n",
-				alloc->pid, data_size, offsets_size);
-		return ERR_PTR(-EINVAL);
-	}
-	size = data_offsets_size + ALIGN(extra_buffers_size, sizeof(void *));
-	if (size < data_offsets_size || size < extra_buffers_size) {
-		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				"%d: got transaction with invalid extra_buffers_size %zd\n",
-				alloc->pid, extra_buffers_size);
-		return ERR_PTR(-EINVAL);
-	}
-
-	/* Pad 0-size buffers so they get assigned unique addresses */
-	size = max(size, sizeof(void *));
-
 	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",
@@ -421,13 +390,14 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		if (size < buffer_size) {
 			best_fit = n;
 			n = n->rb_left;
-		} else if (size > buffer_size)
+		} else if (size > buffer_size) {
 			n = n->rb_right;
-		else {
+		} else {
 			best_fit = n;
 			break;
 		}
 	}
+
 	if (best_fit == NULL) {
 		size_t allocated_buffers = 0;
 		size_t largest_alloc_size = 0;
@@ -505,10 +475,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
 		     "%d: binder_alloc_buf size %zd got %pK\n",
 		      alloc->pid, size, buffer);
-	buffer->data_size = data_size;
-	buffer->offsets_size = offsets_size;
 	buffer->async_transaction = is_async;
-	buffer->extra_buffers_size = extra_buffers_size;
 	buffer->pid = pid;
 	buffer->oneway_spam_suspect = false;
 	if (is_async) {
@@ -527,6 +494,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 			alloc->oneway_spam_detected = false;
 		}
 	}
+
 	return buffer;
 
 err_alloc_buf_struct_failed:
@@ -535,6 +503,28 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	return ERR_PTR(-ENOMEM);
 }
 
+/* Calculate the sanitized total size, returns 0 for invalid request */
+static inline size_t sanitized_size(size_t data_size,
+				    size_t offsets_size,
+				    size_t extra_buffers_size)
+{
+	size_t total, tmp;
+
+	/* Align to pointer size and check for overflows */
+	tmp = ALIGN(data_size, sizeof(void *)) +
+		ALIGN(offsets_size, sizeof(void *));
+	if (tmp < data_size || tmp < offsets_size)
+		return 0;
+	total = tmp + ALIGN(extra_buffers_size, sizeof(void *));
+	if (total < tmp || total < extra_buffers_size)
+		return 0;
+
+	/* Pad 0-sized buffers so they get a unique address */
+	total = max(total, sizeof(void *));
+
+	return total;
+}
+
 /**
  * binder_alloc_new_buf() - Allocate a new binder buffer
  * @alloc:              binder_alloc for this proc
@@ -559,11 +549,38 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 					   int pid)
 {
 	struct binder_buffer *buffer;
+	size_t size;
+
+	/* Check binder_alloc is fully initialized */
+	if (!binder_alloc_get_vma(alloc)) {
+		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+				   "%d: binder_alloc_buf, no vma\n",
+				   alloc->pid);
+		return ERR_PTR(-ESRCH);
+	}
+
+	size = sanitized_size(data_size, offsets_size, extra_buffers_size);
+	if (unlikely(!size)) {
+		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
+				   "%d: got transaction with invalid size %zd-%zd-%zd\n",
+				   alloc->pid, data_size, offsets_size,
+				   extra_buffers_size);
+		return ERR_PTR(-EINVAL);
+	}
 
 	mutex_lock(&alloc->mutex);
-	buffer = binder_alloc_new_buf_locked(alloc, data_size, offsets_size,
-					     extra_buffers_size, is_async, pid);
+	buffer = binder_alloc_new_buf_locked(alloc, size, is_async, pid);
+	if (IS_ERR(buffer)) {
+		mutex_unlock(&alloc->mutex);
+		goto out;
+	}
+
+	buffer->data_size = data_size;
+	buffer->offsets_size = offsets_size;
+	buffer->extra_buffers_size = extra_buffers_size;
 	mutex_unlock(&alloc->mutex);
+
+out:
 	return buffer;
 }
 
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 11/28] binder: do unlocked work in binder_alloc_new_buf()

[Alice Ryhl]

> Extract non-critical sections from binder_alloc_new_buf_locked() that
> don't require holding the alloc->mutex. While we are here, consolidate
> the checks for size overflow and zero-sized padding into a separate
> sanitized_size() helper function.
> 
> Also add a few touchups to follow the coding guidelines.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +	if (IS_ERR(buffer)) {
> +		mutex_unlock(&alloc->mutex);
> +		goto out;
> +	}
> +
> +	buffer->data_size = data_size;
> +	buffer->offsets_size = offsets_size;
> +	buffer->extra_buffers_size = extra_buffers_size;
>  	mutex_unlock(&alloc->mutex);
> +
> +out:
>  	return buffer;
>  }

You could also write this as:

	if (!IS_ERR(buffer)) {
		buffer->data_size = data_size;
		buffer->offsets_size = offsets_size;
		buffer->extra_buffers_size = extra_buffers_size;
	}

	mutex_unlock(&alloc->mutex);
	return buffer;

But I'm happy either way.

Alice

Re: [PATCH v2 11/28] binder: do unlocked work in binder_alloc_new_buf()

[Carlos Llamas]

On Mon, Dec 04, 2023 at 11:57:04AM +0000, 'Alice Ryhl' via kernel-team wrote:
> > Extract non-critical sections from binder_alloc_new_buf_locked() that
> > don't require holding the alloc->mutex. While we are here, consolidate
> > the checks for size overflow and zero-sized padding into a separate
> > sanitized_size() helper function.
> > 
> > Also add a few touchups to follow the coding guidelines.
> > 
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
> > +	if (IS_ERR(buffer)) {
> > +		mutex_unlock(&alloc->mutex);
> > +		goto out;
> > +	}
> > +
> > +	buffer->data_size = data_size;
> > +	buffer->offsets_size = offsets_size;
> > +	buffer->extra_buffers_size = extra_buffers_size;
> >  	mutex_unlock(&alloc->mutex);
> > +
> > +out:
> >  	return buffer;
> >  }
> 
> You could also write this as:
> 
> 	if (!IS_ERR(buffer)) {
> 		buffer->data_size = data_size;
> 		buffer->offsets_size = offsets_size;
> 		buffer->extra_buffers_size = extra_buffers_size;
> 	}
> 
> 	mutex_unlock(&alloc->mutex);
> 	return buffer;

There is a subsequent patch that adds more work after this and makes the
goto statement a better fit (patch 19/28)... at least IMO.

--
Carlos Llamas

Re: [PATCH v2 11/28] binder: do unlocked work in binder_alloc_new_buf()

[Alice Ryhl]

On Mon, Dec 4, 2023 at 3:22 PM Carlos Llamas <cmllamas@google.com> wrote:
> On Mon, Dec 04, 2023 at 11:57:04AM +0000, 'Alice Ryhl' via kernel-team wrote:
> > You could also write this as:
> >
> >       if (!IS_ERR(buffer)) {
> >               buffer->data_size = data_size;
> >               buffer->offsets_size = offsets_size;
> >               buffer->extra_buffers_size = extra_buffers_size;
> >       }
> >
> >       mutex_unlock(&alloc->mutex);
> >       return buffer;
>
> There is a subsequent patch that adds more work after this and makes the
> goto statement a better fit (patch 19/28)... at least IMO.

Ah, ok. SGTM!

Alice

[PATCH v2 12/28] binder: remove pid param in binder_alloc_new_buf()

[Carlos Llamas]

Binder attributes the buffer allocation to the current->tgid everytime.
There is no need to pass this as a parameter so drop it.

Also add a few touchups to follow the coding guidelines. No functional
changes are introduced in this patch.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c                |  2 +-
 drivers/android/binder_alloc.c          | 18 ++++++++----------
 drivers/android/binder_alloc.h          |  7 ++-----
 drivers/android/binder_alloc_selftest.c |  2 +-
 4 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 437d1097b118..45674af6310f 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3225,7 +3225,7 @@ static void binder_transaction(struct binder_proc *proc,
 
 	t->buffer = binder_alloc_new_buf(&target_proc->alloc, tr->data_size,
 		tr->offsets_size, extra_buffers_size,
-		!reply && (t->flags & TF_ONE_WAY), current->tgid);
+		!reply && (t->flags & TF_ONE_WAY));
 	if (IS_ERR(t->buffer)) {
 		char *s;
 
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 40a2ca0c0dea..b5c3e56318e1 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -319,7 +319,7 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
 	return smp_load_acquire(&alloc->vma);
 }
 
-static bool debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
+static bool debug_low_async_space_locked(struct binder_alloc *alloc)
 {
 	/*
 	 * Find the amount and size of buffers allocated by the current caller;
@@ -328,10 +328,11 @@ static bool debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
 	 * and at some point we'll catch them in the act. This is more efficient
 	 * than keeping a map per pid.
 	 */
-	struct rb_node *n;
 	struct binder_buffer *buffer;
 	size_t total_alloc_size = 0;
+	int pid = current->tgid;
 	size_t num_buffers = 0;
+	struct rb_node *n;
 
 	for (n = rb_first(&alloc->allocated_buffers); n != NULL;
 		 n = rb_next(n)) {
@@ -364,8 +365,7 @@ static bool debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
 static struct binder_buffer *binder_alloc_new_buf_locked(
 				struct binder_alloc *alloc,
 				size_t size,
-				int is_async,
-				int pid)
+				int is_async)
 {
 	struct rb_node *n = alloc->free_buffers.rb_node;
 	struct binder_buffer *buffer;
@@ -476,7 +476,6 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		     "%d: binder_alloc_buf size %zd got %pK\n",
 		      alloc->pid, size, buffer);
 	buffer->async_transaction = is_async;
-	buffer->pid = pid;
 	buffer->oneway_spam_suspect = false;
 	if (is_async) {
 		alloc->free_async_space -= size;
@@ -489,7 +488,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 			 * of async space left (which is less than 10% of total
 			 * buffer size).
 			 */
-			buffer->oneway_spam_suspect = debug_low_async_space_locked(alloc, pid);
+			buffer->oneway_spam_suspect = debug_low_async_space_locked(alloc);
 		} else {
 			alloc->oneway_spam_detected = false;
 		}
@@ -532,7 +531,6 @@ static inline size_t sanitized_size(size_t data_size,
  * @offsets_size:       user specified buffer offset
  * @extra_buffers_size: size of extra space for meta-data (eg, security context)
  * @is_async:           buffer for async transaction
- * @pid:				pid to attribute allocation to (used for debugging)
  *
  * Allocate a new buffer given the requested sizes. Returns
  * the kernel version of the buffer pointer. The size allocated
@@ -545,8 +543,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 					   size_t data_size,
 					   size_t offsets_size,
 					   size_t extra_buffers_size,
-					   int is_async,
-					   int pid)
+					   int is_async)
 {
 	struct binder_buffer *buffer;
 	size_t size;
@@ -569,7 +566,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	}
 
 	mutex_lock(&alloc->mutex);
-	buffer = binder_alloc_new_buf_locked(alloc, size, is_async, pid);
+	buffer = binder_alloc_new_buf_locked(alloc, size, is_async);
 	if (IS_ERR(buffer)) {
 		mutex_unlock(&alloc->mutex);
 		goto out;
@@ -578,6 +575,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	buffer->data_size = data_size;
 	buffer->offsets_size = offsets_size;
 	buffer->extra_buffers_size = extra_buffers_size;
+	buffer->pid = current->tgid;
 	mutex_unlock(&alloc->mutex);
 
 out:
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index cb19677a5c15..bbc16bc6d5ac 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -49,15 +49,13 @@ struct binder_buffer {
 	unsigned async_transaction:1;
 	unsigned oneway_spam_suspect:1;
 	unsigned debug_id:27;
-
 	struct binder_transaction *transaction;
-
 	struct binder_node *target_node;
 	size_t data_size;
 	size_t offsets_size;
 	size_t extra_buffers_size;
 	unsigned long user_data;
-	int    pid;
+	int pid;
 };
 
 /**
@@ -125,8 +123,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 					   size_t data_size,
 					   size_t offsets_size,
 					   size_t extra_buffers_size,
-					   int is_async,
-					   int pid);
+					   int is_async);
 void binder_alloc_init(struct binder_alloc *alloc);
 int binder_alloc_shrinker_init(void);
 void binder_alloc_shrinker_exit(void);
diff --git a/drivers/android/binder_alloc_selftest.c b/drivers/android/binder_alloc_selftest.c
index 341c73b4a807..ed753747e54c 100644
--- a/drivers/android/binder_alloc_selftest.c
+++ b/drivers/android/binder_alloc_selftest.c
@@ -119,7 +119,7 @@ static void binder_selftest_alloc_buf(struct binder_alloc *alloc,
 	int i;
 
 	for (i = 0; i < BUFFER_NUM; i++) {
-		buffers[i] = binder_alloc_new_buf(alloc, sizes[i], 0, 0, 0, 0);
+		buffers[i] = binder_alloc_new_buf(alloc, sizes[i], 0, 0, 0);
 		if (IS_ERR(buffers[i]) ||
 		    !check_buffer_pages_allocated(alloc, buffers[i],
 						  sizes[i])) {
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 13/28] binder: separate the no-space debugging logic

[Carlos Llamas]

Move the no-space debugging logic into a separate function. Lets also
mark this branch as unlikely in binder_alloc_new_buf_locked() as most
requests will fit without issue.

Also add a few cosmetic changes and suggestions from checkpatch.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 71 +++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 31 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index b5c3e56318e1..3dca7b199246 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -319,6 +319,43 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
 	return smp_load_acquire(&alloc->vma);
 }
 
+static void debug_no_space_locked(struct binder_alloc *alloc)
+{
+	size_t largest_alloc_size = 0;
+	struct binder_buffer *buffer;
+	size_t allocated_buffers = 0;
+	size_t largest_free_size = 0;
+	size_t total_alloc_size = 0;
+	size_t total_free_size = 0;
+	size_t free_buffers = 0;
+	size_t buffer_size;
+	struct rb_node *n;
+
+	for (n = rb_first(&alloc->allocated_buffers); n; n = rb_next(n)) {
+		buffer = rb_entry(n, struct binder_buffer, rb_node);
+		buffer_size = binder_alloc_buffer_size(alloc, buffer);
+		allocated_buffers++;
+		total_alloc_size += buffer_size;
+		if (buffer_size > largest_alloc_size)
+			largest_alloc_size = buffer_size;
+	}
+
+	for (n = rb_first(&alloc->free_buffers); n; n = rb_next(n)) {
+		buffer = rb_entry(n, struct binder_buffer, rb_node);
+		buffer_size = binder_alloc_buffer_size(alloc, buffer);
+		free_buffers++;
+		total_free_size += buffer_size;
+		if (buffer_size > largest_free_size)
+			largest_free_size = buffer_size;
+	}
+
+	binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+			   "allocated: %zd (num: %zd largest: %zd), free: %zd (num: %zd largest: %zd)\n",
+			   total_alloc_size, allocated_buffers,
+			   largest_alloc_size, total_free_size,
+			   free_buffers, largest_free_size);
+}
+
 static bool debug_low_async_space_locked(struct binder_alloc *alloc)
 {
 	/*
@@ -398,42 +435,14 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		}
 	}
 
-	if (best_fit == NULL) {
-		size_t allocated_buffers = 0;
-		size_t largest_alloc_size = 0;
-		size_t total_alloc_size = 0;
-		size_t free_buffers = 0;
-		size_t largest_free_size = 0;
-		size_t total_free_size = 0;
-
-		for (n = rb_first(&alloc->allocated_buffers); n != NULL;
-		     n = rb_next(n)) {
-			buffer = rb_entry(n, struct binder_buffer, rb_node);
-			buffer_size = binder_alloc_buffer_size(alloc, buffer);
-			allocated_buffers++;
-			total_alloc_size += buffer_size;
-			if (buffer_size > largest_alloc_size)
-				largest_alloc_size = buffer_size;
-		}
-		for (n = rb_first(&alloc->free_buffers); n != NULL;
-		     n = rb_next(n)) {
-			buffer = rb_entry(n, struct binder_buffer, rb_node);
-			buffer_size = binder_alloc_buffer_size(alloc, buffer);
-			free_buffers++;
-			total_free_size += buffer_size;
-			if (buffer_size > largest_free_size)
-				largest_free_size = buffer_size;
-		}
+	if (unlikely(!best_fit)) {
 		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
 				   "%d: binder_alloc_buf size %zd failed, no address space\n",
 				   alloc->pid, size);
-		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
-				   "allocated: %zd (num: %zd largest: %zd), free: %zd (num: %zd largest: %zd)\n",
-				   total_alloc_size, allocated_buffers,
-				   largest_alloc_size, total_free_size,
-				   free_buffers, largest_free_size);
+		debug_no_space_locked(alloc);
 		return ERR_PTR(-ENOSPC);
 	}
+
 	if (n == NULL) {
 		buffer = rb_entry(best_fit, struct binder_buffer, rb_node);
 		buffer_size = binder_alloc_buffer_size(alloc, buffer);
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 14/28] binder: relocate low space calculation

[Carlos Llamas]

Move the low async space calculation to debug_low_async_space_locked().
This logic not only fits better here but also offloads some of the many
tasks currently done in binder_alloc_new_buf_locked().

No functional change in this patch.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 3dca7b199246..167ee6f871dc 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -371,6 +371,15 @@ static bool debug_low_async_space_locked(struct binder_alloc *alloc)
 	size_t num_buffers = 0;
 	struct rb_node *n;
 
+	/*
+	 * Only start detecting spammers once we have less than 20% of async
+	 * space left (which is less than 10% of total buffer size).
+	 */
+	if (alloc->free_async_space >= alloc->buffer_size / 10) {
+		alloc->oneway_spam_detected = false;
+		return false;
+	}
+
 	for (n = rb_first(&alloc->allocated_buffers); n != NULL;
 		 n = rb_next(n)) {
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
@@ -491,16 +500,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		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);
-		if (alloc->free_async_space < alloc->buffer_size / 10) {
-			/*
-			 * Start detecting spammers once we have less than 20%
-			 * of async space left (which is less than 10% of total
-			 * buffer size).
-			 */
-			buffer->oneway_spam_suspect = debug_low_async_space_locked(alloc);
-		} else {
-			alloc->oneway_spam_detected = false;
-		}
+		if (debug_low_async_space_locked(alloc))
+			buffer->oneway_spam_suspect = true;
 	}
 
 	return buffer;
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 15/28] binder: relocate binder_alloc_clear_buf()

[Carlos Llamas]

Move this function up along with binder_alloc_get_page() so that their
prototypes aren't necessary.

No functional change in this patch.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 124 ++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 63 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 167ee6f871dc..99eacd8782b8 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -704,8 +704,68 @@ static void binder_free_buf_locked(struct binder_alloc *alloc,
 	binder_insert_free_buffer(alloc, buffer);
 }
 
+/**
+ * binder_alloc_get_page() - get kernel pointer for given buffer offset
+ * @alloc: binder_alloc for this proc
+ * @buffer: binder buffer to be accessed
+ * @buffer_offset: offset into @buffer data
+ * @pgoffp: address to copy final page offset to
+ *
+ * Lookup the struct page corresponding to the address
+ * at @buffer_offset into @buffer->user_data. If @pgoffp is not
+ * NULL, the byte-offset into the page is written there.
+ *
+ * The caller is responsible to ensure that the offset points
+ * to a valid address within the @buffer and that @buffer is
+ * not freeable by the user. Since it can't be freed, we are
+ * guaranteed that the corresponding elements of @alloc->pages[]
+ * cannot change.
+ *
+ * Return: struct page
+ */
+static struct page *binder_alloc_get_page(struct binder_alloc *alloc,
+					  struct binder_buffer *buffer,
+					  binder_size_t buffer_offset,
+					  pgoff_t *pgoffp)
+{
+	binder_size_t buffer_space_offset = buffer_offset +
+		(buffer->user_data - alloc->buffer);
+	pgoff_t pgoff = buffer_space_offset & ~PAGE_MASK;
+	size_t index = buffer_space_offset >> PAGE_SHIFT;
+	struct binder_lru_page *lru_page;
+
+	lru_page = &alloc->pages[index];
+	*pgoffp = pgoff;
+	return lru_page->page_ptr;
+}
+
+/**
+ * binder_alloc_clear_buf() - zero out buffer
+ * @alloc: binder_alloc for this proc
+ * @buffer: binder buffer to be cleared
+ *
+ * memset the given buffer to 0
+ */
 static void binder_alloc_clear_buf(struct binder_alloc *alloc,
-				   struct binder_buffer *buffer);
+				   struct binder_buffer *buffer)
+{
+	size_t bytes = binder_alloc_buffer_size(alloc, buffer);
+	binder_size_t buffer_offset = 0;
+
+	while (bytes) {
+		unsigned long size;
+		struct page *page;
+		pgoff_t pgoff;
+
+		page = binder_alloc_get_page(alloc, buffer,
+					     buffer_offset, &pgoff);
+		size = min_t(size_t, bytes, PAGE_SIZE - pgoff);
+		memset_page(page, pgoff, 0, size);
+		bytes -= size;
+		buffer_offset += size;
+	}
+}
+
 /**
  * binder_alloc_free_buf() - free a binder buffer
  * @alloc:	binder_alloc for this proc
@@ -1148,68 +1208,6 @@ static inline bool check_buffer(struct binder_alloc *alloc,
 		(!buffer->allow_user_free || !buffer->transaction);
 }
 
-/**
- * binder_alloc_get_page() - get kernel pointer for given buffer offset
- * @alloc: binder_alloc for this proc
- * @buffer: binder buffer to be accessed
- * @buffer_offset: offset into @buffer data
- * @pgoffp: address to copy final page offset to
- *
- * Lookup the struct page corresponding to the address
- * at @buffer_offset into @buffer->user_data. If @pgoffp is not
- * NULL, the byte-offset into the page is written there.
- *
- * The caller is responsible to ensure that the offset points
- * to a valid address within the @buffer and that @buffer is
- * not freeable by the user. Since it can't be freed, we are
- * guaranteed that the corresponding elements of @alloc->pages[]
- * cannot change.
- *
- * Return: struct page
- */
-static struct page *binder_alloc_get_page(struct binder_alloc *alloc,
-					  struct binder_buffer *buffer,
-					  binder_size_t buffer_offset,
-					  pgoff_t *pgoffp)
-{
-	binder_size_t buffer_space_offset = buffer_offset +
-		(buffer->user_data - alloc->buffer);
-	pgoff_t pgoff = buffer_space_offset & ~PAGE_MASK;
-	size_t index = buffer_space_offset >> PAGE_SHIFT;
-	struct binder_lru_page *lru_page;
-
-	lru_page = &alloc->pages[index];
-	*pgoffp = pgoff;
-	return lru_page->page_ptr;
-}
-
-/**
- * binder_alloc_clear_buf() - zero out buffer
- * @alloc: binder_alloc for this proc
- * @buffer: binder buffer to be cleared
- *
- * memset the given buffer to 0
- */
-static void binder_alloc_clear_buf(struct binder_alloc *alloc,
-				   struct binder_buffer *buffer)
-{
-	size_t bytes = binder_alloc_buffer_size(alloc, buffer);
-	binder_size_t buffer_offset = 0;
-
-	while (bytes) {
-		unsigned long size;
-		struct page *page;
-		pgoff_t pgoff;
-
-		page = binder_alloc_get_page(alloc, buffer,
-					     buffer_offset, &pgoff);
-		size = min_t(size_t, bytes, PAGE_SIZE - pgoff);
-		memset_page(page, pgoff, 0, size);
-		bytes -= size;
-		buffer_offset += size;
-	}
-}
-
 /**
  * binder_alloc_copy_user_to_buffer() - copy src user to tgt user
  * @alloc: binder_alloc for this proc
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 16/28] binder: refactor page range allocation

[Carlos Llamas]

Instead of looping through the page range twice to first determine if
the mmap lock is required, simply do it per-page as needed. Split out
all this logic into a separate binder_install_single_page() function.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 107 +++++++++++++++------------------
 1 file changed, 47 insertions(+), 60 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 99eacd8782b8..1caf0e3d3451 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -199,14 +199,51 @@ static void binder_free_page_range(struct binder_alloc *alloc,
 	}
 }
 
+static int binder_install_single_page(struct binder_alloc *alloc,
+				      struct binder_lru_page *lru_page,
+				      unsigned long addr)
+{
+	struct page *page;
+	int ret = 0;
+
+	if (!mmget_not_zero(alloc->mm))
+		return -ESRCH;
+
+	mmap_write_lock(alloc->mm);
+	if (!alloc->vma) {
+		pr_err("%d: %s failed, no vma\n", alloc->pid, __func__);
+		ret = -ESRCH;
+		goto out;
+	}
+
+	page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO);
+	if (!page) {
+		pr_err("%d: failed to allocate page\n", alloc->pid);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = vm_insert_page(alloc->vma, addr, page);
+	if (ret) {
+		pr_err("%d: %s failed to insert page at %lx with %d\n",
+		       alloc->pid, __func__, addr, ret);
+		__free_page(page);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	lru_page->page_ptr = page;
+out:
+	mmap_write_unlock(alloc->mm);
+	mmput_async(alloc->mm);
+	return ret;
+}
+
 static int binder_allocate_page_range(struct binder_alloc *alloc,
 				      unsigned long start, unsigned long end)
 {
-	struct vm_area_struct *vma = NULL;
 	struct binder_lru_page *page;
-	struct mm_struct *mm = NULL;
 	unsigned long page_addr;
-	bool need_mm = false;
 
 	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
 			   "%d: allocate pages %lx-%lx\n",
@@ -218,32 +255,9 @@ static int binder_allocate_page_range(struct binder_alloc *alloc,
 	trace_binder_update_page_range(alloc, true, start, end);
 
 	for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
-		page = &alloc->pages[(page_addr - alloc->buffer) / PAGE_SIZE];
-		if (!page->page_ptr) {
-			need_mm = true;
-			break;
-		}
-	}
-
-	if (need_mm && mmget_not_zero(alloc->mm))
-		mm = alloc->mm;
-
-	if (mm) {
-		mmap_write_lock(mm);
-		vma = alloc->vma;
-	}
-
-	if (!vma && need_mm) {
-		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
-				   "%d: binder_alloc_buf failed to map pages in userspace, no vma\n",
-				   alloc->pid);
-		goto err_no_vma;
-	}
-
-	for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
-		int ret;
+		unsigned long index;
 		bool on_lru;
-		size_t index;
+		int ret;
 
 		index = (page_addr - alloc->buffer) / PAGE_SIZE;
 		page = &alloc->pages[index];
@@ -258,26 +272,15 @@ static int binder_allocate_page_range(struct binder_alloc *alloc,
 			continue;
 		}
 
-		if (WARN_ON(!vma))
-			goto err_page_ptr_cleared;
-
 		trace_binder_alloc_page_start(alloc, index);
-		page->page_ptr = alloc_page(GFP_KERNEL |
-					    __GFP_HIGHMEM |
-					    __GFP_ZERO);
-		if (!page->page_ptr) {
-			pr_err("%d: binder_alloc_buf failed for page at %lx\n",
-			       alloc->pid, page_addr);
-			goto err_alloc_page_failed;
-		}
+
 		page->alloc = alloc;
 		INIT_LIST_HEAD(&page->lru);
 
-		ret = vm_insert_page(vma, page_addr, page->page_ptr);
+		ret = binder_install_single_page(alloc, page, page_addr);
 		if (ret) {
-			pr_err("%d: binder_alloc_buf failed to map page at %lx in userspace\n",
-			       alloc->pid, page_addr);
-			goto err_vm_insert_page_failed;
+			binder_free_page_range(alloc, start, page_addr);
+			return ret;
 		}
 
 		if (index + 1 > alloc->pages_high)
@@ -285,24 +288,8 @@ static int binder_allocate_page_range(struct binder_alloc *alloc,
 
 		trace_binder_alloc_page_end(alloc, index);
 	}
-	if (mm) {
-		mmap_write_unlock(mm);
-		mmput_async(mm);
-	}
-	return 0;
 
-err_vm_insert_page_failed:
-	__free_page(page->page_ptr);
-	page->page_ptr = NULL;
-err_alloc_page_failed:
-err_page_ptr_cleared:
-	binder_free_page_range(alloc, start, page_addr);
-err_no_vma:
-	if (mm) {
-		mmap_write_unlock(mm);
-		mmput_async(mm);
-	}
-	return vma ? -ENOMEM : -ESRCH;
+	return 0;
 }
 
 static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 17/28] binder: malloc new_buffer outside of locks

[Carlos Llamas]

Preallocate new_buffer before acquiring the alloc->mutex and hand it
down to binder_alloc_new_buf_locked(). The new buffer will be used in
the vast majority of requests (measured at 98.2% in field data). The
buffer is discarded otherwise. This change is required in preparation
for transitioning alloc->mutex into a spinlock in subsequent commits.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 44 ++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 1caf0e3d3451..86f4929a55d5 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -395,24 +395,27 @@ static bool debug_low_async_space_locked(struct binder_alloc *alloc)
 	return false;
 }
 
+/* Callers preallocate @new_buffer, it is freed by this function if unused */
 static struct binder_buffer *binder_alloc_new_buf_locked(
 				struct binder_alloc *alloc,
+				struct binder_buffer *new_buffer,
 				size_t size,
 				int is_async)
 {
 	struct rb_node *n = alloc->free_buffers.rb_node;
-	struct binder_buffer *buffer;
-	size_t buffer_size;
 	struct rb_node *best_fit = NULL;
+	struct binder_buffer *buffer;
 	unsigned long has_page_addr;
 	unsigned long end_page_addr;
+	size_t buffer_size;
 	int ret;
 
 	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);
-		return ERR_PTR(-ENOSPC);
+		buffer = ERR_PTR(-ENOSPC);
+		goto out;
 	}
 
 	while (n) {
@@ -436,7 +439,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 				   "%d: binder_alloc_buf size %zd failed, no address space\n",
 				   alloc->pid, size);
 		debug_no_space_locked(alloc);
-		return ERR_PTR(-ENOSPC);
+		buffer = ERR_PTR(-ENOSPC);
+		goto out;
 	}
 
 	if (n == NULL) {
@@ -455,22 +459,17 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		end_page_addr = has_page_addr;
 	ret = binder_allocate_page_range(alloc, PAGE_ALIGN(buffer->user_data),
 					 end_page_addr);
-	if (ret)
-		return ERR_PTR(ret);
+	if (ret) {
+		buffer = ERR_PTR(ret);
+		goto out;
+	}
 
 	if (buffer_size != size) {
-		struct binder_buffer *new_buffer;
-
-		new_buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
-		if (!new_buffer) {
-			pr_err("%s: %d failed to alloc new buffer struct\n",
-			       __func__, alloc->pid);
-			goto err_alloc_buf_struct_failed;
-		}
 		new_buffer->user_data = buffer->user_data + size;
 		list_add(&new_buffer->entry, &buffer->entry);
 		new_buffer->free = 1;
 		binder_insert_free_buffer(alloc, new_buffer);
+		new_buffer = NULL;
 	}
 
 	rb_erase(best_fit, &alloc->free_buffers);
@@ -491,12 +490,10 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 			buffer->oneway_spam_suspect = true;
 	}
 
+out:
+	/* Discard possibly unused new_buffer */
+	kfree(new_buffer);
 	return buffer;
-
-err_alloc_buf_struct_failed:
-	binder_free_page_range(alloc, PAGE_ALIGN(buffer->user_data),
-			       end_page_addr);
-	return ERR_PTR(-ENOMEM);
 }
 
 /* Calculate the sanitized total size, returns 0 for invalid request */
@@ -542,7 +539,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 					   size_t extra_buffers_size,
 					   int is_async)
 {
-	struct binder_buffer *buffer;
+	struct binder_buffer *buffer, *next;
 	size_t size;
 
 	/* Check binder_alloc is fully initialized */
@@ -562,8 +559,13 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 		return ERR_PTR(-EINVAL);
 	}
 
+	/* Preallocate the next buffer */
+	next = kzalloc(sizeof(*next), GFP_KERNEL);
+	if (!next)
+		return ERR_PTR(-ENOMEM);
+
 	mutex_lock(&alloc->mutex);
-	buffer = binder_alloc_new_buf_locked(alloc, size, is_async);
+	buffer = binder_alloc_new_buf_locked(alloc, next, size, is_async);
 	if (IS_ERR(buffer)) {
 		mutex_unlock(&alloc->mutex);
 		goto out;
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 17/28] binder: malloc new_buffer outside of locks

[Alice Ryhl]

> Preallocate new_buffer before acquiring the alloc->mutex and hand it
> down to binder_alloc_new_buf_locked(). The new buffer will be used in
> the vast majority of requests (measured at 98.2% in field data). The
> buffer is discarded otherwise. This change is required in preparation
> for transitioning alloc->mutex into a spinlock in subsequent commits.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v2 18/28] binder: initialize lru pages in mmap callback

[Carlos Llamas]

Rather than repeatedly initializing some of the binder_lru_page members
during binder_alloc_new_buf(), perform this initialization just once in
binder_alloc_mmap_handler(), after the pages have been created.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 86f4929a55d5..25efdbb2ad5d 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -274,9 +274,6 @@ static int binder_allocate_page_range(struct binder_alloc *alloc,
 
 		trace_binder_alloc_page_start(alloc, index);
 
-		page->alloc = alloc;
-		INIT_LIST_HEAD(&page->lru);
-
 		ret = binder_install_single_page(alloc, page, page_addr);
 		if (ret) {
 			binder_free_page_range(alloc, start, page_addr);
@@ -798,9 +795,9 @@ void binder_alloc_free_buf(struct binder_alloc *alloc,
 int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 			      struct vm_area_struct *vma)
 {
-	int ret;
-	const char *failure_string;
 	struct binder_buffer *buffer;
+	const char *failure_string;
+	int ret, i;
 
 	if (unlikely(vma->vm_mm != alloc->mm)) {
 		ret = -EINVAL;
@@ -829,6 +826,11 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
 		goto err_alloc_pages_failed;
 	}
 
+	for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
+		alloc->pages[i].alloc = alloc;
+		INIT_LIST_HEAD(&alloc->pages[i].lru);
+	}
+
 	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
 	if (!buffer) {
 		ret = -ENOMEM;
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 19/28] binder: perform page installation outside of locks

[Carlos Llamas]

Split out the insertion of pages to be outside of the alloc->mutex in a
separate binder_install_buffer_pages() routine. Since this is no longer
serialized, we must look at the full range of pages used by the buffers.
The installation is protected with mmap_sem in write mode since multiple
tasks might race to install the same page.

Besides avoiding unnecessary nested locking this helps in preparation of
switching the alloc->mutex into a spinlock_t in subsequent patches.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 101 ++++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 28 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 25efdbb2ad5d..551f08e84408 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -175,6 +175,21 @@ struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
 	return buffer;
 }
 
+static inline void
+binder_set_installed_page(struct binder_lru_page *lru_page,
+			  struct page *page)
+{
+	/* Pairs with acquire in binder_get_installed_page() */
+	smp_store_release(&lru_page->page_ptr, page);
+}
+
+static inline struct page *
+binder_get_installed_page(struct binder_lru_page *lru_page)
+{
+	/* Pairs with release in binder_set_installed_page() */
+	return smp_load_acquire(&lru_page->page_ptr);
+}
+
 static void binder_free_page_range(struct binder_alloc *alloc,
 				   unsigned long start, unsigned long end)
 {
@@ -190,6 +205,9 @@ static void binder_free_page_range(struct binder_alloc *alloc,
 		index = (page_addr - alloc->buffer) / PAGE_SIZE;
 		page = &alloc->pages[index];
 
+		if (!binder_get_installed_page(page))
+			continue;
+
 		trace_binder_free_lru_start(alloc, index);
 
 		ret = list_lru_add(&binder_alloc_lru, &page->lru);
@@ -209,7 +227,14 @@ static int binder_install_single_page(struct binder_alloc *alloc,
 	if (!mmget_not_zero(alloc->mm))
 		return -ESRCH;
 
+	/*
+	 * Protected with mmap_sem in write mode as multiple tasks
+	 * might race to install the same page.
+	 */
 	mmap_write_lock(alloc->mm);
+	if (binder_get_installed_page(lru_page))
+		goto out;
+
 	if (!alloc->vma) {
 		pr_err("%d: %s failed, no vma\n", alloc->pid, __func__);
 		ret = -ESRCH;
@@ -232,15 +257,50 @@ static int binder_install_single_page(struct binder_alloc *alloc,
 		goto out;
 	}
 
-	lru_page->page_ptr = page;
+	/* Mark page installation complete and safe to use */
+	binder_set_installed_page(lru_page, page);
 out:
 	mmap_write_unlock(alloc->mm);
 	mmput_async(alloc->mm);
 	return ret;
 }
 
-static int binder_allocate_page_range(struct binder_alloc *alloc,
-				      unsigned long start, unsigned long end)
+static int binder_install_buffer_pages(struct binder_alloc *alloc,
+				       struct binder_buffer *buffer,
+				       size_t size)
+{
+	struct binder_lru_page *page;
+	unsigned long start, final;
+	unsigned long page_addr;
+
+	start = buffer->user_data & PAGE_MASK;
+	final = PAGE_ALIGN(buffer->user_data + size);
+
+	for (page_addr = start; page_addr < final; page_addr += PAGE_SIZE) {
+		unsigned long index;
+		int ret;
+
+		index = (page_addr - alloc->buffer) / PAGE_SIZE;
+		page = &alloc->pages[index];
+
+		if (binder_get_installed_page(page))
+			continue;
+
+		trace_binder_alloc_page_start(alloc, index);
+
+		ret = binder_install_single_page(alloc, page, page_addr);
+		if (ret)
+			return ret;
+
+		trace_binder_alloc_page_end(alloc, index);
+	}
+
+	return 0;
+}
+
+/* The range of pages should exclude those shared with other buffers */
+static void binder_allocate_page_range(struct binder_alloc *alloc,
+				       unsigned long start, unsigned long end)
 {
 	struct binder_lru_page *page;
 	unsigned long page_addr;
@@ -249,15 +309,11 @@ static int binder_allocate_page_range(struct binder_alloc *alloc,
 			   "%d: allocate pages %lx-%lx\n",
 			   alloc->pid, start, end);
 
-	if (end <= start)
-		return 0;
-
 	trace_binder_update_page_range(alloc, true, start, end);
 
 	for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
 		unsigned long index;
 		bool on_lru;
-		int ret;
 
 		index = (page_addr - alloc->buffer) / PAGE_SIZE;
 		page = &alloc->pages[index];
@@ -272,21 +328,9 @@ static int binder_allocate_page_range(struct binder_alloc *alloc,
 			continue;
 		}
 
-		trace_binder_alloc_page_start(alloc, index);
-
-		ret = binder_install_single_page(alloc, page, page_addr);
-		if (ret) {
-			binder_free_page_range(alloc, start, page_addr);
-			return ret;
-		}
-
 		if (index + 1 > alloc->pages_high)
 			alloc->pages_high = index + 1;
-
-		trace_binder_alloc_page_end(alloc, index);
 	}
-
-	return 0;
 }
 
 static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
@@ -405,7 +449,6 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	unsigned long has_page_addr;
 	unsigned long end_page_addr;
 	size_t buffer_size;
-	int ret;
 
 	if (is_async && alloc->free_async_space < size) {
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
@@ -449,18 +492,14 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		     "%d: binder_alloc_buf size %zd got buffer %pK size %zd\n",
 		      alloc->pid, size, buffer, buffer_size);
 
-	has_page_addr = (buffer->user_data + buffer_size) & PAGE_MASK;
 	WARN_ON(n && buffer_size != size);
+
+	has_page_addr = (buffer->user_data + buffer_size) & PAGE_MASK;
 	end_page_addr = PAGE_ALIGN(buffer->user_data + size);
 	if (end_page_addr > has_page_addr)
 		end_page_addr = has_page_addr;
-	ret = binder_allocate_page_range(alloc, PAGE_ALIGN(buffer->user_data),
-					 end_page_addr);
-	if (ret) {
-		buffer = ERR_PTR(ret);
-		goto out;
-	}
-
+	binder_allocate_page_range(alloc, PAGE_ALIGN(buffer->user_data),
+				   end_page_addr);
 	if (buffer_size != size) {
 		new_buffer->user_data = buffer->user_data + size;
 		list_add(&new_buffer->entry, &buffer->entry);
@@ -538,6 +577,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 {
 	struct binder_buffer *buffer, *next;
 	size_t size;
+	int ret;
 
 	/* Check binder_alloc is fully initialized */
 	if (!binder_alloc_get_vma(alloc)) {
@@ -574,6 +614,11 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	buffer->pid = current->tgid;
 	mutex_unlock(&alloc->mutex);
 
+	ret = binder_install_buffer_pages(alloc, buffer, size);
+	if (ret) {
+		binder_alloc_free_buf(alloc, buffer);
+		buffer = ERR_PTR(ret);
+	}
 out:
 	return buffer;
 }
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 19/28] binder: perform page installation outside of locks

[Alice Ryhl]

> Split out the insertion of pages to be outside of the alloc->mutex in a
> separate binder_install_buffer_pages() routine. Since this is no longer
> serialized, we must look at the full range of pages used by the buffers.
> The installation is protected with mmap_sem in write mode since multiple
> tasks might race to install the same page.
> 
> Besides avoiding unnecessary nested locking this helps in preparation of
> switching the alloc->mutex into a spinlock_t in subsequent patches.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +static inline struct page *
> +binder_get_installed_page(struct binder_lru_page *lru_page)
> +{
> +	/* Pairs with release in binder_set_installed_page() */
> +	return smp_load_acquire(&lru_page->page_ptr);
> +}

Technically some of the uses of this method do not actually need to be
atomic (because they never race with `binder_set_installed_page`), but I
don't mind using it anyway.

Alice

[PATCH v2 20/28] binder: remove redundant debug log

[Carlos Llamas]

The debug information in this statement is already logged earlier in the
same function. We can get rid of this duplicate log.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 551f08e84408..c9292eee8fee 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -512,9 +512,6 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	buffer->free = 0;
 	buffer->allow_user_free = 0;
 	binder_insert_allocated_buffer_locked(alloc, buffer);
-	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-		     "%d: binder_alloc_buf size %zd got %pK\n",
-		      alloc->pid, size, buffer);
 	buffer->async_transaction = is_async;
 	buffer->oneway_spam_suspect = false;
 	if (is_async) {
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 20/28] binder: remove redundant debug log

[Alice Ryhl]

> The debug information in this statement is already logged earlier in the
> same function. We can get rid of this duplicate log.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v2 21/28] binder: make oversized buffer code more readable

[Carlos Llamas]

The sections in binder_alloc_new_buf_locked() dealing with oversized
buffers are scattered which makes them difficult to read. Instead,
consolidate this code into a single block to improve readability.

No functional change here.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index c9292eee8fee..ad9b73c6ddb7 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -483,32 +483,31 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		goto out;
 	}
 
-	if (n == NULL) {
+	if (buffer_size != size) {
+		/* Found an oversized buffer and needs to be split */
 		buffer = rb_entry(best_fit, struct binder_buffer, rb_node);
 		buffer_size = binder_alloc_buffer_size(alloc, buffer);
+
+		WARN_ON(n || buffer_size == size);
+		new_buffer->user_data = buffer->user_data + size;
+		list_add(&new_buffer->entry, &buffer->entry);
+		new_buffer->free = 1;
+		binder_insert_free_buffer(alloc, new_buffer);
+		new_buffer = NULL;
 	}
 
 	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
 		     "%d: binder_alloc_buf size %zd got buffer %pK size %zd\n",
 		      alloc->pid, size, buffer, buffer_size);
 
-	WARN_ON(n && buffer_size != size);
-
 	has_page_addr = (buffer->user_data + buffer_size) & PAGE_MASK;
 	end_page_addr = PAGE_ALIGN(buffer->user_data + size);
 	if (end_page_addr > has_page_addr)
 		end_page_addr = has_page_addr;
 	binder_allocate_page_range(alloc, PAGE_ALIGN(buffer->user_data),
 				   end_page_addr);
-	if (buffer_size != size) {
-		new_buffer->user_data = buffer->user_data + size;
-		list_add(&new_buffer->entry, &buffer->entry);
-		new_buffer->free = 1;
-		binder_insert_free_buffer(alloc, new_buffer);
-		new_buffer = NULL;
-	}
 
-	rb_erase(best_fit, &alloc->free_buffers);
+	rb_erase(&buffer->rb_node, &alloc->free_buffers);
 	buffer->free = 0;
 	buffer->allow_user_free = 0;
 	binder_insert_allocated_buffer_locked(alloc, buffer);
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 21/28] binder: make oversized buffer code more readable

[Alice Ryhl]

> The sections in binder_alloc_new_buf_locked() dealing with oversized
> buffers are scattered which makes them difficult to read. Instead,
> consolidate this code into a single block to improve readability.
> 
> No functional change here.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Tricky, but looks ok.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v2 22/28] binder: rename lru shrinker utilities

[Carlos Llamas]

Now that the page allocation step is done separately we should rename
the binder_free_page_range() and binder_allocate_page_range() functions
to provide a more accurate description of what they do. Lets borrow the
freelist concept used in other parts of the kernel for this.

No functional change here.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c          | 40 ++++++++++++-------------
 drivers/android/binder_alloc.h          |  4 +--
 drivers/android/binder_alloc_selftest.c |  6 ++--
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index ad9b73c6ddb7..052a8c3b0ce1 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -26,7 +26,7 @@
 #include "binder_alloc.h"
 #include "binder_trace.h"
 
-struct list_lru binder_alloc_lru;
+struct list_lru binder_freelist;
 
 static DEFINE_MUTEX(binder_alloc_mmap_lock);
 
@@ -190,8 +190,8 @@ binder_get_installed_page(struct binder_lru_page *lru_page)
 	return smp_load_acquire(&lru_page->page_ptr);
 }
 
-static void binder_free_page_range(struct binder_alloc *alloc,
-				   unsigned long start, unsigned long end)
+static void binder_lru_freelist_add(struct binder_alloc *alloc,
+				    unsigned long start, unsigned long end)
 {
 	struct binder_lru_page *page;
 	unsigned long page_addr;
@@ -210,7 +210,7 @@ static void binder_free_page_range(struct binder_alloc *alloc,
 
 		trace_binder_free_lru_start(alloc, index);
 
-		ret = list_lru_add(&binder_alloc_lru, &page->lru);
+		ret = list_lru_add(&binder_freelist, &page->lru);
 		WARN_ON(!ret);
 
 		trace_binder_free_lru_end(alloc, index);
@@ -299,14 +299,14 @@ static int binder_install_buffer_pages(struct binder_alloc *alloc,
 }
 
 /* The range of pages should exclude those shared with other buffers */
-static void binder_allocate_page_range(struct binder_alloc *alloc,
-				       unsigned long start, unsigned long end)
+static void binder_lru_freelist_del(struct binder_alloc *alloc,
+				    unsigned long start, unsigned long end)
 {
 	struct binder_lru_page *page;
 	unsigned long page_addr;
 
 	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-			   "%d: allocate pages %lx-%lx\n",
+			   "%d: pages %lx-%lx\n",
 			   alloc->pid, start, end);
 
 	trace_binder_update_page_range(alloc, true, start, end);
@@ -321,7 +321,7 @@ static void binder_allocate_page_range(struct binder_alloc *alloc,
 		if (page->page_ptr) {
 			trace_binder_alloc_lru_start(alloc, index);
 
-			on_lru = list_lru_del(&binder_alloc_lru, &page->lru);
+			on_lru = list_lru_del(&binder_freelist, &page->lru);
 			WARN_ON(!on_lru);
 
 			trace_binder_alloc_lru_end(alloc, index);
@@ -504,8 +504,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	end_page_addr = PAGE_ALIGN(buffer->user_data + size);
 	if (end_page_addr > has_page_addr)
 		end_page_addr = has_page_addr;
-	binder_allocate_page_range(alloc, PAGE_ALIGN(buffer->user_data),
-				   end_page_addr);
+	binder_lru_freelist_del(alloc, PAGE_ALIGN(buffer->user_data),
+				end_page_addr);
 
 	rb_erase(&buffer->rb_node, &alloc->free_buffers);
 	buffer->free = 0;
@@ -671,8 +671,8 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc,
 				   alloc->pid, buffer->user_data,
 				   prev->user_data,
 				   next ? next->user_data : 0);
-		binder_free_page_range(alloc, buffer_start_page(buffer),
-				       buffer_start_page(buffer) + PAGE_SIZE);
+		binder_lru_freelist_add(alloc, buffer_start_page(buffer),
+					buffer_start_page(buffer) + PAGE_SIZE);
 	}
 	list_del(&buffer->entry);
 	kfree(buffer);
@@ -706,8 +706,8 @@ static void binder_free_buf_locked(struct binder_alloc *alloc,
 			      alloc->pid, size, alloc->free_async_space);
 	}
 
-	binder_free_page_range(alloc, PAGE_ALIGN(buffer->user_data),
-			       (buffer->user_data + buffer_size) & PAGE_MASK);
+	binder_lru_freelist_add(alloc, PAGE_ALIGN(buffer->user_data),
+				(buffer->user_data + buffer_size) & PAGE_MASK);
 
 	rb_erase(&buffer->rb_node, &alloc->allocated_buffers);
 	buffer->free = 1;
@@ -953,7 +953,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 			if (!alloc->pages[i].page_ptr)
 				continue;
 
-			on_lru = list_lru_del(&binder_alloc_lru,
+			on_lru = list_lru_del(&binder_freelist,
 					      &alloc->pages[i].lru);
 			page_addr = alloc->buffer + i * PAGE_SIZE;
 			binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
@@ -1152,13 +1152,13 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 static unsigned long
 binder_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
 {
-	return list_lru_count(&binder_alloc_lru);
+	return list_lru_count(&binder_freelist);
 }
 
 static unsigned long
 binder_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
 {
-	return list_lru_walk(&binder_alloc_lru, binder_alloc_free_page,
+	return list_lru_walk(&binder_freelist, binder_alloc_free_page,
 			    NULL, sc->nr_to_scan);
 }
 
@@ -1184,13 +1184,13 @@ int binder_alloc_shrinker_init(void)
 {
 	int ret;
 
-	ret = list_lru_init(&binder_alloc_lru);
+	ret = list_lru_init(&binder_freelist);
 	if (ret)
 		return ret;
 
 	binder_shrinker = shrinker_alloc(0, "android-binder");
 	if (!binder_shrinker) {
-		list_lru_destroy(&binder_alloc_lru);
+		list_lru_destroy(&binder_freelist);
 		return -ENOMEM;
 	}
 
@@ -1205,7 +1205,7 @@ int binder_alloc_shrinker_init(void)
 void binder_alloc_shrinker_exit(void)
 {
 	shrinker_free(binder_shrinker);
-	list_lru_destroy(&binder_alloc_lru);
+	list_lru_destroy(&binder_freelist);
 }
 
 /**
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index bbc16bc6d5ac..a5181916942e 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -15,7 +15,7 @@
 #include <linux/list_lru.h>
 #include <uapi/linux/android/binder.h>
 
-extern struct list_lru binder_alloc_lru;
+extern struct list_lru binder_freelist;
 struct binder_transaction;
 
 /**
@@ -61,7 +61,7 @@ struct binder_buffer {
 /**
  * struct binder_lru_page - page object used for binder shrinker
  * @page_ptr: pointer to physical page in mmap'd space
- * @lru:      entry in binder_alloc_lru
+ * @lru:      entry in binder_freelist
  * @alloc:    binder_alloc for a proc
  */
 struct binder_lru_page {
diff --git a/drivers/android/binder_alloc_selftest.c b/drivers/android/binder_alloc_selftest.c
index ed753747e54c..fba7ab6ca451 100644
--- a/drivers/android/binder_alloc_selftest.c
+++ b/drivers/android/binder_alloc_selftest.c
@@ -158,8 +158,8 @@ static void binder_selftest_free_page(struct binder_alloc *alloc)
 	int i;
 	unsigned long count;
 
-	while ((count = list_lru_count(&binder_alloc_lru))) {
-		list_lru_walk(&binder_alloc_lru, binder_alloc_free_page,
+	while ((count = list_lru_count(&binder_freelist))) {
+		list_lru_walk(&binder_freelist, binder_alloc_free_page,
 			      NULL, count);
 	}
 
@@ -183,7 +183,7 @@ static void binder_selftest_alloc_free(struct binder_alloc *alloc,
 
 	/* Allocate from lru. */
 	binder_selftest_alloc_buf(alloc, buffers, sizes, seq);
-	if (list_lru_count(&binder_alloc_lru))
+	if (list_lru_count(&binder_freelist))
 		pr_err("lru list should be empty but is not\n");
 
 	binder_selftest_free_buf(alloc, buffers, sizes, seq, end);
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 22/28] binder: rename lru shrinker utilities

[Alice Ryhl]

> Now that the page allocation step is done separately we should rename
> the binder_free_page_range() and binder_allocate_page_range() functions
> to provide a more accurate description of what they do. Lets borrow the
> freelist concept used in other parts of the kernel for this.
> 
> No functional change here.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v2 23/28] binder: document the final page calculation

[Carlos Llamas]

The code to determine the page range for binder_lru_freelist_del() is
quite obscure. It leverages the buffer_size calculated before doing an
oversized buffer split. This is used to figure out if the last page is
being shared with another active buffer. If so, the page gets trimmed
out of the range as it has been previously removed from the freelist.

This would be equivalent to getting the start page of the next in-use
buffer explicitly. However, the code for this is much larger as we can
see in binder_free_buf_locked() routine. Instead, lets settle on
documenting the tricky step and using better names for now.

I believe an ideal solution would be to count the binder_page->users to
determine when a page should be added or removed from the freelist.
However, this is a much bigger change than what I'm willing to risk at
this time.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 052a8c3b0ce1..edd9714ec9f5 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -446,8 +446,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 	struct rb_node *n = alloc->free_buffers.rb_node;
 	struct rb_node *best_fit = NULL;
 	struct binder_buffer *buffer;
-	unsigned long has_page_addr;
-	unsigned long end_page_addr;
+	unsigned long next_used_page;
+	unsigned long curr_last_page;
 	size_t buffer_size;
 
 	if (is_async && alloc->free_async_space < size) {
@@ -500,12 +500,16 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 		     "%d: binder_alloc_buf size %zd got buffer %pK size %zd\n",
 		      alloc->pid, size, buffer, buffer_size);
 
-	has_page_addr = (buffer->user_data + buffer_size) & PAGE_MASK;
-	end_page_addr = PAGE_ALIGN(buffer->user_data + size);
-	if (end_page_addr > has_page_addr)
-		end_page_addr = has_page_addr;
+	/*
+	 * Now we remove the pages from the freelist. A clever calculation
+	 * with buffer_size determines if the last page is shared with an
+	 * adjacent in-use buffer. In such case, the page has been already
+	 * removed from the freelist so we trim our range short.
+	 */
+	next_used_page = (buffer->user_data + buffer_size) & PAGE_MASK;
+	curr_last_page = PAGE_ALIGN(buffer->user_data + size);
 	binder_lru_freelist_del(alloc, PAGE_ALIGN(buffer->user_data),
-				end_page_addr);
+				min(next_used_page, curr_last_page));
 
 	rb_erase(&buffer->rb_node, &alloc->free_buffers);
 	buffer->free = 0;
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 23/28] binder: document the final page calculation

[Alice Ryhl]

> The code to determine the page range for binder_lru_freelist_del() is
> quite obscure. It leverages the buffer_size calculated before doing an
> oversized buffer split. This is used to figure out if the last page is
> being shared with another active buffer. If so, the page gets trimmed
> out of the range as it has been previously removed from the freelist.
> 
> This would be equivalent to getting the start page of the next in-use
> buffer explicitly. However, the code for this is much larger as we can
> see in binder_free_buf_locked() routine. Instead, lets settle on
> documenting the tricky step and using better names for now.
> 
> I believe an ideal solution would be to count the binder_page->users to
> determine when a page should be added or removed from the freelist.
> However, this is a much bigger change than what I'm willing to risk at
> this time.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Yes, this does help somewhat.

However, `curr_last_page` is actually not the last page. It's the last
page plus one, since `binder_lru_freelist_del` is exclusive on this
argument. Maybe rename it to `curr_after_last_page` or something like
that? Or maybe even just `curr_last_page_plus_one`.

Alice

Re: [PATCH v2 23/28] binder: document the final page calculation

[Carlos Llamas]

On Mon, Dec 04, 2023 at 11:57:27AM +0000, Alice Ryhl wrote:
> > The code to determine the page range for binder_lru_freelist_del() is
> > quite obscure. It leverages the buffer_size calculated before doing an
> > oversized buffer split. This is used to figure out if the last page is
> > being shared with another active buffer. If so, the page gets trimmed
> > out of the range as it has been previously removed from the freelist.
> > 
> > This would be equivalent to getting the start page of the next in-use
> > buffer explicitly. However, the code for this is much larger as we can
> > see in binder_free_buf_locked() routine. Instead, lets settle on
> > documenting the tricky step and using better names for now.
> > 
> > I believe an ideal solution would be to count the binder_page->users to
> > determine when a page should be added or removed from the freelist.
> > However, this is a much bigger change than what I'm willing to risk at
> > this time.
> > 
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> 
> Yes, this does help somewhat.
> 
> However, `curr_last_page` is actually not the last page. It's the last
> page plus one, since `binder_lru_freelist_del` is exclusive on this
> argument. Maybe rename it to `curr_after_last_page` or something like
> that? Or maybe even just `curr_last_page_plus_one`.

hmmm, I don't know. I think this could be more confusing, the plus-one
is only because of the way that binder_lru_freelist_del() processes the
final page. So you could interpret the name both ways. Do we _really_
need the extra comments to make it clear?

This solution is too complex anyway, it should really be replaced with a
binder_page->nr_users to determine when to add/remove from the lru.

--
Carlos Llamas

Re: [PATCH v2 23/28] binder: document the final page calculation

[Alice Ryhl]

On Mon, Dec 4, 2023 at 3:39 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Mon, Dec 04, 2023 at 11:57:27AM +0000, Alice Ryhl wrote:
> > > The code to determine the page range for binder_lru_freelist_del() is
> > > quite obscure. It leverages the buffer_size calculated before doing an
> > > oversized buffer split. This is used to figure out if the last page is
> > > being shared with another active buffer. If so, the page gets trimmed
> > > out of the range as it has been previously removed from the freelist.
> > >
> > > This would be equivalent to getting the start page of the next in-use
> > > buffer explicitly. However, the code for this is much larger as we can
> > > see in binder_free_buf_locked() routine. Instead, lets settle on
> > > documenting the tricky step and using better names for now.
> > >
> > > I believe an ideal solution would be to count the binder_page->users to
> > > determine when a page should be added or removed from the freelist.
> > > However, this is a much bigger change than what I'm willing to risk at
> > > this time.
> > >
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> >
> > Yes, this does help somewhat.
> >
> > However, `curr_last_page` is actually not the last page. It's the last
> > page plus one, since `binder_lru_freelist_del` is exclusive on this
> > argument. Maybe rename it to `curr_after_last_page` or something like
> > that? Or maybe even just `curr_last_page_plus_one`.
>
> hmmm, I don't know. I think this could be more confusing, the plus-one
> is only because of the way that binder_lru_freelist_del() processes the
> final page. So you could interpret the name both ways. Do we _really_
> need the extra comments to make it clear?
>
> This solution is too complex anyway, it should really be replaced with a
> binder_page->nr_users to determine when to add/remove from the lru.

You could also just remove the `next_used_page` part entirely. This
means that you will sometimes call `binder_lru_freelist_del` on a page
that's in use, but that's just a no-op.

Alice

Re: [PATCH v2 23/28] binder: document the final page calculation

[Carlos Llamas]

On Mon, Dec 04, 2023 at 03:43:54PM +0100, Alice Ryhl wrote:
> You could also just remove the `next_used_page` part entirely. This
> means that you will sometimes call `binder_lru_freelist_del` on a page
> that's in use, but that's just a no-op.

Yes, that is right and I did look into this. The WARN_ON(!on_lru) checks
are a bit confusing when it's a no-op as you mention. The purpose though
is to assert the "algorithm" of add/del pages from the lru is somewhat
correct (particularly the page range).

--
Carlos Llamas

[PATCH v2 24/28] binder: collapse print_binder_buffer() into caller

[Carlos Llamas]

The code in print_binder_buffer() is quite small so it can be collapsed
into its single caller binder_alloc_print_allocated().

No functional change in this patch.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index edd9714ec9f5..60c829506d31 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -978,16 +978,6 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 		     __func__, alloc->pid, buffers, page_count);
 }
 
-static void print_binder_buffer(struct seq_file *m, const char *prefix,
-				struct binder_buffer *buffer)
-{
-	seq_printf(m, "%s %d: %lx size %zd:%zd:%zd %s\n",
-		   prefix, buffer->debug_id, buffer->user_data,
-		   buffer->data_size, buffer->offsets_size,
-		   buffer->extra_buffers_size,
-		   buffer->transaction ? "active" : "delivered");
-}
-
 /**
  * binder_alloc_print_allocated() - print buffer info
  * @m:     seq_file for output via seq_printf()
@@ -999,12 +989,18 @@ static void print_binder_buffer(struct seq_file *m, const char *prefix,
 void binder_alloc_print_allocated(struct seq_file *m,
 				  struct binder_alloc *alloc)
 {
+	struct binder_buffer *buffer;
 	struct rb_node *n;
 
 	mutex_lock(&alloc->mutex);
-	for (n = rb_first(&alloc->allocated_buffers); n != NULL; n = rb_next(n))
-		print_binder_buffer(m, "  buffer",
-				    rb_entry(n, struct binder_buffer, rb_node));
+	for (n = rb_first(&alloc->allocated_buffers); n; n = rb_next(n)) {
+		buffer = rb_entry(n, struct binder_buffer, rb_node);
+		seq_printf(m, "  buffer %d: %lx size %zd:%zd:%zd %s\n",
+			   buffer->debug_id, buffer->user_data,
+			   buffer->data_size, buffer->offsets_size,
+			   buffer->extra_buffers_size,
+			   buffer->transaction ? "active" : "delivered");
+	}
 	mutex_unlock(&alloc->mutex);
 }
 
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 24/28] binder: collapse print_binder_buffer() into caller

[Alice Ryhl]

> The code in print_binder_buffer() is quite small so it can be collapsed
> into its single caller binder_alloc_print_allocated().
> 
> No functional change in this patch.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v2 25/28] binder: refactor binder_delete_free_buffer()

[Carlos Llamas]

Skip the freelist call immediately as needed, instead of continuing the
pointless checks. Also, drop the debug logs that we don't really need.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 44 +++++++++-------------------------
 1 file changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 60c829506d31..c3fc90966867 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -636,48 +636,26 @@ static unsigned long prev_buffer_end_page(struct binder_buffer *buffer)
 static void binder_delete_free_buffer(struct binder_alloc *alloc,
 				      struct binder_buffer *buffer)
 {
-	struct binder_buffer *prev, *next = NULL;
-	bool to_free = true;
+	struct binder_buffer *prev, *next;
+
+	if (PAGE_ALIGNED(buffer->user_data))
+		goto skip_freelist;
 
 	BUG_ON(alloc->buffers.next == &buffer->entry);
 	prev = binder_buffer_prev(buffer);
 	BUG_ON(!prev->free);
-	if (prev_buffer_end_page(prev) == buffer_start_page(buffer)) {
-		to_free = false;
-		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				   "%d: merge free, buffer %lx share page with %lx\n",
-				   alloc->pid, buffer->user_data,
-				   prev->user_data);
-	}
+	if (prev_buffer_end_page(prev) == buffer_start_page(buffer))
+		goto skip_freelist;
 
 	if (!list_is_last(&buffer->entry, &alloc->buffers)) {
 		next = binder_buffer_next(buffer);
-		if (buffer_start_page(next) == buffer_start_page(buffer)) {
-			to_free = false;
-			binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-					   "%d: merge free, buffer %lx share page with %lx\n",
-					   alloc->pid,
-					   buffer->user_data,
-					   next->user_data);
-		}
+		if (buffer_start_page(next) == buffer_start_page(buffer))
+			goto skip_freelist;
 	}
 
-	if (PAGE_ALIGNED(buffer->user_data)) {
-		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				   "%d: merge free, buffer start %lx is page aligned\n",
-				   alloc->pid, buffer->user_data);
-		to_free = false;
-	}
-
-	if (to_free) {
-		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				   "%d: merge free, buffer %lx do not share page with %lx or %lx\n",
-				   alloc->pid, buffer->user_data,
-				   prev->user_data,
-				   next ? next->user_data : 0);
-		binder_lru_freelist_add(alloc, buffer_start_page(buffer),
-					buffer_start_page(buffer) + PAGE_SIZE);
-	}
+	binder_lru_freelist_add(alloc, buffer_start_page(buffer),
+				buffer_start_page(buffer) + PAGE_SIZE);
+skip_freelist:
 	list_del(&buffer->entry);
 	kfree(buffer);
 }
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 25/28] binder: refactor binder_delete_free_buffer()

[Alice Ryhl]

> Skip the freelist call immediately as needed, instead of continuing the
> pointless checks. Also, drop the debug logs that we don't really need.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v2 26/28] binder: avoid user addresses in debug logs

[Carlos Llamas]

Prefer logging vma offsets instead of addresses or simply drop the debug
log altogether if not useful. Note this covers the instances affected by
the switch to store addresses as unsigned long. However, there are other
sections in the driver that could do the same.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c       |  4 ++--
 drivers/android/binder_alloc.c | 15 ++++++---------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 45674af6310f..c4bb18305e77 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5980,9 +5980,9 @@ static void print_binder_transaction_ilocked(struct seq_file *m,
 	}
 	if (buffer->target_node)
 		seq_printf(m, " node %d", buffer->target_node->debug_id);
-	seq_printf(m, " size %zd:%zd data %lx\n",
+	seq_printf(m, " size %zd:%zd offset %lx\n",
 		   buffer->data_size, buffer->offsets_size,
-		   buffer->user_data);
+		   proc->alloc.buffer - buffer->user_data);
 }
 
 static void print_binder_work_ilocked(struct seq_file *m,
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index c3fc90966867..5783675f2970 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -250,8 +250,8 @@ static int binder_install_single_page(struct binder_alloc *alloc,
 
 	ret = vm_insert_page(alloc->vma, addr, page);
 	if (ret) {
-		pr_err("%d: %s failed to insert page at %lx with %d\n",
-		       alloc->pid, __func__, addr, ret);
+		pr_err("%d: %s failed to insert page at offset %lx with %d\n",
+		       alloc->pid, __func__, addr - alloc->buffer, ret);
 		__free_page(page);
 		ret = -ENOMEM;
 		goto out;
@@ -305,10 +305,6 @@ static void binder_lru_freelist_del(struct binder_alloc *alloc,
 	struct binder_lru_page *page;
 	unsigned long page_addr;
 
-	binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-			   "%d: pages %lx-%lx\n",
-			   alloc->pid, start, end);
-
 	trace_binder_update_page_range(alloc, true, start, end);
 
 	for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
@@ -939,8 +935,8 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 					      &alloc->pages[i].lru);
 			page_addr = alloc->buffer + i * PAGE_SIZE;
 			binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
-				     "%s: %d: page %d at %lx %s\n",
-				     __func__, alloc->pid, i, page_addr,
+				     "%s: %d: page %d %s\n",
+				     __func__, alloc->pid, i,
 				     on_lru ? "on lru" : "active");
 			__free_page(alloc->pages[i].page_ptr);
 			page_count++;
@@ -974,7 +970,8 @@ void binder_alloc_print_allocated(struct seq_file *m,
 	for (n = rb_first(&alloc->allocated_buffers); n; n = rb_next(n)) {
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
 		seq_printf(m, "  buffer %d: %lx size %zd:%zd:%zd %s\n",
-			   buffer->debug_id, buffer->user_data,
+			   buffer->debug_id,
+			   buffer->user_data - alloc->buffer,
 			   buffer->data_size, buffer->offsets_size,
 			   buffer->extra_buffers_size,
 			   buffer->transaction ? "active" : "delivered");
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 26/28] binder: avoid user addresses in debug logs

[Alice Ryhl]

> Prefer logging vma offsets instead of addresses or simply drop the debug
> log altogether if not useful. Note this covers the instances affected by
> the switch to store addresses as unsigned long. However, there are other
> sections in the driver that could do the same.
> 
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v2 27/28] binder: reverse locking order in shrinker callback

[Carlos Llamas]

The locking order currently requires the alloc->mutex to be acquired
first followed by the mmap lock. However, the alloc->mutex is converted
into a spinlock in subsequent commits so the order needs to be reversed
to avoid nesting the sleeping mmap lock under the spinlock.

The shrinker's callback binder_alloc_free_page() is the only place that
needs to be reordered since other functions have been refactored and no
longer nest these locks.

Some minor cosmetic changes are also included in this patch.

Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 46 ++++++++++++++++------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 5783675f2970..a3e56637db4f 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1061,35 +1061,39 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 				       void *cb_arg)
 	__must_hold(lock)
 {
-	struct mm_struct *mm = NULL;
-	struct binder_lru_page *page = container_of(item,
-						    struct binder_lru_page,
-						    lru);
-	struct binder_alloc *alloc;
+	struct binder_lru_page *page = container_of(item, typeof(*page), lru);
+	struct binder_alloc *alloc = page->alloc;
+	struct mm_struct *mm = alloc->mm;
+	struct vm_area_struct *vma;
+	struct page *page_to_free;
 	unsigned long page_addr;
 	size_t index;
-	struct vm_area_struct *vma;
 
-	alloc = page->alloc;
+	if (!mmget_not_zero(mm))
+		goto err_mmget;
+	if (!mmap_read_trylock(mm))
+		goto err_mmap_read_lock_failed;
 	if (!mutex_trylock(&alloc->mutex))
 		goto err_get_alloc_mutex_failed;
-
 	if (!page->page_ptr)
 		goto err_page_already_freed;
 
 	index = page - alloc->pages;
 	page_addr = alloc->buffer + index * PAGE_SIZE;
 
-	mm = alloc->mm;
-	if (!mmget_not_zero(mm))
-		goto err_mmget;
-	if (!mmap_read_trylock(mm))
-		goto err_mmap_read_lock_failed;
 	vma = vma_lookup(mm, page_addr);
 	if (vma && vma != binder_alloc_get_vma(alloc))
 		goto err_invalid_vma;
 
+	trace_binder_unmap_kernel_start(alloc, index);
+
+	page_to_free = page->page_ptr;
+	page->page_ptr = NULL;
+
+	trace_binder_unmap_kernel_end(alloc, index);
+
 	list_lru_isolate(lru, item);
+	mutex_unlock(&alloc->mutex);
 	spin_unlock(lock);
 
 	if (vma) {
@@ -1099,28 +1103,22 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 
 		trace_binder_unmap_user_end(alloc, index);
 	}
+
 	mmap_read_unlock(mm);
 	mmput_async(mm);
-
-	trace_binder_unmap_kernel_start(alloc, index);
-
-	__free_page(page->page_ptr);
-	page->page_ptr = NULL;
-
-	trace_binder_unmap_kernel_end(alloc, index);
+	__free_page(page_to_free);
 
 	spin_lock(lock);
-	mutex_unlock(&alloc->mutex);
 	return LRU_REMOVED_RETRY;
 
 err_invalid_vma:
+err_page_already_freed:
+	mutex_unlock(&alloc->mutex);
+err_get_alloc_mutex_failed:
 	mmap_read_unlock(mm);
 err_mmap_read_lock_failed:
 	mmput_async(mm);
 err_mmget:
-err_page_already_freed:
-	mutex_unlock(&alloc->mutex);
-err_get_alloc_mutex_failed:
 	return LRU_SKIP;
 }
 
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 27/28] binder: reverse locking order in shrinker callback

[Alice Ryhl]

> +	trace_binder_unmap_kernel_start(alloc, index);
> +
> +	page_to_free = page->page_ptr;
> +	page->page_ptr = NULL;
> +
> +	trace_binder_unmap_kernel_end(alloc, index);

What are these trace calls used for? Previously they wrapped the call to
__free_page, and happened after the `trace_binder_unmap_user_*` calls.

I'm guessing they should be moved down to wrap the call to __free_page.

Alice

Re: [PATCH v2 27/28] binder: reverse locking order in shrinker callback

[Carlos Llamas]

On Mon, Dec 04, 2023 at 11:57:42AM +0000, 'Alice Ryhl' via kernel-team wrote:
> > +	trace_binder_unmap_kernel_start(alloc, index);
> > +
> > +	page_to_free = page->page_ptr;
> > +	page->page_ptr = NULL;
> > +
> > +	trace_binder_unmap_kernel_end(alloc, index);
> 
> What are these trace calls used for? Previously they wrapped the call to
> __free_page, and happened after the `trace_binder_unmap_user_*` calls.
> 

It also used to wrap an unmap_kernel_range() call, which explains the
naming of the tracepoint I suppose. It looked like this:

	trace_binder_unmap_kernel_start(alloc, index);

	unmap_kernel_range(page_addr, PAGE_SIZE);
	__free_page(page->page_ptr);
	page->page_ptr = NULL;

	trace_binder_unmap_kernel_end(alloc, index);

My thinking was that we care more about the page->page_ptr clearing but
it sounds like we could just drop the tracepoint, we no longer have a
kernel mapped area.

--
Carlos Llamas

Re: [PATCH v2 27/28] binder: reverse locking order in shrinker callback

[Alice Ryhl]

On Mon, Dec 4, 2023 at 3:45 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Mon, Dec 04, 2023 at 11:57:42AM +0000, 'Alice Ryhl' via kernel-team wrote:
> > > +   trace_binder_unmap_kernel_start(alloc, index);
> > > +
> > > +   page_to_free = page->page_ptr;
> > > +   page->page_ptr = NULL;
> > > +
> > > +   trace_binder_unmap_kernel_end(alloc, index);
> >
> > What are these trace calls used for? Previously they wrapped the call to
> > __free_page, and happened after the `trace_binder_unmap_user_*` calls.
> >
>
> It also used to wrap an unmap_kernel_range() call, which explains the
> naming of the tracepoint I suppose. It looked like this:
>
>         trace_binder_unmap_kernel_start(alloc, index);
>
>         unmap_kernel_range(page_addr, PAGE_SIZE);
>         __free_page(page->page_ptr);
>         page->page_ptr = NULL;
>
>         trace_binder_unmap_kernel_end(alloc, index);
>
> My thinking was that we care more about the page->page_ptr clearing but
> it sounds like we could just drop the tracepoint, we no longer have a
> kernel mapped area.

Ok. Feel free to remove or move the trace calls however it needs to be
done and add my tag:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 27/28] binder: reverse locking order in shrinker callback

[Carlos Llamas]

On Mon, Dec 04, 2023 at 03:47:22PM +0100, Alice Ryhl wrote:
> Ok. Feel free to remove or move the trace calls however it needs to be
> done and add my tag:

Sounds good. I'll remove the trace calls in a follow up patch since it
needs to be separate, thanks.

--
Carlos Llamas

[PATCH v2 28/28] binder: switch alloc->mutex to spinlock_t

[Carlos Llamas]

The alloc->mutex is a highly contended lock that causes performance
issues on Android devices. When a low-priority task is given this lock
and it sleeps, it becomes difficult for the task to wake up and complete
its work. This delays other tasks that are also waiting on the mutex.

The problem gets worse when there is memory pressure in the system,
because this increases the contention on the alloc->mutex while the
shrinker reclaims binder pages.

Switching to a spinlock helps to keep the waiters running and avoids the
overhead of waking up tasks. This significantly improves the transaction
latency when the problematic scenario occurs.

The performance impact of this patchset was measured by stress-testing
the binder alloc contention. In this test, several clients of different
priorities send thousands of transactions of different sizes to a single
server. In parallel, pages get reclaimed using the shinker's debugfs.

The test was run on a Pixel 8, Pixel 6 and qemu machine. The results
were similar on all three devices:

after:
  | sched  | prio | average | max       | min     |
  |--------+------+---------+-----------+---------|
  | fifo   |   99 | 0.135ms |   1.197ms | 0.022ms |
  | fifo   |   01 | 0.136ms |   5.232ms | 0.018ms |
  | other  |  -20 | 0.180ms |   7.403ms | 0.019ms |
  | other  |   19 | 0.241ms |  58.094ms | 0.018ms |

before:
  | sched  | prio | average | max       | min     |
  |--------+------+---------+-----------+---------|
  | fifo   |   99 | 0.350ms | 248.730ms | 0.020ms |
  | fifo   |   01 | 0.357ms | 248.817ms | 0.024ms |
  | other  |  -20 | 0.399ms | 249.906ms | 0.020ms |
  | other  |   19 | 0.477ms | 297.756ms | 0.022ms |

The key metrics above are the average and max latencies (wall time).
These improvements should roughly translate to p95-p99 latencies on real
workloads. The response time is up to 200x faster in these scenarios and
there is no penalty in the regular path.

Note that it is only possible to convert this lock after a series of
changes made by previous patches. These mainly include refactoring the
sections that might_sleep() and changing the locking order with the
mmap_lock amongst others.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder_alloc.c | 46 +++++++++++++++++-----------------
 drivers/android/binder_alloc.h | 10 ++++----
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index a3e56637db4f..a4a4dc87ba53 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -169,9 +169,9 @@ struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
 {
 	struct binder_buffer *buffer;
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	buffer = binder_alloc_prepare_to_free_locked(alloc, user_ptr);
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	return buffer;
 }
 
@@ -597,10 +597,10 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	if (!next)
 		return ERR_PTR(-ENOMEM);
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	buffer = binder_alloc_new_buf_locked(alloc, next, size, is_async);
 	if (IS_ERR(buffer)) {
-		mutex_unlock(&alloc->mutex);
+		spin_unlock(&alloc->lock);
 		goto out;
 	}
 
@@ -608,7 +608,7 @@ struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 	buffer->offsets_size = offsets_size;
 	buffer->extra_buffers_size = extra_buffers_size;
 	buffer->pid = current->tgid;
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 
 	ret = binder_install_buffer_pages(alloc, buffer, size);
 	if (ret) {
@@ -785,17 +785,17 @@ void binder_alloc_free_buf(struct binder_alloc *alloc,
 	 * We could eliminate the call to binder_alloc_clear_buf()
 	 * from binder_alloc_deferred_release() by moving this to
 	 * binder_free_buf_locked(). However, that could
-	 * increase contention for the alloc mutex if clear_on_free
-	 * is used frequently for large buffers. The mutex is not
+	 * increase contention for the alloc->lock if clear_on_free
+	 * is used frequently for large buffers. This lock is not
 	 * needed for correctness here.
 	 */
 	if (buffer->clear_on_free) {
 		binder_alloc_clear_buf(alloc, buffer);
 		buffer->clear_on_free = false;
 	}
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	binder_free_buf_locked(alloc, buffer);
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 }
 
 /**
@@ -893,7 +893,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 	struct binder_buffer *buffer;
 
 	buffers = 0;
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	BUG_ON(alloc->vma);
 
 	while ((n = rb_first(&alloc->allocated_buffers))) {
@@ -943,7 +943,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
 		}
 		kfree(alloc->pages);
 	}
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	if (alloc->mm)
 		mmdrop(alloc->mm);
 
@@ -966,7 +966,7 @@ void binder_alloc_print_allocated(struct seq_file *m,
 	struct binder_buffer *buffer;
 	struct rb_node *n;
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	for (n = rb_first(&alloc->allocated_buffers); n; n = rb_next(n)) {
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
 		seq_printf(m, "  buffer %d: %lx size %zd:%zd:%zd %s\n",
@@ -976,7 +976,7 @@ void binder_alloc_print_allocated(struct seq_file *m,
 			   buffer->extra_buffers_size,
 			   buffer->transaction ? "active" : "delivered");
 	}
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 }
 
 /**
@@ -993,7 +993,7 @@ void binder_alloc_print_pages(struct seq_file *m,
 	int lru = 0;
 	int free = 0;
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	/*
 	 * Make sure the binder_alloc is fully initialized, otherwise we might
 	 * read inconsistent state.
@@ -1009,7 +1009,7 @@ void binder_alloc_print_pages(struct seq_file *m,
 				lru++;
 		}
 	}
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	seq_printf(m, "  pages: %d:%d:%d\n", active, lru, free);
 	seq_printf(m, "  pages high watermark: %zu\n", alloc->pages_high);
 }
@@ -1025,10 +1025,10 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc)
 	struct rb_node *n;
 	int count = 0;
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	for (n = rb_first(&alloc->allocated_buffers); n != NULL; n = rb_next(n))
 		count++;
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	return count;
 }
 
@@ -1073,8 +1073,8 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 		goto err_mmget;
 	if (!mmap_read_trylock(mm))
 		goto err_mmap_read_lock_failed;
-	if (!mutex_trylock(&alloc->mutex))
-		goto err_get_alloc_mutex_failed;
+	if (!spin_trylock(&alloc->lock))
+		goto err_get_alloc_lock_failed;
 	if (!page->page_ptr)
 		goto err_page_already_freed;
 
@@ -1093,7 +1093,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	trace_binder_unmap_kernel_end(alloc, index);
 
 	list_lru_isolate(lru, item);
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	spin_unlock(lock);
 
 	if (vma) {
@@ -1113,8 +1113,8 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 
 err_invalid_vma:
 err_page_already_freed:
-	mutex_unlock(&alloc->mutex);
-err_get_alloc_mutex_failed:
+	spin_unlock(&alloc->lock);
+err_get_alloc_lock_failed:
 	mmap_read_unlock(mm);
 err_mmap_read_lock_failed:
 	mmput_async(mm);
@@ -1149,7 +1149,7 @@ void binder_alloc_init(struct binder_alloc *alloc)
 	alloc->pid = current->group_leader->pid;
 	alloc->mm = current->mm;
 	mmgrab(alloc->mm);
-	mutex_init(&alloc->mutex);
+	spin_lock_init(&alloc->lock);
 	INIT_LIST_HEAD(&alloc->buffers);
 }
 
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index a5181916942e..70387234477e 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -9,7 +9,7 @@
 #include <linux/rbtree.h>
 #include <linux/list.h>
 #include <linux/mm.h>
-#include <linux/rtmutex.h>
+#include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/slab.h>
 #include <linux/list_lru.h>
@@ -72,7 +72,7 @@ struct binder_lru_page {
 
 /**
  * struct binder_alloc - per-binder proc state for binder allocator
- * @mutex:              protects binder_alloc fields
+ * @lock:               protects binder_alloc fields
  * @vma:                vm_area_struct passed to mmap_handler
  *                      (invariant after mmap)
  * @mm:                 copy of task->mm (invariant after open)
@@ -96,7 +96,7 @@ struct binder_lru_page {
  * struct binder_buffer objects used to track the user buffers
  */
 struct binder_alloc {
-	struct mutex mutex;
+	spinlock_t lock;
 	struct vm_area_struct *vma;
 	struct mm_struct *mm;
 	unsigned long buffer;
@@ -153,9 +153,9 @@ binder_alloc_get_free_async_space(struct binder_alloc *alloc)
 {
 	size_t free_async_space;
 
-	mutex_lock(&alloc->mutex);
+	spin_lock(&alloc->lock);
 	free_async_space = alloc->free_async_space;
-	mutex_unlock(&alloc->mutex);
+	spin_unlock(&alloc->lock);
 	return free_async_space;
 }
 
-- 
2.43.0.rc2.451.g8631bc7472-goog