Re: [PATCH] binder: make sure fd closes complete

[Martijn Coenen]

On Mon, Aug 30, 2021 at 9:51 PM 'Todd Kjos' via kernel-team
 wrote:
>
> During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object
> cleanup may close 1 or more fds. The close operations are
> completed using the task work mechanism -- which means the thread
> needs to return to userspace or the file object may never be
> dereferenced -- which can lead to hung processes.
>
> Force the binder thread back to userspace if an fd is closed during
> BC_FREE_BUFFER handling.
>
> Signed-off-by: Todd Kjos 
Reviewed-by: Martijn Coenen 

> ---
>  drivers/android/binder.c | 23 +--
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index bcec598b89f2..c2823f0d588f 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1852,6 +1852,7 @@ static void binder_deferred_fd_close(int fd)
>  }
>
>  static void binder_transaction_buffer_release(struct binder_proc *proc,
> + struct binder_thread *thread,
>   struct binder_buffer *buffer,
>   binder_size_t failed_at,
>   bool is_failure)
> @@ -2011,8 +2012,16 @@ static void binder_transaction_buffer_release(struct 
> binder_proc *proc,
> >alloc, , buffer,
> offset, sizeof(fd));
> WARN_ON(err);
> -   if (!err)
> +   if (!err) {
> binder_deferred_fd_close(fd);
> +   /*
> +* Need to make sure the thread goes
> +* back to userspace to complete the
> +* deferred close
> +*/
> +   if (thread)
> +   thread->looper_need_return = 
> true;
> +   }
> }
> } break;
> default:
> @@ -3105,7 +3114,7 @@ static void binder_transaction(struct binder_proc *proc,
>  err_copy_data_failed:
> binder_free_txn_fixups(t);
> trace_binder_transaction_failed_buffer_release(t->buffer);
> -   binder_transaction_buffer_release(target_proc, t->buffer,
> +   binder_transaction_buffer_release(target_proc, NULL, t->buffer,
>   buffer_offset, true);
> if (target_node)
> binder_dec_node_tmpref(target_node);
> @@ -3184,7 +3193,9 @@ static void binder_transaction(struct binder_proc *proc,
>   * Cleanup buffer and free it.
>   */
>  static void
> -binder_free_buf(struct binder_proc *proc, struct binder_buffer *buffer)
> +binder_free_buf(struct binder_proc *proc,
> +   struct binder_thread *thread,
> +   struct binder_buffer *buffer)
>  {
> binder_inner_proc_lock(proc);
> if (buffer->transaction) {
> @@ -3212,7 +3223,7 @@ binder_free_buf(struct binder_proc *proc, struct 
> binder_buffer *buffer)
> binder_node_inner_unlock(buf_node);
> }
> trace_binder_transaction_buffer_release(buffer);
> -   binder_transaction_buffer_release(proc, buffer, 0, false);
> +   binder_transaction_buffer_release(proc, thread, buffer, 0, false);
> binder_alloc_free_buf(>alloc, buffer);
>  }
>
> @@ -3414,7 +3425,7 @@ static int binder_thread_write(struct binder_proc *proc,
>  proc->pid, thread->pid, (u64)data_ptr,
>  buffer->debug_id,
>  buffer->transaction ? "active" : 
> "finished");
> -   binder_free_buf(proc, buffer);
> +   binder_free_buf(proc, thread, buffer);
> break;
> }
>
> @@ -4107,7 +4118,7 @@ static int binder_thread_read(struct binder_proc *proc,
> buffer->transaction = NULL;
> binder_cleanup_transaction(t, "fd fixups failed",
>BR_FAILED_REPLY);
> -   binder_free_buf(proc, buffer);
> +   binder_free_buf(proc, thread, buffer);
> binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
>  "%d:%d %stransaction %d fd fixups failed 
> %d/%d, line %d\n",
>  proc->pid, thread->pid,
> --
> 2.33.0.259.gc128427fd7-goog
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH -next] binder: change error code from postive to negative in binder_transaction

[Martijn Coenen]

Thanks!

On Mon, Oct 26, 2020 at 11:52 AM Zhang Qilong  wrote:
>
> Depending on the context, the error return value
> here (extra_buffers_size < added_size) should be
> negative.
>
> Signed-off-by: Zhang Qilong 
Acked-by: Martijn Coenen 

> ---
>  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 b5117576792b..8bbfb9124fa2 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3103,7 +3103,7 @@ static void binder_transaction(struct binder_proc *proc,
> if (extra_buffers_size < added_size) {
> /* integer overflow of extra_buffers_size */
> return_error = BR_FAILED_REPLY;
> -   return_error_param = EINVAL;
> +   return_error_param = -EINVAL;
> return_error_line = __LINE__;
> goto err_bad_extra_size;
> }
> --
> 2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 -next] binder: simplify the return expression of binder_mmap

[Martijn Coenen]

Thanks!

On Tue, Sep 29, 2020 at 3:30 AM Liu Shixin  wrote:
>
> Simplify the return expression.
>
> Signed-off-by: Liu Shixin 

Acked-by: Martijn Coenen 

> ---
> v3: Add the change description.
> v2: Get rid of the "ret" and "failure string" variables.
> v1: Simplify the return expression.
> ---
>  drivers/android/binder.c | 18 --
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 37a505c41dec..49c0700816a5 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5180,9 +5180,7 @@ static const struct vm_operations_struct binder_vm_ops 
> = {
>
>  static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
> -   int ret;
> struct binder_proc *proc = filp->private_data;
> -   const char *failure_string;
>
> if (proc->tsk != current->group_leader)
> return -EINVAL;
> @@ -5194,9 +5192,9 @@ static int binder_mmap(struct file *filp, struct 
> vm_area_struct *vma)
>  (unsigned long)pgprot_val(vma->vm_page_prot));
>
> if (vma->vm_flags & FORBIDDEN_MMAP_FLAGS) {
> -   ret = -EPERM;
> -   failure_string = "bad vm_flags";
> -   goto err_bad_arg;
> +   pr_err("%s: %d %lx-%lx %s failed %d\n", __func__,
> +  proc->pid, vma->vm_start, vma->vm_end, "bad vm_flags", 
> -EPERM);
> +   return -EPERM;
> }
> vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
> vma->vm_flags &= ~VM_MAYWRITE;
> @@ -5204,15 +5202,7 @@ static int binder_mmap(struct file *filp, struct 
> vm_area_struct *vma)
> vma->vm_ops = _vm_ops;
> vma->vm_private_data = proc;
>
> -   ret = binder_alloc_mmap_handler(>alloc, vma);
> -   if (ret)
> -   return ret;
> -   return 0;
> -
> -err_bad_arg:
> -   pr_err("%s: %d %lx-%lx %s failed %d\n", __func__,
> -  proc->pid, vma->vm_start, vma->vm_end, failure_string, ret);
> -   return ret;
> +   return binder_alloc_mmap_handler(>alloc, vma);
>  }
>
>  static int binder_open(struct inode *nodp, struct file *filp)
> --
> 2.25.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3] binder: print warnings when detecting oneway spamming.

[Martijn Coenen]

The most common cause of the binder transaction buffer filling up is a
client rapidly firing oneway transactions into a process, before it has
a chance to handle them. Yet the root cause of this is often hard to
debug, because either the system or the app will stop, and by that time
binder debug information we dump in bugreports is no longer relevant.

This change warns as soon as a process dips below 80% of its oneway
space (less than 100kB available in the configuration), when any one
process is responsible for either more than 50 transactions, or more
than 50% of the oneway space.

Signed-off-by: Martijn Coenen 
---
v2: fixed call-site in binder_alloc_selftest

v3: include size of struct binder_buffer in calculation, fix comments

 drivers/android/binder.c|  2 +-
 drivers/android/binder_alloc.c  | 55 +++--
 drivers/android/binder_alloc.h  |  5 ++-
 drivers/android/binder_alloc_selftest.c |  2 +-
 4 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..946332bc871a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3136,7 +3136,7 @@ static void binder_transaction(struct binder_proc *proc,
 
t->buffer = binder_alloc_new_buf(_proc->alloc, tr->data_size,
tr->offsets_size, extra_buffers_size,
-   !reply && (t->flags & TF_ONE_WAY));
+   !reply && (t->flags & TF_ONE_WAY), current->tgid);
if (IS_ERR(t->buffer)) {
/*
 * -ESRCH indicates VMA cleared. The target is dying.
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 69609696a843..b5b6c9cf1b0b 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -338,12 +338,50 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
return vma;
 }
 
+static void debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
+{
+   /*
+* Find the amount and size of buffers allocated by the current caller;
+* The idea is that once we cross the threshold, whoever is responsible
+* for the low async space is likely to try to send another async txn,
+* 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 = alloc->free_buffers.rb_node;
+   struct binder_buffer *buffer;
+   size_t total_alloc_size = 0;
+   size_t num_buffers = 0;
+
+   for (n = rb_first(>allocated_buffers); n != NULL;
+n = rb_next(n)) {
+   buffer = rb_entry(n, struct binder_buffer, rb_node);
+   if (buffer->pid != pid)
+   continue;
+   if (!buffer->async_transaction)
+   continue;
+   total_alloc_size += binder_alloc_buffer_size(alloc, buffer)
+   + sizeof(struct binder_buffer);
+   num_buffers++;
+   }
+
+   /*
+* Warn if this pid has more than 50 transactions, or more than 50% of
+* async space (which is 25% of total buffer size).
+*/
+   if (num_buffers > 50 || total_alloc_size > alloc->buffer_size / 4) {
+   binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+"%d: pid %d spamming oneway? %zd buffers allocated 
for a total size of %zd\n",
+ alloc->pid, pid, num_buffers, total_alloc_size);
+   }
+}
+
 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,
-   int is_async)
+   int is_async,
+   int pid)
 {
struct rb_node *n = alloc->free_buffers.rb_node;
struct binder_buffer *buffer;
@@ -486,11 +524,20 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
buffer->offsets_size = offsets_size;
buffer->async_transaction = is_async;
buffer->extra_buffers_size = extra_buffers_size;
+   buffer->pid = pid;
if (is_async) {
alloc->free_async_space -= size + sizeof(struct binder_buffer);
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 th

[PATCH v2] ANDROID: binder: print warnings when detecting oneway spamming.

[Martijn Coenen]

The most common cause of the binder transaction buffer filling up is a
client rapidly firing oneway transactions into a process, before it has
a chance to handle them. Yet the root cause of this is often hard to
debug, because either the system or the app will stop, and by that time
binder debug information we dump in bugreports is no longer relevant.

This change warns as soon as a process dips below 80% of its oneway
space (less than 100kB available in the configuration), when any one
process is responsible for either more than 50 transactions, or more
than 50% of the oneway space.

Signed-off-by: Martijn Coenen 
---
v2: fixed call-site in binder_alloc_selftest

 drivers/android/binder.c|  2 +-
 drivers/android/binder_alloc.c  | 49 +++--
 drivers/android/binder_alloc.h  |  5 ++-
 drivers/android/binder_alloc_selftest.c |  2 +-
 4 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..946332bc871a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3136,7 +3136,7 @@ static void binder_transaction(struct binder_proc *proc,
 
t->buffer = binder_alloc_new_buf(_proc->alloc, tr->data_size,
tr->offsets_size, extra_buffers_size,
-   !reply && (t->flags & TF_ONE_WAY));
+   !reply && (t->flags & TF_ONE_WAY), current->tgid);
if (IS_ERR(t->buffer)) {
/*
 * -ESRCH indicates VMA cleared. The target is dying.
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 69609696a843..76e8e633dbd4 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -338,12 +338,48 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
return vma;
 }
 
+static void debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
+{
+   /*
+* Find the amount and size of buffers allocated by the current caller;
+* The idea is that once we cross the threshold, whoever is responsible
+* for the low async space is likely to try to send another async txn,
+* 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 = alloc->free_buffers.rb_node;
+   struct binder_buffer *buffer;
+   size_t buffer_size;
+   size_t total_alloc_size = 0;
+   size_t num_buffers = 0;
+
+   for (n = rb_first(>allocated_buffers); n != NULL;
+n = rb_next(n)) {
+   buffer = rb_entry(n, struct binder_buffer, rb_node);
+   if (buffer->pid != pid)
+   continue;
+   if (!buffer->async_transaction)
+   continue;
+   buffer_size = binder_alloc_buffer_size(alloc, buffer);
+   total_alloc_size += buffer_size;
+   num_buffers++;
+   }
+
+   // Warn if this pid has more than 50% of async space, or more than 50 
txns
+   if (num_buffers > 50 || total_alloc_size > alloc->buffer_size / 4) {
+   binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+"%d: pid %d spamming oneway? %zd buffers allocated 
for a total size of %zd\n",
+ alloc->pid, pid, num_buffers, total_alloc_size);
+   }
+}
+
 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,
-   int is_async)
+   int is_async,
+   int pid)
 {
struct rb_node *n = alloc->free_buffers.rb_node;
struct binder_buffer *buffer;
@@ -486,11 +522,16 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
buffer->offsets_size = offsets_size;
buffer->async_transaction = is_async;
buffer->extra_buffers_size = extra_buffers_size;
+   buffer->pid = pid;
if (is_async) {
alloc->free_async_space -= size + sizeof(struct binder_buffer);
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 reach 80% of async 
space used
+   debug_low_async_space_locked(alloc, pid);
+   }
}
return buffer;
 
@@ -508,6 +549,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
 

Re: [PATCH] binder: print warnings when detecting oneway spamming.

[Martijn Coenen]

On Thu, Aug 20, 2020 at 12:41 PM kernel test robot  wrote:
>
> Hi Martijn,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on staging/staging-testing]
> [also build test ERROR on v5.9-rc1 next-20200820]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:
> https://github.com/0day-ci/linux/commits/Martijn-Coenen/binder-print-warnings-when-detecting-oneway-spamming/20200820-155358
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 
> bc752d2f345bf55d71b3422a6a24890ea03168dc
> config: s390-randconfig-r002-20200818 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 
> 4deda57106f7c9b982a49cb907c33e3966c8de7f)
> reproduce (this is a W=1 build):
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install s390 cross compiling tool for clang build
> # apt-get install binutils-s390x-linux-gnu
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot 
>
> All errors (new ones prefixed by >>):
>
> >> drivers/android/binder_alloc_selftest.c:122:61: error: too few arguments 
> >> to function call, expected 6, have 5
>buffers[i] = binder_alloc_new_buf(alloc, sizes[i], 0, 0, 
> 0);

missed this call-site, will send v2.

Martijn
>  ^
>drivers/android/binder_alloc.h:118:30: note: 'binder_alloc_new_buf' 
> declared here
>extern struct binder_buffer *binder_alloc_new_buf(struct binder_alloc 
> *alloc,
> ^
>1 error generated.
>
> # 
> https://github.com/0day-ci/linux/commit/9d0b269f4468d6793f6fd76a410fdde39dbf6ac2
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review 
> Martijn-Coenen/binder-print-warnings-when-detecting-oneway-spamming/20200820-155358
> git checkout 9d0b269f4468d6793f6fd76a410fdde39dbf6ac2
> vim +122 drivers/android/binder_alloc_selftest.c
>
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  114
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  115  static void 
> binder_selftest_alloc_buf(struct binder_alloc *alloc,
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  116
> struct binder_buffer *buffers[],
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  117
> size_t *sizes, int *seq)
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  118  {
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  119  int i;
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  120
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  121  for (i = 0; i < BUFFER_NUM; 
> i++) {
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 @122  buffers[i] = 
> binder_alloc_new_buf(alloc, sizes[i], 0, 0, 0);
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  123  if 
> (IS_ERR(buffers[i]) ||
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  124  
> !check_buffer_pages_allocated(alloc, buffers[i],
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  125
> sizes[i])) {
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  126  
> pr_err_size_seq(sizes, seq);
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  127  
> binder_selftest_failures++;
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  128  }
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  129  }
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  130  }
> 4175e2b46fd4b9 Sherry Yang 2017-08-23  131
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] binder: print warnings when detecting oneway spamming.

[Martijn Coenen]

The most common cause of the binder transaction buffer filling up is a
client rapidly firing oneway transactions into a process, before it has
a chance to handle them. Yet the root cause of this is often hard to
debug, because either the system or the app will stop, and by that time
binder debug information we dump in bugreports is no longer relevant.

This change warns as soon as a process dips below 80% of its oneway
space (less than 100kB available in the configuration), when any one
process is responsible for either more than 50 transactions, or more
than 50% of the oneway space.

Signed-off-by: Martijn Coenen 
---
 drivers/android/binder.c   |  2 +-
 drivers/android/binder_alloc.c | 49 +++---
 drivers/android/binder_alloc.h |  5 +++-
 3 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..946332bc871a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3136,7 +3136,7 @@ static void binder_transaction(struct binder_proc *proc,
 
t->buffer = binder_alloc_new_buf(_proc->alloc, tr->data_size,
tr->offsets_size, extra_buffers_size,
-   !reply && (t->flags & TF_ONE_WAY));
+   !reply && (t->flags & TF_ONE_WAY), current->tgid);
if (IS_ERR(t->buffer)) {
/*
 * -ESRCH indicates VMA cleared. The target is dying.
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 69609696a843..76e8e633dbd4 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -338,12 +338,48 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
return vma;
 }
 
+static void debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
+{
+   /*
+* Find the amount and size of buffers allocated by the current caller;
+* The idea is that once we cross the threshold, whoever is responsible
+* for the low async space is likely to try to send another async txn,
+* 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 = alloc->free_buffers.rb_node;
+   struct binder_buffer *buffer;
+   size_t buffer_size;
+   size_t total_alloc_size = 0;
+   size_t num_buffers = 0;
+
+   for (n = rb_first(>allocated_buffers); n != NULL;
+n = rb_next(n)) {
+   buffer = rb_entry(n, struct binder_buffer, rb_node);
+   if (buffer->pid != pid)
+   continue;
+   if (!buffer->async_transaction)
+   continue;
+   buffer_size = binder_alloc_buffer_size(alloc, buffer);
+   total_alloc_size += buffer_size;
+   num_buffers++;
+   }
+
+   // Warn if this pid has more than 50% of async space, or more than 50 
txns
+   if (num_buffers > 50 || total_alloc_size > alloc->buffer_size / 4) {
+   binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+"%d: pid %d spamming oneway? %zd buffers allocated 
for a total size of %zd\n",
+ alloc->pid, pid, num_buffers, total_alloc_size);
+   }
+}
+
 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,
-   int is_async)
+   int is_async,
+   int pid)
 {
struct rb_node *n = alloc->free_buffers.rb_node;
struct binder_buffer *buffer;
@@ -486,11 +522,16 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
buffer->offsets_size = offsets_size;
buffer->async_transaction = is_async;
buffer->extra_buffers_size = extra_buffers_size;
+   buffer->pid = pid;
if (is_async) {
alloc->free_async_space -= size + sizeof(struct binder_buffer);
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 reach 80% of async 
space used
+   debug_low_async_space_locked(alloc, pid);
+   }
}
return buffer;
 
@@ -508,6 +549,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
  * @offsets_size:   user specified buffer offset
  * @extra_buffers_size: size of extra space for meta-d

Re: [PATCH] binder: Modify comments

[Martijn Coenen]

On Tue, Aug 18, 2020 at 3:34 AM hui yang  wrote:
>
> From: YangHui 
>
> The function name should is binder_alloc_new_buf()
>
Reviewed-by: Martijn Coenen 

> Signed-off-by: YangHui 
> ---
>  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 6960969..8c98d12 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -652,7 +652,7 @@ static void binder_free_buf_locked(struct binder_alloc 
> *alloc,
>   * @alloc: binder_alloc for this proc
>   * @buffer:kernel pointer to buffer
>   *
> - * Free the buffer allocated via binder_alloc_new_buffer()
> + * Free the buffer allocated via binder_alloc_new_buf()
>   */
>  void binder_alloc_free_buf(struct binder_alloc *alloc,
> struct binder_buffer *buffer)
> --
> 2.7.4
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0

[Martijn Coenen]

Thanks a lot for the detailed explanation, I understood now.

Martijn

On Tue, Jul 28, 2020 at 4:50 PM Jann Horn  wrote:
>
> On Tue, Jul 28, 2020 at 3:50 PM Martijn Coenen  wrote:
> > On Mon, Jul 27, 2020 at 2:04 PM Jann Horn  wrote:
> > >  - task B opens /dev/binder once, creating binder_proc instance P3
> > >  - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
> > >transaction)
> > >  - P2 receives the handle and uses it to call P3 (two-way transaction)
> > >  - P3 calls P2 (via magic handle 0) (two-way transaction)
> > >  - P2 calls P2 (via handle 1) (two-way transaction)
> >
> > Why do you need P3 involved at all? Could P2 just straight away make a
> > call on handle 1?
>
> Yes, it could, I think. IIRC these steps are necessary if you want to
> actually get a KASAN splat, instead of just a transaction-to-self with
> no further consequences. It has been a while since I looked at this,
> but I'll try to figure out again what was going on...
>
>
> A UAF occurs in the following code due to the transaction-to-self,
> because the "if (t->to_thread == thread)" is tricked into believing
> that the transaction has already been accepted.
>
> static int binder_thread_release(struct binder_proc *proc,
>  struct binder_thread *thread)
> {
> struct binder_transaction *t;
> struct binder_transaction *send_reply = NULL;
> [...]
> t = thread->transaction_stack;
> if (t) {
> [...]
> if (t->to_thread == thread)
> send_reply = t;
> } else {
> [...]
> }
> [...]
> //NOTE: transaction is freed here because top-of-stack is
> //  wrongly treated as already-accepted incoming transaction)
> if (send_reply)
> binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> //NOTE pending transaction work is processed here (transaction has not
> // yet been accepted)
> binder_release_work(proc, >todo);
> [...]
> }
>
> An unaccepted transaction will only have a non-NULL ->to_thread if the
> transaction has a specific target thread; for a non-reply transaction,
> that is only the case if it is a two-way transaction that was sent
> while the binder call stack already contained the target task (iow,
> the transaction looks like a synchronous callback invocation).
>
> With the steps:
>
>  - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
>transaction)
>  - P2 receives the handle and uses it to call P3 (two-way transaction)
>  - P3 calls P2 (via magic handle 0) (two-way transaction)
>  - P2 calls P2 (via handle 1) (two-way transaction)
>
> the call stack will look like this:
>
> P3 -> P2 -> P3 -> P2 -> P2
>
> The initial call from P3 to P2 was IIRC just to give P2 a handle to
> P3; IIRC the relevant part of the call stack was:
>
> P2 -> P3 -> P2 -> P2
>
> Because P2 already occurs down in the call stack, the final
> transaction "P2 -> P2" is considered to be a callback and is
> thread-directed.
>
>
> The reason why P3 has to be created from task B is the "if
> (target_node && target_proc->pid == proc->pid)" check for transactions
> to reference 0.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] binder: Prevent context manager from incrementing ref 0

[Martijn Coenen]

Thanks Jann, the change LGTM, one question on the repro scenario that
wasn't immediately obvious to me:

On Mon, Jul 27, 2020 at 2:04 PM Jann Horn  wrote:
>  - task B opens /dev/binder once, creating binder_proc instance P3
>  - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way
>transaction)
>  - P2 receives the handle and uses it to call P3 (two-way transaction)
>  - P3 calls P2 (via magic handle 0) (two-way transaction)
>  - P2 calls P2 (via handle 1) (two-way transaction)

Why do you need P3 involved at all? Could P2 just straight away make a
call on handle 1?

>
> And then, if P2 does *NOT* accept the incoming transaction work, but
> instead closes the binder fd, we get a crash.
>
> Solve it by preventing the context manager from using ACQUIRE on ref 0.
> There shouldn't be any legitimate reason for the context manager to do
> that.
>
> Additionally, print a warning if someone manages to find another way to
> trigger a transaction-to-self bug in the future.
>
> Cc: sta...@vger.kernel.org
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Acked-by: Todd Kjos 
Reviewed-by: Martijn Coenen 

> Signed-off-by: Jann Horn 
> ---
> fixed that broken binder_user_error() from the first version...
> I sent v1 while I had a dirty tree containing the missing fix. whoops.
>
>  drivers/android/binder.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f50c5f182bb5..5b310eea9e52 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2982,6 +2982,12 @@ static void binder_transaction(struct binder_proc 
> *proc,
> goto err_dead_binder;
> }
> e->to_node = target_node->debug_id;
> +   if (WARN_ON(proc == target_proc)) {
> +   return_error = BR_FAILED_REPLY;
> +   return_error_param = -EINVAL;
> +   return_error_line = __LINE__;
> +   goto err_invalid_target_handle;
> +   }
> if (security_binder_transaction(proc->tsk,
> target_proc->tsk) < 0) {
> return_error = BR_FAILED_REPLY;
> @@ -3635,10 +3641,17 @@ static int binder_thread_write(struct binder_proc 
> *proc,
> struct binder_node *ctx_mgr_node;
> mutex_lock(>context_mgr_node_lock);
> ctx_mgr_node = 
> context->binder_context_mgr_node;
> -   if (ctx_mgr_node)
> +   if (ctx_mgr_node) {
> +   if (ctx_mgr_node->proc == proc) {
> +   binder_user_error("%d:%d 
> context manager tried to acquire desc 0\n",
> + proc->pid, 
> thread->pid);
> +   
> mutex_unlock(>context_mgr_node_lock);
> +   return -EINVAL;
> +   }
> ret = binder_inc_ref_for_node(
> proc, ctx_mgr_node,
> strong, NULL, );
> +   }
> mutex_unlock(>context_mgr_node_lock);
> }
> if (ret)
>
> base-commit: 2a89b99f580371b86ae9bafd6cbeccd3bfab524a
> --
> 2.28.0.rc0.142.g3c755180ce-goog
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] binder: Set end of SG buffer area properly.

[Martijn Coenen]

In case the target node requests a security context, the
extra_buffers_size is increased with the size of the security context.
But, that size is not available for use by regular scatter-gather
buffers; make sure the ending of that buffer is marked correctly.

Acked-by: Todd Kjos 
Fixes: ec74136ded79 ("binder: create node flag to request sender's
security context")
Signed-off-by: Martijn Coenen 
Cc: sta...@vger.kernel.org # 5.1+
---
 drivers/android/binder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 38a59a630cd4c..5bde08603fbc2 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3239,7 +3239,8 @@ static void binder_transaction(struct binder_proc *proc,
buffer_offset = off_start_offset;
off_end_offset = off_start_offset + tr->offsets_size;
sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));
-   sg_buf_end_offset = sg_buf_offset + extra_buffers_size;
+   sg_buf_end_offset = sg_buf_offset + extra_buffers_size -
+   ALIGN(secctx_sz, sizeof(u64));
off_min = 0;
for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
 buffer_offset += sizeof(binder_size_t)) {
-- 
2.22.0.410.gd8fdbe21b5-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

[Martijn Coenen]

This allows the context manager to retrieve information about nodes
that it holds a reference to, such as the current number of
references to those nodes.

Such information can for example be used to determine whether the
servicemanager is the only process holding a reference to a node.
This information can then be passed on to the process holding the
node, which can in turn decide whether it wants to shut down to
reduce resource usage.

Signed-off-by: Martijn Coenen 
---
v2: made sure reserved fields are aligned, and enforce caller zeroes
all fields except handle, as suggested by Dan Carpenter.

 drivers/android/binder.c| 55 +
 include/uapi/linux/android/binder.h | 10 ++
 2 files changed, 65 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d58763b6b0090..5b25412e15ccf 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4544,6 +4544,42 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
return ret;
 }
 
+static int binder_ioctl_get_node_info_for_ref(struct binder_proc *proc,
+   struct binder_node_info_for_ref *info)
+{
+   struct binder_node *node;
+   struct binder_context *context = proc->context;
+   __u32 handle = info->handle;
+
+   if (info->strong_count || info->weak_count || info->reserved1 ||
+   info->reserved2 || info->reserved3) {
+   binder_user_error("%d BINDER_GET_NODE_INFO_FOR_REF: only handle 
may be non-zero.",
+ proc->pid);
+   return -EINVAL;
+   }
+
+   /* This ioctl may only be used by the context manager */
+   mutex_lock(>context_mgr_node_lock);
+   if (!context->binder_context_mgr_node ||
+   context->binder_context_mgr_node->proc != proc) {
+   mutex_unlock(>context_mgr_node_lock);
+   return -EPERM;
+   }
+   mutex_unlock(>context_mgr_node_lock);
+
+   node = binder_get_node_from_ref(proc, handle, true, NULL);
+   if (!node)
+   return -EINVAL;
+
+   info->strong_count = node->local_strong_refs +
+   node->internal_strong_refs;
+   info->weak_count = node->local_weak_refs;
+
+   binder_put_node(node);
+
+   return 0;
+}
+
 static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
struct binder_node_debug_info *info)
 {
@@ -4638,6 +4674,25 @@ static long binder_ioctl(struct file *filp, unsigned int 
cmd, unsigned long arg)
}
break;
}
+   case BINDER_GET_NODE_INFO_FOR_REF: {
+   struct binder_node_info_for_ref info;
+
+   if (copy_from_user(, ubuf, sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+
+   ret = binder_ioctl_get_node_info_for_ref(proc, );
+   if (ret < 0)
+   goto err;
+
+   if (copy_to_user(ubuf, , sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+
+   break;
+   }
case BINDER_GET_NODE_DEBUG_INFO: {
struct binder_node_debug_info info;
 
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/binder.h
index bfaec6903b8bc..b9ba520f7e4bb 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -200,6 +200,15 @@ struct binder_node_debug_info {
__u32has_weak_ref;
 };
 
+struct binder_node_info_for_ref {
+   __u32handle;
+   __u32strong_count;
+   __u32weak_count;
+   __u32reserved1;
+   __u32reserved2;
+   __u32reserved3;
+};
+
 #define BINDER_WRITE_READ  _IOWR('b', 1, struct binder_write_read)
 #define BINDER_SET_IDLE_TIMEOUT_IOW('b', 3, __s64)
 #define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
@@ -208,6 +217,7 @@ struct binder_node_debug_info {
 #define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
 #define BINDER_VERSION _IOWR('b', 9, struct binder_version)
 #define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct 
binder_node_debug_info)
+#define BINDER_GET_NODE_INFO_FOR_REF   _IOWR('b', 12, struct 
binder_node_info_for_ref)
 
 /*
  * NOTE: Two special error codes you should check for when calling
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: Latelimit binder_debug().

[Martijn Coenen]

> Any progress on this problem?

A patch was recently submitted to address this:

https://lkml.org/lkml/2018/8/7/802

>
>>
>>> Without disabling by default or latelimit printk(), the system shall become 
>>> unusable.
>>>
>>> $ grep binder: log | wc -l
>>> 13214
>>> $ head log
>>> [ 1167.389978] binder: 15631:15631 got reply transaction with no 
>>> transaction stack
>>> [ 1167.391813] binder: 15629:15629 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1167.399282] binder: 15631:15631 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1167.399548] binder: undelivered TRANSACTION_ERROR: 29201
>>> [ 1167.419645] binder: 15625:15625 got reply transaction with no 
>>> transaction stack
>>> [ 1167.427800] binder: 15625:15625 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1167.461506] binder: 15634:15634 got reply transaction with no 
>>> transaction stack
>>> [ 1167.469060] binder: 15634:15634 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1167.472747] binder: 15638:15638 got reply transaction with no 
>>> transaction stack
>>> [ 1167.477550] binder: 15633:15633 got reply transaction with no 
>>> transaction stack
>>> $ tail log
>>> [ 1291.131046] binder: 25566:25566 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1291.140761] binder: 25553:25553 got reply transaction with no 
>>> transaction stack
>>> [ 1291.151698] binder: 25553:25553 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1291.182362] binder: 25568:25568 got reply transaction with no 
>>> transaction stack
>>> [ 1291.183361] binder: undelivered TRANSACTION_ERROR: 29201
>>> [ 1291.189926] binder: 25568:25568 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1291.204438] binder: 25569:25569 got reply transaction with no 
>>> transaction stack
>>> [ 1291.211953] binder: 25569:25569 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>> [ 1291.213825] binder: 25572:25572 got reply transaction with no 
>>> transaction stack
>>> [ 1291.227018] binder: 25572:25572 transaction failed 29201/-71, size 0-8 
>>> line 2759
>>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

[Martijn Coenen]

On Wed, Sep 5, 2018 at 11:09 AM, Dan Carpenter  wrote:
> What's the reserved for?  On 64 bit systems there is a 4 byte struct
> hole between weak_count and reserved.

There's many more pieces of information that we hold for a node. While
we don't have a use for most of that now, we may want some of it in
the future, and so I thought it would be wise to reserve some space
here so we don't need a new ioctl when that happens. I'm actually not
sure it's common to do things this way.

> Why not just make reserved a
> __u32 and get rid of the hole?  (Not rhetorical, I have no idea).

Because I thought 8 bytes of reserved space would be nice :-) But you
have a good point re:alignment, I should make it two __u32's then.

>
> Btw, people sometimes complain about that we don't check that user input
> is zeroed in ioctls.  Like for example maybe they're passing random data
> in the the strong_count field and then later we decide that actually
> that field should mean something but we can't make it mean anything
> because we've been letting the user put whatever they want there.  These
> are just random thoughts in my head, not necessarily important.

That's a good point, I will change the code to check for that.

Thanks,
Martijn

>
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: Add BINDER_GET_NODE_INFO_FOR_REF ioctl.

[Martijn Coenen]

This allows the context manager to retrieve information about nodes
that it holds a reference to, such as the current number of
references to those nodes.

Such information can for example be used to determine whether the
servicemanager is the only process holding a reference to a node.
This information can then be passed on to the process holding the
node, which can in turn decide whether it wants to shut down to
reduce resource usage.

Signed-off-by: Martijn Coenen 
---
 drivers/android/binder.c| 50 +
 include/uapi/linux/android/binder.h |  8 +
 2 files changed, 58 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d58763b6b0090..1c7e965241fb8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4544,6 +4544,37 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
return ret;
 }
 
+static int binder_ioctl_get_node_info_for_ref(struct binder_proc *proc,
+   struct binder_node_info_for_ref *info)
+{
+   struct binder_node *node;
+   struct binder_context *context = proc->context;
+   __u32 handle = info->handle;
+
+   memset(info, 0, sizeof(*info));
+
+   /* This ioctl may only be used by the context manager */
+   mutex_lock(>context_mgr_node_lock);
+   if (!context->binder_context_mgr_node ||
+   context->binder_context_mgr_node->proc != proc) {
+   mutex_unlock(>context_mgr_node_lock);
+   return -EPERM;
+   }
+   mutex_unlock(>context_mgr_node_lock);
+
+   node = binder_get_node_from_ref(proc, handle, true, NULL);
+   if (!node)
+   return -EINVAL;
+
+   info->strong_count = node->local_strong_refs +
+   node->internal_strong_refs;
+   info->weak_count = node->local_weak_refs;
+
+   binder_put_node(node);
+
+   return 0;
+}
+
 static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
struct binder_node_debug_info *info)
 {
@@ -4638,6 +4669,25 @@ static long binder_ioctl(struct file *filp, unsigned int 
cmd, unsigned long arg)
}
break;
}
+   case BINDER_GET_NODE_INFO_FOR_REF: {
+   struct binder_node_info_for_ref info;
+
+   if (copy_from_user(, ubuf, sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+
+   ret = binder_ioctl_get_node_info_for_ref(proc, );
+   if (ret < 0)
+   goto err;
+
+   if (copy_to_user(ubuf, , sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+
+   break;
+   }
case BINDER_GET_NODE_DEBUG_INFO: {
struct binder_node_debug_info info;
 
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/binder.h
index bfaec6903b8bc..a54a680ff2936 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -200,6 +200,13 @@ struct binder_node_debug_info {
__u32has_weak_ref;
 };
 
+struct binder_node_info_for_ref {
+   __u32handle;
+   __u32strong_count;
+   __u32weak_count;
+   __u64reserved;
+};
+
 #define BINDER_WRITE_READ  _IOWR('b', 1, struct binder_write_read)
 #define BINDER_SET_IDLE_TIMEOUT_IOW('b', 3, __s64)
 #define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
@@ -208,6 +215,7 @@ struct binder_node_debug_info {
 #define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
 #define BINDER_VERSION _IOWR('b', 9, struct binder_version)
 #define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct 
binder_node_debug_info)
+#define BINDER_GET_NODE_INFO_FOR_REF   _IOWR('b', 12, struct 
binder_node_info_for_ref)
 
 /*
  * NOTE: Two special error codes you should check for when calling
-- 
2.19.0.rc1.350.ge57e33dbd1-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: KASAN: null-ptr-deref Write in binder_update_page_range

[Martijn Coenen]

Thanks Minchan!

On Thu, Aug 23, 2018 at 7:29 AM, Minchan Kim  wrote:
> Signed-off-by: Todd Kjos 
> Signed-off-by: Minchan Kim 
Reviewed-by: Martijn Coenen 

> ---
>  drivers/android/binder_alloc.c | 43 +++---
>  1 file changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 3f3b7b253445..64fd96eada31 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -332,6 +332,35 @@ static int binder_update_page_range(struct binder_alloc 
> *alloc, int allocate,
> return vma ? -ENOMEM : -ESRCH;
>  }
>
> +
> +static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
> +   struct vm_area_struct *vma)
> +{
> +   if (vma)
> +   alloc->vma_vm_mm = vma->vm_mm;
> +   /*
> +* If we see alloc->vma is not NULL, buffer data structures set up
> +* completely. Look at smp_rmb side binder_alloc_get_vma.
> +* We also want to guarantee new alloc->vma_vm_mm is always visible
> +* if alloc->vma is set.
> +*/
> +   smp_wmb();
> +   alloc->vma = vma;
> +}
> +
> +static inline struct vm_area_struct *binder_alloc_get_vma(
> +   struct binder_alloc *alloc)
> +{
> +   struct vm_area_struct *vma = NULL;
> +
> +   if (alloc->vma) {
> +   /* Look at description in binder_alloc_set_vma */
> +   smp_rmb();
> +   vma = alloc->vma;
> +   }
> +   return vma;
> +}
> +
>  static struct binder_buffer *binder_alloc_new_buf_locked(
> struct binder_alloc *alloc,
> size_t data_size,
> @@ -348,7 +377,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
> size_t size, data_offsets_size;
> int ret;
>
> -   if (alloc->vma == NULL) {
> +   if (!binder_alloc_get_vma(alloc)) {
> binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
>"%d: binder_alloc_buf, no vma\n",
>alloc->pid);
> @@ -723,9 +752,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
> buffer->free = 1;
> binder_insert_free_buffer(alloc, buffer);
> alloc->free_async_space = alloc->buffer_size / 2;
> -   barrier();
> -   alloc->vma = vma;
> -   alloc->vma_vm_mm = vma->vm_mm;
> +   binder_alloc_set_vma(alloc, vma);
> mmgrab(alloc->vma_vm_mm);
>
> return 0;
> @@ -754,10 +781,10 @@ void binder_alloc_deferred_release(struct binder_alloc 
> *alloc)
> int buffers, page_count;
> struct binder_buffer *buffer;
>
> -   BUG_ON(alloc->vma);
> -
> buffers = 0;
> mutex_lock(>mutex);
> +   BUG_ON(alloc->vma);
> +
> while ((n = rb_first(>allocated_buffers))) {
> buffer = rb_entry(n, struct binder_buffer, rb_node);
>
> @@ -900,7 +927,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc 
> *alloc)
>   */
>  void binder_alloc_vma_close(struct binder_alloc *alloc)
>  {
> -   WRITE_ONCE(alloc->vma, NULL);
> +   binder_alloc_set_vma(alloc, NULL);
>  }
>
>  /**
> @@ -935,7 +962,7 @@ enum lru_status binder_alloc_free_page(struct list_head 
> *item,
>
> index = page - alloc->pages;
> page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE;
> -   vma = alloc->vma;
> +   vma = binder_alloc_get_vma(alloc);
> if (vma) {
> if (!mmget_not_zero(alloc->vma_vm_mm))
> goto err_mmget;
> --
> 2.18.0.1017.ga543ac7ca45-goog
>
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: no outgoing transaction when thread todo has transaction

[Martijn Coenen]

Sherry, this was found by syzkaller, right? In that case, can you add
the  tag so the issue gets closed automatically when this
gets merged?


On Tue, Aug 14, 2018 at 2:28 AM, Sherry Yang  wrote:
> When a process dies, failed reply is sent to the sender of any transaction
> queued on a dead thread's todo list. The sender asserts that the
> received failed reply corresponds to the head of the transaction stack.
> This assert can fail if the dead thread is allowed to send outgoing
> transactions when there is already a transaction on its todo list,
> because this new transaction can end up on the transaction stack of the
> original sender. The following steps illustrate how this assertion can
> fail.
>
> 1. Thread1 sends txn19 to Thread2
>(T1->transaction_stack=txn19, T2->todo+=txn19)
> 2. Without processing todo list, Thread2 sends txn20 to Thread1
>(T1->todo+=txn20, T2->transaction_stack=txn20)
> 3. T1 processes txn20 on its todo list
>(T1->transaction_stack=txn20->txn19, T1->todo=)
> 4. T2 dies, T2->todo cleanup attempts to send failed reply for txn19, but
>T1->transaction_stack points to txn20 -- assertion failes
>
> Step 2. is the incorrect behavior. When there is a transaction on a
> thread's todo list, this thread should not be able to send any outgoing
> synchronous transactions. Only the head of the todo list needs to be
> checked because only threads that are waiting for proc work can directly
> receive work from another thread, and no work is allowed to be queued
> on such a thread without waking up the thread. This patch also enforces
> that a thread is not waiting for proc work when a work is directly
> enqueued to its todo list.
>
> Acked-by: Arve Hjønnevåg 
> Signed-off-by: Sherry Yang 

Reviewed-by: Martijn Coenen 

Thanks,
Martijn

> ---
>  drivers/android/binder.c | 44 +---
>  1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index d58763b6b009..f2081934eb8b 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -822,6 +822,7 @@ static void
>  binder_enqueue_deferred_thread_work_ilocked(struct binder_thread *thread,
> struct binder_work *work)
>  {
> +   WARN_ON(!list_empty(>waiting_thread_node));
> binder_enqueue_work_ilocked(work, >todo);
>  }
>
> @@ -839,6 +840,7 @@ static void
>  binder_enqueue_thread_work_ilocked(struct binder_thread *thread,
>struct binder_work *work)
>  {
> +   WARN_ON(!list_empty(>waiting_thread_node));
> binder_enqueue_work_ilocked(work, >todo);
> thread->process_todo = true;
>  }
> @@ -1270,19 +1272,12 @@ static int binder_inc_node_nilocked(struct 
> binder_node *node, int strong,
> } else
> node->local_strong_refs++;
> if (!node->has_strong_ref && target_list) {
> +   struct binder_thread *thread = 
> container_of(target_list,
> +   struct binder_thread, 
> todo);
> binder_dequeue_work_ilocked(>work);
> -   /*
> -* Note: this function is the only place where we 
> queue
> -* directly to a thread->todo without using the
> -* corresponding binder_enqueue_thread_work() helper
> -* functions; in this case it's ok to not set the
> -* process_todo flag, since we know this node work 
> will
> -* always be followed by other work that starts queue
> -* processing: in case of synchronous transactions, a
> -* BR_REPLY or BR_ERROR; in case of oneway
> -* transactions, a BR_TRANSACTION_COMPLETE.
> -*/
> -   binder_enqueue_work_ilocked(>work, target_list);
> +   BUG_ON(>todo != target_list);
> +   binder_enqueue_deferred_thread_work_ilocked(thread,
> +  
> >work);
> }
> } else {
> if (!internal)
> @@ -2723,6 +2718,7 @@ static void binder_transaction(struct binder_proc *proc,
>  {
> int ret;
> struct binder_transaction *t;
> +   struct binder_work *w;
> struct binder_work *tcomplete;
> binder_size_t *offp, *off_end, *off_start;
> binder_size_t off_min;
> @@ -2864,6 +2860,2

Re: [PATCH] ANDROID: binder: Latelimit binder_debug().

[Martijn Coenen]

On Tue, Jul 10, 2018 at 2:09 PM, Tetsuo Handa
 wrote:
> I don't have benchmark data (I'm not an Android user). But an example log at
> https://syzkaller.appspot.com/text?tag=CrashLog=12f316fc40 got
> about 13214 messages in 124 seconds (over 100 messages per a second).

I meant data for the cond_resched() change, which could have an impact
on IPC latency. But yeah, this is a lot. We typically don't see these
messages on Android userspace, because most of them would be caused be
userspace errors that can't happen when using binder libraries.
Obviously syzkaller doesn't use those. I think we'd like to keep
having these in Android builds, let me discuss internally if we can
just switch the default debug level and enable them in Android through
other means.

> Without disabling by default or latelimit printk(), the system shall become 
> unusable.
>
> $ grep binder: log | wc -l
> 13214
> $ head log
> [ 1167.389978] binder: 15631:15631 got reply transaction with no transaction 
> stack
> [ 1167.391813] binder: 15629:15629 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1167.399282] binder: 15631:15631 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1167.399548] binder: undelivered TRANSACTION_ERROR: 29201
> [ 1167.419645] binder: 15625:15625 got reply transaction with no transaction 
> stack
> [ 1167.427800] binder: 15625:15625 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1167.461506] binder: 15634:15634 got reply transaction with no transaction 
> stack
> [ 1167.469060] binder: 15634:15634 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1167.472747] binder: 15638:15638 got reply transaction with no transaction 
> stack
> [ 1167.477550] binder: 15633:15633 got reply transaction with no transaction 
> stack
> $ tail log
> [ 1291.131046] binder: 25566:25566 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1291.140761] binder: 25553:25553 got reply transaction with no transaction 
> stack
> [ 1291.151698] binder: 25553:25553 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1291.182362] binder: 25568:25568 got reply transaction with no transaction 
> stack
> [ 1291.183361] binder: undelivered TRANSACTION_ERROR: 29201
> [ 1291.189926] binder: 25568:25568 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1291.204438] binder: 25569:25569 got reply transaction with no transaction 
> stack
> [ 1291.211953] binder: 25569:25569 transaction failed 29201/-71, size 0-8 
> line 2759
> [ 1291.213825] binder: 25572:25572 got reply transaction with no transaction 
> stack
> [ 1291.227018] binder: 25572:25572 transaction failed 29201/-71, size 0-8 
> line 2759
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: Latelimit binder_debug().

[Martijn Coenen]

On Mon, Jul 9, 2018 at 3:27 PM, Dmitry Vyukov  wrote:
> I know almost nothing about binder. How these debug messages are
> enabled?  I don't see anything like CONFIG_BINDER_VERBOSE_DEBUG in the
> config:
> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-kasan.config
> Also no mentions of binder in sysctl/cmline.
> All binder maintainers are in CC already, perhaps they can shed some
> light on this.

Some are enabled by default here:
https://github.com/torvalds/linux/blob/master/drivers/android/binder.c#L138

Perhaps we should revise that set then, as it can be quite noisy when
lots of processes are dying (which also happens a lot in syzbot
tests).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: Latelimit binder_debug().

[Martijn Coenen]

On Mon, Jul 9, 2018 at 3:10 AM, Tetsuo Handa
 wrote:
> While at it, let's add cond_resched() to binder_thread_write(),
> binder_transaction() and binder_release_work() loops because they might
> take long time.

This should be a separate patch, and I would love to see some
benchmark data around this change (there's a few in Android userspace
- let me know if you need help setting them up). Did you actually
observe these particular functions running for a long time without the
scheduler running?

Thanks,
Martijn

>
> [1] 
> https://syzkaller.appspot.com/bug?id=0e75779a6f0faac461510c6330514e8f0e893038
> [2] 
> https://syzkaller.appspot.com/bug?id=aa11d2d767f3750ef9a40d156a149e9cfa735b73
>
> Signed-off-by: Tetsuo Handa 
> Reported-by: syzbot+e38306788a2e7102a...@syzkaller.appspotmail.com
> Reported-by: syzbot+4417a2fa149da3802...@syzkaller.appspotmail.com
> ---
>  drivers/android/binder.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 95283f3..c136fce 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -161,7 +161,7 @@ static int binder_set_stop_on_user_error(const char *val,
>  #define binder_debug(mask, x...) \
> do { \
> if (binder_debug_mask & mask) \
> -   pr_info(x); \
> +   pr_info_ratelimited(x); \
> } while (0)
>
>  #define binder_user_error(x...) \
> @@ -3016,7 +3016,7 @@ static void binder_transaction(struct binder_proc *proc,
> sg_bufp = (u8 *)(PTR_ALIGN(off_end, sizeof(void *)));
> sg_buf_end = sg_bufp + extra_buffers_size;
> off_min = 0;
> -   for (; offp < off_end; offp++) {
> +   for (; offp < off_end; cond_resched(), offp++) {
> struct binder_object_header *hdr;
> size_t object_size = binder_validate_object(t->buffer, *offp);
>
> @@ -3307,6 +3307,7 @@ static int binder_thread_write(struct binder_proc *proc,
>
> if (get_user(cmd, (uint32_t __user *)ptr))
> return -EFAULT;
> +   cond_resched();
> ptr += sizeof(uint32_t);
> trace_binder_command(cmd);
> if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
> @@ -4193,6 +4194,7 @@ static void binder_release_work(struct binder_proc 
> *proc,
> struct binder_work *w;
>
> while (1) {
> +   cond_resched();
> w = binder_dequeue_work_head(proc, list);
> if (!w)
> return;
> --
> 1.8.3.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android/vsoc: stop using 'timespec'

[Martijn Coenen]

On Mon, Jun 18, 2018 at 5:09 PM, Arnd Bergmann  wrote:
> The timespec structure suffers from the y2038 overflow and should not
> be used. This changes handle_vsoc_cond_wait() to use ktime_t directly.
>
> Signed-off-by: Arnd Bergmann 

Reviewed-by: Martijn Coenen 

Thanks!

> ---
>  drivers/staging/android/vsoc.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/vsoc.c b/drivers/staging/android/vsoc.c
> index 806beda1040b..22571abcaa4e 100644
> --- a/drivers/staging/android/vsoc.c
> +++ b/drivers/staging/android/vsoc.c
> @@ -405,7 +405,7 @@ static int handle_vsoc_cond_wait(struct file *filp, 
> struct vsoc_cond_wait *arg)
> int ret = 0;
> struct vsoc_device_region *region_p = vsoc_region_from_filep(filp);
> atomic_t *address = NULL;
> -   struct timespec ts;
> +   ktime_t wake_time;
>
> /* Ensure that the offset is aligned */
> if (arg->offset & (sizeof(uint32_t) - 1))
> @@ -433,14 +433,13 @@ static int handle_vsoc_cond_wait(struct file *filp, 
> struct vsoc_cond_wait *arg)
>  * We do things this way to flatten differences between 32 bit
>  * and 64 bit timespecs.
>  */
> -   ts.tv_sec = arg->wake_time_sec;
> -   ts.tv_nsec = arg->wake_time_nsec;
> -
> -   if (!timespec_valid())
> +   if (arg->wake_time_nsec >= NSEC_PER_SEC)
> return -EINVAL;
> +   wake_time = ktime_set(arg->wake_time_sec, 
> arg->wake_time_nsec);
> +
> hrtimer_init_on_stack(>timer, CLOCK_MONOTONIC,
>   HRTIMER_MODE_ABS);
> -   hrtimer_set_expires_range_ns(>timer, 
> timespec_to_ktime(ts),
> +   hrtimer_set_expires_range_ns(>timer, wake_time,
>  current->timer_slack_ns);
>
> hrtimer_init_sleeper(to, current);
> --
> 2.9.0
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2 v2] staging: android: ashmem: Fix mmap size validation

[Martijn Coenen]

On Thu, Jun 21, 2018 at 1:29 AM, Joel Fernandes  wrote:
> Also if you look at the kernel sources, there are dozens of drivers that
> check for correct VMA size in mmap handler and fail if it isn't sized
> correctly.

If that's the case, we should definitely do it this way for ashmem as
well. Since its size is fixed after creation, preventing anyone from
mapping a larger size seems reasonable to me.

Reviewed-by: Martijn Coenen 

>
> thanks!
>
>  - Joel
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: remove 32-bit binder interface.

[Martijn Coenen]

On Mon, May 14, 2018 at 4:00 PM, Geert Uytterhoeven
 wrote:
> Patch sent.

Thanks for the quick turn-around!

>
> BTW, sh also doesn't seem to have 64-bit get_user().
> There may be others.

I checked quickly, nios2 is the only other arch that explicitly
doesn't support it and would result in a build error; some other archs
don't define __get_user, but in that case they just fall back to
raw_copy_from_user().

>
> BTW2, does the Android Binder need to care about endianness when talking
> to userspace?

No, I don't think it should.

Thanks,
Martijn

>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] ANDROID: binder: remove 32-bit binder interface.

[Martijn Coenen]

From: Martijn Coenen <m...@google.com>

New devices launching with Android P need to use the 64-bit
binder interface, even on 32-bit SoCs [0].

This change removes the Kconfig option to select the 32-bit
binder interface. We don't think this will affect existing
userspace for the following reasons:
1) The latest Android common tree is 4.14, so we don't
   believe any Android devices are on kernels >4.14.
2) Android devices launch on an LTS release and stick with
   it, so we wouldn't expect devices running on <= 4.14 now
   to upgrade to 4.17 or later. But even if they did, they'd
   rebuild the world (kernel + userspace) anyway.
3) Other userspaces like 'anbox' are already using the
   64-bit interface.

Note that this change doesn't remove the 32-bit UAPI
itself; the reason for that is that Android userspace
always uses the latest UAPI headers from upstream, and
userspace retains 32-bit support for devices that are
upgrading. This will be removed as well in 2-3 years,
at which point we can remove the code from the UAPI
as well.

Finally, this change introduces build errors on archs where
64-bit get_user/put_user is not supported, so make binder
unavailable on m68k (which wouldn't want it anyway).

[0]: https://android-review.googlesource.com/c/platform/build/+/595193

Signed-off-by: Martijn Coenen <m...@android.com>
---
Changes in v2:
  - Dropped support for m68k to avoid build errors.

 drivers/android/Kconfig  | 15 +--
 drivers/android/binder.c |  4 
 2 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
index 7dce3795b887..ee4880bfdcdc 100644
--- a/drivers/android/Kconfig
+++ b/drivers/android/Kconfig
@@ -10,7 +10,7 @@ if ANDROID
 
 config ANDROID_BINDER_IPC
bool "Android Binder IPC Driver"
-   depends on MMU
+   depends on MMU && !M68K
default n
---help---
  Binder is used in Android for both communication between processes,
@@ -32,19 +32,6 @@ config ANDROID_BINDER_DEVICES
  created. Each binder device has its own context manager, and is
  therefore logically separated from the other devices.
 
-config ANDROID_BINDER_IPC_32BIT
-   bool "Use old (Android 4.4 and earlier) 32-bit binder API"
-   depends on !64BIT && ANDROID_BINDER_IPC
-   default y
-   ---help---
- The Binder API has been changed to support both 32 and 64bit
- applications in a mixed environment.
-
- Enable this to support an old 32-bit Android user-space (v4.4 and
- earlier).
-
- Note that enabling this will break newer Android user-space.
-
 config ANDROID_BINDER_IPC_SELFTEST
bool "Android Binder IPC Driver Selftest"
depends on ANDROID_BINDER_IPC
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e578eee31589..2ee9fb02dfb8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -72,10 +72,6 @@
 #include 
 #include 
 
-#ifdef CONFIG_ANDROID_BINDER_IPC_32BIT
-#define BINDER_IPC_32BIT 1
-#endif
-
 #include 
 #include "binder_alloc.h"
 #include "binder_trace.h"
-- 
2.17.0.441.gb46fe60e1d-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: remove 32-bit binder interface.

[Martijn Coenen]

On Fri, May 11, 2018 at 10:08 AM, Greg KH  wrote:
> I think using !CONFIG_M68K is a good start.  We can blacklist any other
> arch that doesn't support this, and that list should be small as I doubt
> any new ones will be added without this support.

Thanks, I will send a v2.

>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: remove 32-bit binder interface.

[Martijn Coenen]

On Sat, May 5, 2018 at 2:10 PM, kbuild test robot  wrote:
>drivers/android/binder.o: In function `binder_thread_write':
>>> binder.c:(.text+0x6a16): undefined reference to `__get_user_bad'

Looks like m68k doesn't support 64-bit get_user(). I could just have
binder depend on !CONFIG_M68K, but there may be other architectures
still that don't support this. Another alternative would be to
whitelist the architectures Android supports - eg arm, arm64, x86,
x86_64. But I'm not sure if arch-limited drivers are considered bad
form. Does anybody have suggestions for how to deal with this?

Thanks,
Martijn

>binder.c:(.text+0x6c9a): undefined reference to `__get_user_bad'
>binder.c:(.text+0x701e): undefined reference to `__get_user_bad'
>binder.c:(.text+0x7414): undefined reference to `__get_user_bad'
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: Fix a possible data race in binder_alloc_mmap_handler

[Martijn Coenen]

On Tue, May 8, 2018 at 2:06 AM, Jia-Ju Bai  wrote:
> The write operations to "alloc->buffer" are protected by
> the lock on line 679 and 730, but the read operation to
> this data on line 712 is not protected by the lock.
> Thus, there may exist a data race for "alloc->buffer".

It's read by the same thread that just wrote it, there is no data race.

The locks at line 679 and 730 protect against multiple threads calling
mmap() at the same time.

>
> To fix this data race, the read operation to "alloc->buffer"
> should be also protected by the lock.
>
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/android/binder_alloc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 5a426c877dfb..596acc3a84e4 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -709,7 +709,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
> goto err_alloc_buf_struct_failed;
> }
>
> +   mutex_lock(_alloc_mmap_lock);
> buffer->data = alloc->buffer;
> +   mutex_unlock(_alloc_mmap_lock);
> list_add(>entry, >buffers);
> buffer->free = 1;
> binder_insert_free_buffer(alloc, buffer);
> --
> 2.17.0
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: remove 32-bit binder interface.

[Martijn Coenen]

New devices launching with Android P need to use the 64-bit
binder interface, even on 32-bit SoCs [0].

This change removes the Kconfig option to select the 32-bit
binder interface. We don't think this will affect existing
userspace for the following reasons:
1) The latest Android common tree is 4.14, so we don't
   believe any Android devices are on kernels >4.14.
2) Android devices launch on an LTS release and stick with
   it, so we wouldn't expect devices running on <= 4.14 now
   to upgrade to 4.17 or later. But even if they did, they'd
   rebuild the world (kernel + userspace) anyway.
3) Other userspaces like 'anbox' are already using the
   64-bit interface.

Note that this change doesn't remove the 32-bit UAPI
itself; the reason for that is that Android userspace
always uses the latest UAPI headers from upstream, and
userspace retains 32-bit support for devices that are
upgrading. This will be removed as well in 2-3 years,
at which point we can remove the code from the UAPI
as well.

[0]: https://android-review.googlesource.com/c/platform/build/+/595193

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/Kconfig  | 13 -
 drivers/android/binder.c |  4 
 2 files changed, 17 deletions(-)

diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
index 7dce3795b887..432e9ad77070 100644
--- a/drivers/android/Kconfig
+++ b/drivers/android/Kconfig
@@ -32,19 +32,6 @@ config ANDROID_BINDER_DEVICES
  created. Each binder device has its own context manager, and is
  therefore logically separated from the other devices.
 
-config ANDROID_BINDER_IPC_32BIT
-   bool "Use old (Android 4.4 and earlier) 32-bit binder API"
-   depends on !64BIT && ANDROID_BINDER_IPC
-   default y
-   ---help---
- The Binder API has been changed to support both 32 and 64bit
- applications in a mixed environment.
-
- Enable this to support an old 32-bit Android user-space (v4.4 and
- earlier).
-
- Note that enabling this will break newer Android user-space.
-
 config ANDROID_BINDER_IPC_SELFTEST
bool "Android Binder IPC Driver Selftest"
depends on ANDROID_BINDER_IPC
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e578eee31589..2ee9fb02dfb8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -72,10 +72,6 @@
 #include 
 #include 
 
-#ifdef CONFIG_ANDROID_BINDER_IPC_32BIT
-#define BINDER_IPC_32BIT 1
-#endif
-
 #include 
 #include "binder_alloc.h"
 #include "binder_trace.h"
-- 
2.17.0.441.gb46fe60e1d-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support

[Martijn Coenen]

On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez  wrote:
> Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
>
> It would be good for us to hear from Android folks if their current use of
> request_firmware_into_buf() is designed in practice to *never* use the direct
> filesystem firmware loading interface, and always rely instead on the
> fallback mechanism.

It's hard to answer this question for Android in general. As far as I
can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK)
are:
1) We have multiple different paths on our devices where firmware can
be located, and the direct loader only supports one custom path
2) Most of those paths are not mounted by the time the corresponding
drivers are loaded, because pretty much all Android kernels today are
built without module support, and therefore drivers are loaded well
before the firmware partition is mounted
3) I think we use _FALLBACK because doing this with uevents is just
the easiest thing to do; our init code has a firmware helper that
deals with this and searches the paths that we care about

2) will change at some point, because Android is moving towards a
model where device-specific peripheral drivers will be loaded as
modules, and since those modules would likely come from the same
partition as the firmware, it's possible that the direct load would
succeed (depending on whether the custom path is configured there or
not). But I don't think we can rely on the direct loader even in those
cases, unless we could configure it with multiple custom paths.

I have no reason to believe request_firmware_into_buf() is special in
this regard; drivers that depend on it may have their corresponding
firmware in different locations, so just depending on the direct
loader would not be good enough.

>
> Is ptr below
>
> ret = request_firmware_into_buf(_fw, fw_name, dev,
> ptr, phdr->p_filesz);
>
> Also part of the DMA buffer allocated earlier via:
>
> ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
>
> Android folks?

I think the Qualcomm folks owning this (Andy, David, Bjorn, already
cc'd here) are better suited to answer that question.

Thanks,
Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support

[Martijn Coenen]

On Thu, May 3, 2018 at 5:21 PM, Luis R. Rodriguez  wrote:
> Android folks, poke below. otherwise we'll have no option but to seriously
> consider Mimi's patch to prevent these calls when IMA appraisal is enforced:

Sorry, figuring out who's the right person to answer this, will get
back to you ASAP.

Martijn

>
> http://lkml.kernel.org/r/1525182503-13849-7-git-send-email-zo...@linux.vnet.ibm.com
>
> Please read below
>
> On Wed, Apr 25, 2018 at 05:55:57PM +, Luis R. Rodriguez wrote:
>> On Wed, Apr 25, 2018 at 01:00:09AM -0400, Mimi Zohar wrote:
>> > On Tue, 2018-04-24 at 23:42 +, Luis R. Rodriguez wrote:
>> > > On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
>> > > > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
>> > > If its of any help --
>> > >
>> > > drivers/soc/qcom/mdt_loader.c is the only driver currently using
>> > > request_firmware_into_buf() however I'll note qcom_mdt_load() is used in 
>> > > many
>> > > other drivers so they are wrappers around request_firmware_into_buf():
>> > >
>> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c:   * adreno_request_fw() handles 
>> > > this, but qcom_mdt_load() does
>> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c:  ret = qcom_mdt_load(dev, 
>> > > fw, fwname, GPU_PAS_ID,
>> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c:  ret = qcom_mdt_load(dev, 
>> > > fw, newname, GPU_PAS_ID,
>> > > drivers/media/platform/qcom/venus/firmware.c:   ret = qcom_mdt_load(dev, 
>> > > mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
>> > > drivers/remoteproc/qcom_adsp_pil.c: return qcom_mdt_load(adsp->dev, 
>> > > fw, rproc->firmware, adsp->pas_id,
>> > > drivers/remoteproc/qcom_wcnss.c:return qcom_mdt_load(wcnss->dev, 
>> > > fw, rproc->firmware, WCNSS_PAS_ID,
>> > >
>> > > > > As such the current IMA code (from v4.17-rc2) actually does
>> > > > > not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
>> > > >
>> > > > Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
>> > > > should.
>> > > >
>> > > > Depending on whether the device requesting the firmware has access to
>> > > > the DMA memory, before the signature verification,
>> > >
>> > > It would seem from the original patch review about 
>> > > READING_FIRMWARE_PREALLOC_BUFFER
>> > > that this is not a DMA buffer.
>
> To be very clear I believe Stephen implied this was not DMA buffer. Mimi
> asked for READING_FIRMWARE_DMA if it was:
>
> https://patchwork.kernel.org/patch/9074611/
>
>> > The call sequence:
>> > qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent()
>> >
>> > If dma_alloc_coherent() isn't allocating a DMA buffer, then the
>> > function name is misleading/confusing.
>>
>> Hah, by *definition* the device *and* processor has immediate access
>> to data written *immediately* when dma_alloc_coherent() is used. From
>> Documentation/DMA-API.txt:
>>
>> ---
>> Part Ia - Using large DMA-coherent buffers
>> --
>>
>> ::
>>
>> void *
>> dma_alloc_coherent(struct device *dev, size_t size,
>>dma_addr_t *dma_handle, gfp_t flag)
>>
>> Consistent memory is memory for which a write by either the device or
>> the processor can immediately be read by the processor or device
>> without having to worry about caching effects.  (You may however need
>> to make sure to flush the processor's write buffers before telling
>> devices to read that memory.)
>> 
>>
>> Is ptr below
>>
>>   ret = request_firmware_into_buf(_fw, fw_name, dev,
>>   ptr, phdr->p_filesz);
>>
>> Also part of the DMA buffer allocated earlier via:
>>
>> ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
>>
>> Android folks?
>
> Android folks?
>
>> > > The device driver should have access to the buffer pointer with write 
>> > > given
>> > > that with request_firmware_into_buf() the driver is giving full write 
>> > > access to
>> > > the memory pointer so that the firmware API can stuff the firmware it 
>> > > finds
>> > > there.
>> > >
>> > > Firmware signature verification would be up to the device hardware to do 
>> > > upon
>> > > load *after* request_firmware_into_buf().
>> >
>> > We're discussing the kernel's signature verification, not the device
>> > hardware's signature verification.  Can the device driver access the
>> > buffer, before IMA-appraisal has verified the firmware's signature?
>>
>> It will depend on the above question.
>
>   Luis
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: fix binder work return error is wrongly consumed

[Martijn Coenen]

On Wed, May 2, 2018 at 7:30 AM,   wrote:
> But there is potential risks in the future, future functional extensions
> need to consider nesting issues, maybe extending more methods where we
> push to thread->todo. I think that using queueing return error transaction
> to the head of thread todo list is more appropriate, as follows:

Historically it was not safe to issue binder transactions from death
recipients because of issues like this - though I don't think we've
ever been clear about that in the documentation. "ANDROID: binder:
don't enqueue
death notifications to thread todo" fixes that, and makes it safe to
do - though the current driver may still have an issue if two death
receipts are queued (need to look into that). If we ever were to add
functionality into the driver again that queues something to
thread->todo that could result in nesting, we would need to do more
than just this change to make it safe. Consider this scenario, which
would fail even with your patch:

1) BR_DEAD_BINDER is in thread->todo
2) We issue a new transaction T1 to a different process, which succeeds
3) We queue the reply BR_REPLY for T1 to thread->todo and return to userspace
4) userspace finds BR_DEAD_BINDER first, runs a death recipient which
issues a new binder transaction, T2
5) We find the BR_REPLY for T1 instead of T2, and now we have a problem

So, this fix by itself won't make it safe, so I think instead we
should always prevent such nesting in the driver. That also keeps the
code simpler and easier to understand - just queue things in order.

Thanks,
Martijn

>
> 1) During a transaction, the client will add BINDER_WORK_RETURN_ERROR into
> thread->todo list, however, it would be better to pick up
> BINDER_WORK_RETURN_ERROR firstly and finish the transaction immediately,
> jump out of the nest.
>
> 2) Client pick up the left binder work from thread->todo, using the
> same thread, do not need wake up other idle binder thread.
>
> 3) It works fine in the old binder version (before split big binder
> lock), binder_transaction only set thread return_error when target
> process has dead, do not add BINDER_WORK_RETURN_ERROR into thread->todo,
> but binder_thread_read() check return_error firstly. If occurs return_error,
> finish this transaction and back to userspace immediately.
>
> So I prefer to put the BINDER_WORK_RETURN_ERROR to the head of the
> queue, same as the old version of binder driver, once and for all.
>
> Signed-off-by: yuanhuihui 
> ---
>  drivers/android/binder.c | 59 
> ++--
>  1 file changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 4eab5be3d00f..1ed1809b8769 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -861,6 +861,60 @@ binder_enqueue_thread_work(struct binder_thread *thread,
> binder_inner_proc_unlock(thread->proc);
>  }
>
> +/**
> + * binder_enqueue_work_head_ilocked() - Add an item to the head of work list
> + * @work: struct binder_work to add to list
> + * @target_list:  list to add work to
> + *
> + * Adds the work to the specified list. Asserts that work
> + * is not already on a list.
> + *
> + * Requires the proc->inner_lock to be held.
> + */
> +static void
> +binder_enqueue_work_head_ilocked(struct binder_work *work,
> +  struct list_head *target_list)
> +{
> +   BUG_ON(target_list == NULL);
> +   BUG_ON(work->entry.next && !list_empty(>entry));
> +   list_add(>entry, target_list);
> +}
> +
> +/**
> + * binder_enqueue_thread_work_head_ilocked() - Add an item to the head of 
> thread work list
> + * @thread:   thread to queue work to
> + * @work: struct binder_work to add to list
> + *
> + * Adds the work to the head of thread todo list, and enables processing
> + * of the todo queue.
> + *
> + * Requires the proc->inner_lock to be held.
> + */
> +static void
> +binder_enqueue_thread_work_head_ilocked(struct binder_thread *thread,
> +  struct binder_work *work)
> +{
> +   binder_enqueue_work_head_ilocked(work, >todo);
> +   thread->process_todo = true;
> +}
> +
> +/**
> + * binder_enqueue_thread_work_head() - Add an item to the head of thread 
> work list
> + * @thread:   thread to queue work to
> + * @work: struct binder_work to add to list
> + *
> + * Adds the work to the head of thread todo list, and enables processing
> + * of the todo queue.
> + */
> +static void
> +binder_enqueue_thread_work_head(struct binder_thread *thread,
> +  struct binder_work *work)
> +{
> +   binder_inner_proc_lock(thread->proc);
> +   binder_enqueue_thread_work_head_ilocked(thread, work);
> +   binder_inner_proc_unlock(thread->proc);
> +}
> +
>  static void
>  binder_dequeue_work_ilocked(struct binder_work *work)
>  {
> @@ -3287,11 +3341,11 @@ static void 

Re: KASAN: use-after-free Read in binder_release_work

[Martijn Coenen]

On Mon, Apr 23, 2018 at 12:17 PM, Dmitry Vyukov  wrote:
> syzbot does not extract this info from patch emails.

Ok so IIUC, Reported-By tags will only be considered when they are
actually part of commits in one of the tested trees - makes sense. So
does sending "#syz fix: xyz" cause syzbot to look inside all the trees
it analyzes for xyz and mark it as closed if found? Does it look
immediately or on some schedule, and does it retry? In this case, I
think my patch wasn't in any tree yet when you sent "#syz fix", only
in Greg's queue (Greg actually pushed it half an hour after your
message). Just want to make sure I do the right thing next time.

Thanks,
Martijn


> First of all, it's not possible to discover them all.
> Second, a mailed patch does not mean committed patch. v2 can be resent
> and potentially change title too.
>
> syzbot takes this info from commits in the tree it tests. It probably
> could extract some emails from the commit. But they can come months
> later, so their value will be questionable. Also consider that 2
> commits in different trees mention the same bug. syzbot generally
> overwrites old info with new info, because that's the only way to fix
> up things. Now this can lead to infinite stream of emails saying that
> this commit fixes this bug, no that commit fixes this bug, no this
> commit fixes this bug, etc.
> Also consider that a bug is first marked as fixed with some commit,
> bug later is marked as dup of another or re-marked as fixed with
> another commit. You won't get a notification, because the whole
> sequence looks reasonable.
> This can also lead to problems when commits backported to
> android/chromeos trees that syzbot also tests. There these fix tags
> look plain bogus because they reference upstream bug, not
> android/chromeos bugs.
>
> By default we try to keep syzbot silent and non-spammy. And we do not
> seem to have lots of such cases where things are somewhat messed. And
> in all cases it should come to eventual consistency. If something is
> marked as fixed prematurely, syzbot will open another bug. If
> something is not marked as fixed (or marked as fixed with a
> non-existent commit), then these bugs still hang on the dashboard and
> visible.
>
>
 Thanks,
 Martijn

> Now syzbot already skips list_del frame and takes the next one, so it
> should become slightly better.
>
> Let's close this one with the binder fix (since that one was closed
> with an rdma fix):
>
> #syz fix: ANDROID: binder: prevent transactions into own process.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: KASAN: use-after-free Read in binder_release_work

[Martijn Coenen]

On Mon, Apr 23, 2018 at 11:49 AM, Dmitry Vyukov  wrote:
> Since it's already in Greg's queue, it's not worth bothering. We can
> fix up things here with these "#syz fix" tags in emails, which
> associate fixes with bugs.

I meant, when I sent the original patch a month or so ago, could
syzbot have replied saying "The reported-by tag you used belongs to a
bug that was already marked as closed by this other commit?".

>
>
>> Thanks,
>> Martijn
>>
>>> Now syzbot already skips list_del frame and takes the next one, so it
>>> should become slightly better.
>>>
>>> Let's close this one with the binder fix (since that one was closed
>>> with an rdma fix):
>>>
>>> #syz fix: ANDROID: binder: prevent transactions into own process.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: KASAN: use-after-free Read in binder_release_work

[Martijn Coenen]

On Mon, Apr 23, 2018 at 11:28 AM, Dmitry Vyukov  wrote:
> https://syzkaller.appspot.com/bug?extid=09e05aba06723a94d43d
> and that happened in binder. But then syzkaller found a reproducer for
> it, but it turned out to be in rdma subsystem. It's generally not
> possible to properly distinguish different bugs that look similar, and
> if syzbot does more sensitive bug classification, then it will also
> inevitably report more duplicates. So that bug was closed as an rdma
> bug.

Thanks for the clarification! It looks like I sent the patch with the
original reported-by tag after it was closed as an rdma issue; would
it help if syzbot sent a reply saying this bug was already marked as
closed with a different commit, or are there other complications with
that?

Thanks,
Martijn

> Now syzbot already skips list_del frame and takes the next one, so it
> should become slightly better.
>
> Let's close this one with the binder fix (since that one was closed
> with an rdma fix):
>
> #syz fix: ANDROID: binder: prevent transactions into own process.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: KASAN: use-after-free Read in binder_release_work

[Martijn Coenen]

On Thu, Apr 19, 2018 at 11:35 PM, Eric Biggers  wrote:
> Martijn, this is going to be fixed by
> https://patchwork.kernel.org/patch/10312345/
> ("ANDROID: binder: prevent transactions into own process"), right?
> The syzbot bug ID in that patch is for a bug that is already closed,
> so if it's not too late you should use this one.

Yeah that should fix it. Why was it closed? I think the syzbot bug ID
I used in that patch was from the original report to LKML. Greg
mentioned the patch was already in his queue.

Thanks,
Martijn

>
> - Eric
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] ANDROID: binder: prevent transactions into own process.

[Martijn Coenen]

On Wed, Mar 28, 2018 at 1:34 PM, Martijn Coenen <m...@android.com> wrote:
> On Wed, Mar 28, 2018 at 1:28 PM, Greg KH <gre...@linuxfoundation.org> wrote:
>> What is different from "v2" you sent before this?  No change information
>> from v1?

Greg, is this in your queue, or should I just send a v3 to clean this
up? See below for my original reply.

Thanks,
Martijn

>
> Sorry I messed this up - the first resend did not have "v2" in the
> subject but did contain v2 change information, the second resend had
> the v2 subject and did not contain the change information :( Either of
> the last two patches is fine - it just used "target_proc" instead of
> "target_node->proc".
>
>>
>> I'm totally confused as to which is the "latest" patch here :(
>>
>> thanks,
>>
>> greg k-h-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: re-order some conditions

[Martijn Coenen]

On Thu, Mar 29, 2018 at 11:14 AM, Dan Carpenter
<dan.carpen...@oracle.com> wrote:
> It doesn't make any difference to runtime but I've switched these two
> checks to make my static checker happy.
>
> The problem is that "buffer->data_size" is user controlled and if it's
> less than "sizeo(*hdr)" then that means "offset" can be more than
> "buffer->data_size".  It's just cleaner to check it in the other order.
>
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
Acked-by: Martijn Coenen <m...@android.com>

>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 764b63a5aade..00322b146469 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2058,8 +2058,8 @@ static size_t binder_validate_object(struct 
> binder_buffer *buffer, u64 offset)
> struct binder_object_header *hdr;
> size_t object_size = 0;
>
> -   if (offset > buffer->data_size - sizeof(*hdr) ||
> -   buffer->data_size < sizeof(*hdr) ||
> +   if (buffer->data_size < sizeof(*hdr) ||
> +   offset > buffer->data_size - sizeof(*hdr) ||
> !IS_ALIGNED(offset, sizeof(u32)))
> return 0;
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] ANDROID: binder: prevent transactions into own process.

[Martijn Coenen]

On Wed, Mar 28, 2018 at 1:28 PM, Greg KH  wrote:
> What is different from "v2" you sent before this?  No change information
> from v1?

Sorry I messed this up - the first resend did not have "v2" in the
subject but did contain v2 change information, the second resend had
the v2 subject and did not contain the change information :( Either of
the last two patches is fine - it just used "target_proc" instead of
"target_node->proc".

>
> I'm totally confused as to which is the "latest" patch here :(
>
> thanks,
>
> greg k-h-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] ANDROID: binder: prevent transactions into own process.

[Martijn Coenen]

This can't happen with normal nodes (because you can't get a ref
to a node you own), but it could happen with the context manager;
to make the behavior consistent with regular nodes, reject
transactions into the context manager by the process owning it.

Reported-by: syzbot+09e05aba06723a94d...@syzkaller.appspotmail.com
Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 764b63a5aade..e578eee31589 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2839,6 +2839,14 @@ static void binder_transaction(struct binder_proc *proc,
else
return_error = BR_DEAD_REPLY;
mutex_unlock(>context_mgr_node_lock);
+   if (target_node && target_proc == proc) {
+   binder_user_error("%d:%d got transaction to 
context manager from process owning it\n",
+ proc->pid, thread->pid);
+   return_error = BR_FAILED_REPLY;
+   return_error_param = -EINVAL;
+   return_error_line = __LINE__;
+   goto err_invalid_target_handle;
+   }
}
if (!target_node) {
/*
-- 
2.17.0.rc1.321.gba9d0f2565-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[v2] ANDROID: binder: prevent transactions into own process.

[Martijn Coenen]

This can't happen with normal nodes (because you can't get a ref
to a node you own), but it could happen with the context manager;
to make the behavior consistent with regular nodes, reject
transactions into the context manager by the process owning it.

Reported-by: syzbot+09e05aba06723a94d...@syzkaller.appspotmail.com
Signed-off-by: Martijn Coenen <m...@android.com>
---
Changed in v2:
   - Use target_proc directly to avoid dereference.

 drivers/android/binder.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 764b63a5aade..e578eee31589 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2839,6 +2839,14 @@ static void binder_transaction(struct binder_proc *proc,
else
return_error = BR_DEAD_REPLY;
mutex_unlock(>context_mgr_node_lock);
+   if (target_node && target_proc == proc) {
+   binder_user_error("%d:%d got transaction to 
context manager from process owning it\n",
+ proc->pid, thread->pid);
+   return_error = BR_FAILED_REPLY;
+   return_error_param = -EINVAL;
+   return_error_line = __LINE__;
+   goto err_invalid_target_handle;
+   }
}
if (!target_node) {
/*
-- 
2.17.0.rc1.321.gba9d0f2565-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: prevent transactions into own process.

[Martijn Coenen]

On Wed, Mar 28, 2018 at 10:19 AM, Greg KH  wrote:
> Does this need to go to older kernels as well?

Yes, this should apply cleanly to 4.14 as well. It won't apply to
pre-4.14 kernels because of the fine-grained locking changes, but the
same issue exists there and I suspect it would cause the same crash.
Do you want me to send a separate patch for pre-4.14?

Also, I'm going to send a v2 for this because it can be a bit cleaner
(avoiding a deref).

Thanks,
Martijn

>
> I have a script that picks up everything the syzbot finds and tries to
> backport them, after they are applied in Linus's tree.  Might as well
> catch things before we have to rely on my script :)
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: prevent transactions into own process.

[Martijn Coenen]

This can't happen with normal nodes (because you can't get a ref
to a node you own), but it could happen with the context manager;
to make the behavior consistent with regular nodes, reject
transactions into the context manager by the process owning it.

Reported-by: syzbot+09e05aba06723a94d...@syzkaller.appspotmail.com
Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e7e4560e4c6e..57d4ba926ed0 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3001,6 +3001,14 @@ static void binder_transaction(struct binder_proc *proc,
else
return_error = BR_DEAD_REPLY;
mutex_unlock(>context_mgr_node_lock);
+   if (target_node && target_node->proc == proc) {
+   binder_user_error("%d:%d got transaction to 
context manager from process owning it\n",
+ proc->pid, thread->pid);
+   return_error = BR_FAILED_REPLY;
+   return_error_param = -EINVAL;
+   return_error_line = __LINE__;
+   goto err_invalid_target_handle;
+   }
}
if (!target_node) {
/*
-- 
2.17.0.rc0.231.g781580f067-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: KASAN: use-after-free Read in __list_del_entry_valid (3)

[Martijn Coenen]

On Tue, Mar 6, 2018 at 9:30 AM, syzbot
 wrote:
> Hello,
>
> syzbot hit the following crash on upstream commit
> 094b58e1040a44f991d7ab628035e69c4d6b79c9 (Mon Mar 5 19:57:06 2018 +)
> Merge tag 'linux-kselftest-4.16-rc5' of
> git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest

I'll take a look at this one,

Martijn

>
> Unfortunately, I don't have any reproducer for this crash yet.
> Raw console output is attached.
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached.
> user-space arch: i386
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+09e05aba06723a94d...@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
>
> binder: release 6174:6185 transaction 4 in, still active
> binder: send failed reply for transaction 4 to 6174:6185
> binder: 6194:6198 ERROR: BC_REGISTER_LOOPER called without request
> ==
> binder: 6198 RLIMIT_NICE not set
> BUG: KASAN: use-after-free in __list_del_entry_valid+0x144/0x150
> lib/list_debug.c:54
> Read of size 8 at addr 8801daede810 by task kworker/1:1/24
>
> CPU: 1 PID: 24 Comm: kworker/1:1 Not tainted 4.16.0-rc4+ #252
> binder: BINDER_SET_CONTEXT_MGR already set
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Workqueue: events binder_deferred_func
> Call Trace:
>  __dump_stack lib/dump_stack.c:17 [inline]
>  dump_stack+0x194/0x24d lib/dump_stack.c:53
> binder: 6194:6206 got new transaction with bad transaction stack,
> transaction 9 has target 6194:0
>  print_address_description+0x73/0x250 mm/kasan/report.c:256
>  kasan_report_error mm/kasan/report.c:354 [inline]
>  kasan_report+0x23c/0x360 mm/kasan/report.c:412
>  __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
>  __list_del_entry_valid+0x144/0x150 lib/list_debug.c:54
>  __list_del_entry include/linux/list.h:117 [inline]
>  list_del_init include/linux/list.h:159 [inline]
>  binder_dequeue_work_head_ilocked drivers/android/binder.c:893 [inline]
>  binder_dequeue_work_head drivers/android/binder.c:913 [inline]
>  binder_release_work+0x163/0x490 drivers/android/binder.c:4191
> binder: 6194:6206 transaction failed 29201/-71, size 0-0 line 2875
> binder: 6191:6205 ioctl 40046207 0 returned -16
>  binder_thread_release+0x4d0/0x720 drivers/android/binder.c:4396
>  binder_deferred_release drivers/android/binder.c:4939 [inline]
>  binder_deferred_func+0x4f4/0x1340 drivers/android/binder.c:5022
> binder: BINDER_SET_CONTEXT_MGR already set
> binder: 6200:6207 ioctl 40046207 0 returned -16
> binder: 6191:6208 ERROR: BC_REGISTER_LOOPER called without request
>  process_one_work+0xc47/0x1bb0 kernel/workqueue.c:2113
> binder: 6208 RLIMIT_NICE not set
> binder: 6200:6212 ERROR: BC_REGISTER_LOOPER called without request
> binder: 6212 RLIMIT_NICE not set
> binder: 6191:6213 got new transaction with bad transaction stack,
> transaction 11 has target 6194:0
>  worker_thread+0x223/0x1990 kernel/workqueue.c:2247
> binder: 6191:6213 transaction failed 29201/-71, size 0-0 line 2875
> binder: 6198 RLIMIT_NICE not set
> binder: release 6200:6207 transaction 14 out, still active
> binder: undelivered TRANSACTION_COMPLETE
>  kthread+0x33c/0x400 kernel/kthread.c:238
>  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:406
>
> Allocated by task 6185:
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>  set_track mm/kasan/kasan.c:459 [inline]
>  kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:552
>  kmem_cache_alloc_trace+0x136/0x740 mm/slab.c:3607
>  kmalloc include/linux/slab.h:512 [inline]
>  kzalloc include/linux/slab.h:701 [inline]
>  binder_transaction+0x13c1/0x81c0 drivers/android/binder.c:2900
>  binder_thread_write+0xb50/0x3840 drivers/android/binder.c:3513
>  binder_ioctl_write_read.isra.38+0x261/0xcb0 drivers/android/binder.c:4451
>  binder_ioctl+0xb72/0x1417 drivers/android/binder.c:4591
>  C_SYSC_ioctl fs/compat_ioctl.c:1461 [inline]
>  compat_SyS_ioctl+0x151/0x2a30 fs/compat_ioctl.c:1407
>  do_syscall_32_irqs_on arch/x86/entry/common.c:330 [inline]
>  do_fast_syscall_32+0x3ec/0xf9f arch/x86/entry/common.c:392
>  entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139
>
> Freed by task 24:
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>  set_track mm/kasan/kasan.c:459 [inline]
>  __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:520
>  kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:527
>  __cache_free mm/slab.c:3485 [inline]
>  kfree+0xd9/0x260 mm/slab.c:3800
>  binder_free_transaction+0x6a/0x90 drivers/android/binder.c:1966
>  binder_send_failed_reply+0x1c9/0x380 drivers/android/binder.c:2005
>  binder_thread_release+0x4bb/0x720 drivers/android/binder.c:4395
>  binder_deferred_release drivers/android/binder.c:4939 [inline]
>  binder_deferred_func+0x4f4/0x1340 

Re: [PATCH] ANDROID: binder: synchronize_rcu() when using POLLFREE.

[Martijn Coenen]

Greg,

This is for 4.14 LTS and 4.16.

Thanks,
Martijn

On Fri, Feb 16, 2018 at 9:47 AM, Martijn Coenen <m...@android.com> wrote:
> To prevent races with ep_remove_waitqueue() removing the
> waitqueue at the same time.
>
> Reported-by: syzbot+a2a3c4909716e2714...@syzkaller.appspotmail.com
> Signed-off-by: Martijn Coenen <m...@android.com>
> ---
>  drivers/android/binder.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 15e3d3c2260d..359220f87e1d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -4376,6 +4376,15 @@ static int binder_thread_release(struct binder_proc 
> *proc,
>
> binder_inner_proc_unlock(thread->proc);
>
> +   /*
> +* This is needed to avoid races between wake_up_poll() above and
> +* and ep_remove_waitqueue() called for other reasons (eg the epoll 
> file
> +* descriptor being closed); ep_remove_waitqueue() holds an RCU read
> +* lock, so we can be sure it's done after calling synchronize_rcu().
> +*/
> +   if (thread->looper & BINDER_LOOPER_STATE_POLL)
> +   synchronize_rcu();
> +
> if (send_reply)
> binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
> binder_release_work(proc, >todo);
> --
> 2.16.1.291.g4437f3f132-goog
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: synchronize_rcu() when using POLLFREE.

[Martijn Coenen]

To prevent races with ep_remove_waitqueue() removing the
waitqueue at the same time.

Reported-by: syzbot+a2a3c4909716e2714...@syzkaller.appspotmail.com
Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 15e3d3c2260d..359220f87e1d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4376,6 +4376,15 @@ static int binder_thread_release(struct binder_proc 
*proc,
 
binder_inner_proc_unlock(thread->proc);
 
+   /*
+* This is needed to avoid races between wake_up_poll() above and
+* and ep_remove_waitqueue() called for other reasons (eg the epoll file
+* descriptor being closed); ep_remove_waitqueue() holds an RCU read
+* lock, so we can be sure it's done after calling synchronize_rcu().
+*/
+   if (thread->looper & BINDER_LOOPER_STATE_POLL)
+   synchronize_rcu();
+
if (send_reply)
binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
binder_release_work(proc, >todo);
-- 
2.16.1.291.g4437f3f132-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: KASAN: use-after-free Read in remove_wait_queue

[Martijn Coenen]

On Mon, Feb 12, 2018 at 7:31 PM, Al Viro  wrote:
> Any chance of bisecting it?

Perhaps my fix introduced another (related) problem, I'm looking into it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: remove waitqueue when thread exits.

[Martijn Coenen]

On Fri, Jan 5, 2018 at 1:20 PM, Greg KH  wrote:
> Should this be a 4.15-final thing, as well as backported to any range of
> older kernels?

This was found by syzkaller and wouldn't be hit in normal code paths,
so I think it's not critical for 4.15. This code was introduced in
4.14, so it should be backported there (let me know if it doesn't
apply cleanly - I think it should).

Thanks,
Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: remove waitqueue when thread exits.

[Martijn Coenen]

binder_poll() passes the thread->wait waitqueue that
can be slept on for work. When a thread that uses
epoll explicitly exits using BINDER_THREAD_EXIT,
the waitqueue is freed, but it is never removed
from the corresponding epoll data structure. When
the process subsequently exits, the epoll cleanup
code tries to access the waitlist, which results in
a use-after-free.

Prevent this by using POLLFREE when the thread exits.

Signed-off-by: Martijn Coenen <m...@android.com>
Reported-by: syzbot <syzkal...@googlegroups.com>
---
 drivers/android/binder.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 778caed570c6..26a66da72f02 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4365,6 +4365,18 @@ static int binder_thread_release(struct binder_proc 
*proc,
if (t)
spin_lock(>lock);
}
+
+   /*
+* If this thread used poll, make sure we remove the waitqueue
+* from any epoll data structures holding it with POLLFREE.
+* waitqueue_active() is safe to use here because we're holding
+* the inner lock.
+*/
+   if ((thread->looper & BINDER_LOOPER_STATE_POLL) &&
+   waitqueue_active(>wait)) {
+   wake_up_poll(>wait, POLLHUP | POLLFREE);
+   }
+
binder_inner_proc_unlock(thread->proc);
 
if (send_reply)
-- 
2.16.0.rc0.223.g4a4ac83678-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: Remove obsolete proc waitqueue.

[Martijn Coenen]

It was no longer being used.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 778caed570c6..06067636 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -493,8 +493,6 @@ enum binder_deferred_state {
  *(protected by @inner_lock)
  * @todo: list of work for this process
  *(protected by @inner_lock)
- * @wait: wait queue head to wait for proc work
- *(invariant after initialized)
  * @stats:per-process binder statistics
  *(atomics, no lock needed)
  * @delivered_death:  list of delivered death notification
@@ -537,7 +535,6 @@ struct binder_proc {
bool is_dead;
 
struct list_head todo;
-   wait_queue_head_t wait;
struct binder_stats stats;
struct list_head delivered_death;
int max_threads;
-- 
2.16.0.rc0.223.g4a4ac83678-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] MAINTAINERS: update Android driver maintainers.

[Martijn Coenen]

Add Todd Kjos and myself, remove Riley (who no
longer works at Google).

Signed-off-by: Martijn Coenen <m...@android.com>
---
Changes in v2: adds commit message.

 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index aa71ab52fd76..da8264fc09d4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -859,7 +859,8 @@ F:  kernel/configs/android*
 ANDROID DRIVERS
 M: Greg Kroah-Hartman <gre...@linuxfoundation.org>
 M: Arve Hjønnevåg <a...@android.com>
-M: Riley Andrews <riandr...@android.com>
+M: Todd Kjos <tk...@android.com>
+M: Martijn Coenen <m...@android.com>
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
 L: de...@driverdev.osuosl.org
 S: Supported
-- 
2.15.0.531.g2ccb3012c9-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] MAINTAINERS: update Android driver maintainers.

[Martijn Coenen]

Signed-off-by: Martijn Coenen <m...@android.com>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index aa71ab52fd76..da8264fc09d4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -859,7 +859,8 @@ F:  kernel/configs/android*
 ANDROID DRIVERS
 M: Greg Kroah-Hartman <gre...@linuxfoundation.org>
 M: Arve Hjønnevåg <a...@android.com>
-M: Riley Andrews <riandr...@android.com>
+M: Todd Kjos <tk...@android.com>
+M: Martijn Coenen <m...@android.com>
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
 L: de...@driverdev.osuosl.org
 S: Supported
-- 
2.15.0.531.g2ccb3012c9-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/6] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

On Thu, Nov 16, 2017 at 4:10 PM, Peter Zijlstra  wrote:
> Well, I go by the one described in all the real-time computing texts;
> also found on Wikipedia FWIW:
>
>   https://en.wikipedia.org/wiki/Priority_inheritance

Guess I was taking inheritance too literally :-)

>
>> This behavior is also related to binder's threadpool - all those
>> threads run at a default prio of 120 (nice 0), and call into the
>> kernel to wait for work. If somebody then makes a call with a lower
>> prio (>120), we do change the binder thread that handles that call to
>> use the same low prio as well. Why run at nice 0 if the caller runs at
>> nice 10?
>
> Not immediately saying it doesn't make sense in the sync-rpc scenario;
> just saying that calling it PI is terribly confusing.
>
>> Yes, in case of asynchronous transactions it's not PI at all.
>
> It might be good to untangle some of that.

Yeah, I could possibly separate the part that deals with synchronous
transactions out, since that is "true PI".

> I'm still struggling with the fact that _binder_ has threads and not the
> apps themselves. Totally weird that.

Just to clarify - the threads do belong to the app; they are spawned
from the app process in userspace (sometimes on request of the
kernel), and call into the kernel driver for work. But, the app itself
is merely responsible for starting the threadpool; the libbinder
userspace library takes care of the rest. Beyond that, apps just
receive incoming calls on one of these threads, and that's it.

> Right; but real-time systems are about guarantees, not about mostly and
> on average.

Agreed. I wouldn't go so far to claim Android is RT, but we'll take
any gains we can get.

> But yes, you touch upon one way to have mixed priority thread pools. You
> need a max prio receiver thread that is basically always runnable, this
> is the one thread doing poll(). It is the thread you can immediately
> assign your rt_mutex ownership to etc..

This is an interesting design, I'll think about if I can make it work
with synchronous transactions. One concern is that running such a
thread at max prio means it would always preempt other work,
regardless of the importance of the incoming binder transaction.
Though I guess if you assign rt_mutex ownership to that thread before
even waking it up, it doesn't need to be max prio - it will be at the
prio it needs to be at.

Also I'm not sure how to make this work with async transactions that
need to be handled at a certain prio; either we already have a thread
lying around running at that prio, but if we don't we need to change
the prio before waking it up (which is more or less what this series
does).

> Sounds really weird to me though; I'd be curious to know why this
> 'feature' was created.

I think it may be because binder tries to abstract away from the fact
that a node can be remote - for userspace a binder object can be both
local and remote, and it shouldn't have to care. If you have a local
function call chain A() -> B() -> C(), then these functions obviously
all run on the same thread. But if A() and C() live in process P1 and
B lives in process P2, having A() and C() run in the same thread in
process P1 preserves those semantics. Arve (on CC) probably knows more
about this.

> I'm taking it changing this stuff is 'difficult' since much of it has
> been directly exposed to apps? And you'll need to build a parallel
> interface and slowly migrate apps away from it?

I think the whole threadpool design is actually abstracted pretty
nicely from apps, but we do have transaction ordering semantics that
we need to preserve; also the binder protocol between the kernel
driver and userspace is UAPI, and it can be hard to make large changes
to it while retaining support for an old Android userspace.

> Yeah I suppose so. Also I think the comments could be made clearer to
> avoid some of the confusion we've had here.

A lot of what we talked about is also about how binder works in
general. I'm writing more documentation about that internally, and I
hope I could some day add Documentation/ipc/binder.txt or something
like that :-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/6] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

On Thu, Nov 16, 2017 at 12:27 PM, Peter Zijlstra  wrote:
>> On Wed, Nov 15, 2017 at 2:01 PM, Peter Zijlstra  wrote:
>> >> + * 1) binder supports a "minimum node priority", meaning that all 
>> >> transactions
>> >> + *into a node must run at this priority at a minimum. This means 
>> >> that the
>> >> + *desired priority for handling a transaction is not necessarily 
>> >> equal to
>> >> + *the priority of the caller.
>> >
>> > Isn't that the same as running everything in that node (whatever that
>> > is) at that given prio?
>>
>> Not for synchronous transactions; in that case it's max(min_node_prio,
>> caller_prio).
>
> But that's exactly what you get!? How would it ever get below
> min_node_prio? PI only ever (temporarily) raises prio, it never lowers
> it.

Ah I guess we have different definitions of inheritance then. This
behavior is also related to binder's threadpool - all those threads
run at a default prio of 120 (nice 0), and call into the kernel to
wait for work. If somebody then makes a call with a lower prio (>120),
we do change the binder thread that handles that call to use the same
low prio as well. Why run at nice 0 if the caller runs at nice 10?

>
>> One reason min_node_prio exists is that for certain
>> critical nodes, we don't want calls into it to run at a very low
>> priority, because the implementation of said calls may take locks that
>> are contended. I realize one possible solution to that is to use PI
>> mutexes in userspace, but Android doesn't support that today.
>
> Fix that?

That's probably easier said than done :) I don't know why we don't
support them, I'll ask around.

>
> I'm confused -- again. Why not run those threads at the min prio to
> begin with?

This also gets back to binder's threadpool; they are generic threads
in a process waiting for work at nice 0. The prio they get depends on
the work they get, and in case of synchronous transactions, who calls
into them. If we had dedicated threads for each node, we could indeed
run them at the min_prio of that node by default. But this is not how
binder works today.

> Also, this doesn't seem related to PI.

Yes, in case of asynchronous transactions it's not PI at all.

> I'm not understanding; are you saying that some app does an async
> request for sensor data, then later this sensor triggers and we need to
> route the data back to the app?

The way this typically works is that an interested app creates a
binder node to receive sensor data on (basically a callback). The app
then passes this node into the Sensor HAL and says "call me back when
you have relevant sensor data". When the Sensor HAL has data, it makes
calls on this node with the relevant data. Because the receiving app
process has a few generic binder threads that may receive any type of
data, not just sensor data, we want to raise the priority only when
handling sensor data, and restore it to the default when the thread is
done.

>
> But above you said that async requests do not boost the priority; so
> then I would expect this sensor's data to land on regular min-prio
> thread for dispatch.

As you found out later in this e-mail, this doesn't work because of
binder's current threadpool design.

> *blink* what? Binder has its own threads? Its not just an RPC/ORB?

Yeah :-)

> That smells like fail; if you need to raise your prio it means you are
> low(er) prio and could at that point be subject to inversion.

True - it does expose a race condition, but it's still better than not
raising the priority at all. I'm looking into selecting a busy thread
and boost that. Though in general we don't run into this very often by
sizing the threadpool large enough.

>
> Assuming no idle threads in the pool, and all 'busy' at lower priority.
> A medium prio something is running, keeping these pool thingies from
> asking for more work. A high prio request comes in.. nobody around to
> service. You loose, game over.

Agree, we're trying to fix that.


> Yeah, that sounds really dodgy. I've been a _long_ time since I read the
> RT CORBA stuff, so I'm not sure if there's a sensible solution to the
> problem, but it sounds really really dodgy.
>
> Regular RPCs are synchronous; if A asks B to do something, A blocks. B
> can then not turn around and ask A something. That is called a deadlock.

In case of binder, this actually works, we call it "recursive calls";
the same thread in A that called into B can receive an incoming call
from B and handle that. Of course you still need to be careful when
that happens - eg if you hold a lock when making the first outbound
call, you better make sure you can't take that same lock on any
inbound call that can result from it. "Don't hold locks over
synchronous binder calls" is a well-adapted Android design paradigm.


> Alternatively, if you consider A and B to be services/nodes and you get
> service A to call into B and then have B call back into A, you need to
> guarantee there are 

Re: [PATCH v3 2/6] ANDROID: binder: add min sched_policy to node.

[Martijn Coenen]

On Wed, Nov 15, 2017 at 2:02 PM, Peter Zijlstra  wrote:
>> Internally, we use the priority map that the kernel
>> uses, e.g. [0..99] for real-time policies and [100..139]
>> for the SCHED_NORMAL/SCHED_BATCH policies.
>
> I will break that without consideration if I have to. That really isn't
> something anybody outside of the core code should rely on.

I mostly used these because it makes it easier to debug, since that
range is used in other places in the kernel (and in trace events). All
priority calculations use things that are in sched headers, like
NICE_TO_PRIO, PRIO_TO_NICE, and MAX_USER_RT_PRIO. So things wouldn't
necessarily break if you just changed the value ranges. If you
inverted the range, that would be a problem...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 4/6] ANDROID: binder: add RT inheritance flag to node.

[Martijn Coenen]

On Wed, Nov 15, 2017 at 2:05 PM, Peter Zijlstra <pet...@infradead.org> wrote:
> On Thu, Oct 26, 2017 at 04:07:48PM +0200, Martijn Coenen wrote:
>> Allows a binder node to specify whether it wants to
>> inherit real-time scheduling policy from a caller. This
>> inheritance may not always be desirable, for example in
>> cases where the binder call runs untrusted and therefore
>> potentially unbounded code.
>
> Isn't that backwards? That is, binder should enforce not inheriting
> across a trust boundary, not let a node opt out voluntary.

That's a good point. In practice on Android, nodes are constructed
with the default policy of "don't inherit", and we have an internal
API (not available to untrusted code) to change it. That said,
somebody could talk directly to the kernel driver and present a node
with the flag set. I'll look into this. That said, I think we want
this change regardless of enforcing across the trust boundary; that
is, we don't want to inherit by default even if the callee is trusted.
In Android O, all of the framework runs with binder PI disabled; it's
mostly the HALs serving latency-critical data that need it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 3/6] ANDROID: binder: improve priority inheritance.

[Martijn Coenen]

On Wed, Nov 15, 2017 at 2:03 PM, Peter Zijlstra <pet...@infradead.org> wrote:
> On Thu, Oct 26, 2017 at 04:07:47PM +0200, Martijn Coenen wrote:
>> By raising the priority of a thread selected for
>> a transaction *before* we wake it up.
>>
>> Delay restoring the priority when doing a reply
>> until after we wake-up the process receiving
>> the reply.
>
> What happens if a thread dies?

Binder threads should never exit, but if they do, the whole process
exits. Anyway in that case we still restore the priority of this
thread - it's in the error path in the "@@ -3328,6 +3369,7 @@"
section.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/6] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

Thanks Peter for looking at this, more inline.

On Wed, Nov 15, 2017 at 2:01 PM, Peter Zijlstra  wrote:
>> + * 1) binder supports a "minimum node priority", meaning that all 
>> transactions
>> + *into a node must run at this priority at a minimum. This means that 
>> the
>> + *desired priority for handling a transaction is not necessarily equal 
>> to
>> + *the priority of the caller.
>
> Isn't that the same as running everything in that node (whatever that
> is) at that given prio?

Not for synchronous transactions; in that case it's max(min_node_prio,
caller_prio). One reason min_node_prio exists is that for certain
critical nodes, we don't want calls into it to run at a very low
priority, because the implementation of said calls may take locks that
are contended. I realize one possible solution to that is to use PI
mutexes in userspace, but Android doesn't support that today.

>
>> + * 2) binder supports asynchronous transactions, where the caller is not 
>> blocked
>> + *on transaction completion; so, it also can't be blocked on an 
>> rt_mutex.
>
> This is not a theoretically sound thing to do... For a system to be
> real-time it not only needs to guarantee forward progress but also have
> bounded runtime.
>
> Elevating another task's priority to your own and then not blocking
> effectively injects time. At the very least this time should be taking
> into consideration when doing runtime analysis of the system.

In case of asynchronous transactions, we never inherit the priority of
the caller. We run at max(default_prio, min_node_prio). For the
default node configuration, this means that we don't change priority
at all for async transactions (they all run at nice 0). But for nodes
that have a minimum priority set, we want calls into the node to run
at that priority instead.

>
> Also, since its async, why do we care? The original task is still making
> progress. Only at the point where we start to wait for completion does
> it make sense to boost. Otherwise you end up having to charge one task
> double time.

Yeah, so this is actually not about the caller; it's about certain
critical nodes that need to process events at a high priority. For
example, consider a node that receives sensor events using async
transactions. It's imperative that the threads handling these events
run at real-time priority from the moment they wake up. Binder's
thread model has a generic threadpool for the entire process; threads
aren't dedicated to specific nodes. So to avoid such a thread being
preempted, we need to raise its priority even before waking it up, and
the only place to do it is from the caller.

>
>> + * 3) similarly, there may not necessarily be a thread waiting for
>> + *transactions at the time the call is made, so we don't know who to 
>> proxy-
>> + *lock the lock for.
>
> Non-issue; you have the exact same problem now. Your interface takes a
> @task argument, so you have to have a @task either way around.

But in my current implementation once a thread does call in for more
work and finds the work pending, it will raise its own priority. I
don't see how this is possible with rt_mutex, because the thread is
not yet available at the moment the caller goes to sleep.

>
>> + * 4) binder supports nested transactions, where A can call into B, and B 
>> can
>> + *call back into A before returning a reply to the original transaction.
>> + *This means that if A is blocked on an rt_mutex B holds, B must first 
>> wake
>> + *up A to handle a new transaction, and only then can it proxy-lock and 
>> try
>> + *to acquire the new rt_mutex. This leaves a race condition where B
>> + *temporarily runs at its original priority.
>
> That doesn't sound like something a well formed (RT) program should do
> in the first place.

What do you mean specifically - nested transactions?

>
> You could play horrible games with lock ownership, but YUCK!!
>
>> + * 5) rt_mutex does not currently support PI for CFS tasks.
>
> Neither do you.. just inheriting the nice value is not correct for a WFQ
> style scheduler.

I don't think we're trying to make this fit perfectly with WFQ - we're
just trying to improve the latency on Android devices where it counts,
by telling the scheduler which threads are important. Without these
patches, we would often see binder threads receiving sensor events get
preempted in the kernel, and the delay would cause issues with things
like video stabilization, etc.

> What you're proposing is a bunch of make-work-ish hacks on a system that
> isn't designed for RT.

What parts of the system aren't designed for RT? I'm trying to
understand if it's something in the way binder currently works - eg
the use of asynchronous transactions and generic threadpools - or
something in this patchset in particular. How should we deal with
high-priority sensor events that go over binder IPC in your opinion?
___
devel 

[PATCH] ANDROID: binder: Add thread->process_todo flag.

[Martijn Coenen]

This flag determines whether the thread should currently
process the work in the thread->todo worklist.

The prime usecase for this is improving the performance
of synchronous transactions: all synchronous transactions
post a BR_TRANSACTION_COMPLETE to the calling thread,
but there's no reason to return that command to userspace
right away - userspace anyway needs to wait for the reply.

Likewise, a synchronous transaction that contains a binder
object can cause a BC_ACQUIRE/BC_INCREFS to be returned to
userspace; since the caller must anyway hold a strong/weak
ref for the duration of the call, postponing these commands
until the reply comes in is not a problem.

Note that this flag is not used to determine whether a
thread can handle process work; a thread should never pick
up process work when thread work is still pending.

Before patch:
--
Benchmark   Time   CPU Iterations
--
BM_sendVec_binderize/4  45959 ns  20288 ns  34351
BM_sendVec_binderize/8  45603 ns  20080 ns  34909
BM_sendVec_binderize/16 45528 ns  20113 ns  34863
BM_sendVec_binderize/32 45551 ns  20122 ns  34881
BM_sendVec_binderize/64 45701 ns  20183 ns  34864
BM_sendVec_binderize/12845824 ns  20250 ns  34576
BM_sendVec_binderize/25645695 ns  20171 ns  34759
BM_sendVec_binderize/51245743 ns  20211 ns  34489
BM_sendVec_binderize/1024   46169 ns  20430 ns  34081

After patch:
--
Benchmark   Time   CPU Iterations
--
BM_sendVec_binderize/4  42939 ns  17262 ns  40653
BM_sendVec_binderize/8  42823 ns  17243 ns  40671
BM_sendVec_binderize/16 42898 ns  17243 ns  40594
BM_sendVec_binderize/32 42838 ns  17267 ns  40527
BM_sendVec_binderize/64 42854 ns  17249 ns  40379
BM_sendVec_binderize/12842881 ns  17288 ns  40427
BM_sendVec_binderize/25642917 ns  17297 ns  40429
BM_sendVec_binderize/51243184 ns  17395 ns  40411
BM_sendVec_binderize/1024   43119 ns  17357 ns  40432

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 151 +--
 1 file changed, 107 insertions(+), 44 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 95a96a254e5d..04d356f27721 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -577,6 +577,8 @@ enum {
  *(protected by @proc->inner_lock)
  * @todo: list of work to do for this thread
  *(protected by @proc->inner_lock)
+ * @process_todo: whether work in @todo should be processed
+ *(protected by @proc->inner_lock)
  * @return_error: transaction errors reported by this thread
  *(only accessed by this thread)
  * @reply_error:  transaction errors reported by target thread
@@ -602,6 +604,7 @@ struct binder_thread {
bool looper_need_return; /* can be written by other thread */
struct binder_transaction *transaction_stack;
struct list_head todo;
+   bool process_todo;
struct binder_error return_error;
struct binder_error reply_error;
wait_queue_head_t wait;
@@ -787,6 +790,16 @@ static bool binder_worklist_empty(struct binder_proc *proc,
return ret;
 }
 
+/**
+ * binder_enqueue_work_ilocked() - Add an item to the work list
+ * @work: struct binder_work to add to list
+ * @target_list:  list to add work to
+ *
+ * Adds the work to the specified list. Asserts that work
+ * is not already on a list.
+ *
+ * Requires the proc->inner_lock to be held.
+ */
 static void
 binder_enqueue_work_ilocked(struct binder_work *work,
   struct list_head *target_list)
@@ -797,22 +810,56 @@ binder_enqueue_work_ilocked(struct binder_work *work,
 }
 
 /**
- * binder_enqueue_work() - Add an item to the work list
- * @proc: binder_proc associated with list
+ * binder_enqueue_deferred_thread_work_ilocked() - Add deferred thread work
+ * @thread:   thread to queue work to
  * @work: struct binder_work to add to list
- * @target_list:  list to add work to
  *
- * Adds the work to the specified list. Asserts that work
- * is not already on a list.
+ * Adds the work to the todo list of the thread. Doesn't set the process_todo
+ * flag, which means that (if it wasn't already set) the thread will go to
+ * sleep without handling this work when it calls read.
+ *
+ * Requires the proc-&

Re: [PATCH] ANDROID: binder: show high watermark of alloc->pages.

[Martijn Coenen]

On Mon, Nov 13, 2017 at 10:49 AM, Greg KH  wrote:
> Who can use this?  A userspace tool?  Or something else?

The output is part of Android bugreports, it's not parsed
automatically but the information is very useful even to manually look
at. Since Treble, we have more processes using binder, and some 32-bit
Android targets run out of vmalloc space because every process using
binder allocates 1MB in vmalloc address space. 1MB is overkill for
many processes, and this helps us find out which processes that are.

> This is ok for 4.15-rc1, right?

Yes.

Thanks,
Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] ANDROID: binder: fix transaction leak.

[Martijn Coenen]

On Mon, Nov 13, 2017 at 10:49 AM, Greg KH  wrote:
> Is this relevant for 4.14 and any older kernels as well?

The problem was introduced with fine-grained locking, which is 4.14 and up only.

Thanks,
Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: show high watermark of alloc->pages.

[Martijn Coenen]

Show the high watermark of the index into the alloc->pages
array, to facilitate sizing the buffer on a per-process
basis.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder_alloc.c | 4 
 drivers/android/binder_alloc.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 6f6f745605af..0dba2308125c 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -281,6 +281,9 @@ static int binder_update_page_range(struct binder_alloc 
*alloc, int allocate,
goto err_vm_insert_page_failed;
}
 
+   if (index + 1 > alloc->pages_high)
+   alloc->pages_high = index + 1;
+
trace_binder_alloc_page_end(alloc, index);
/* vm_insert_page does not seem to increment the refcount */
}
@@ -853,6 +856,7 @@ void binder_alloc_print_pages(struct seq_file *m,
}
mutex_unlock(>mutex);
seq_printf(m, "  pages: %d:%d:%d\n", active, lru, free);
+   seq_printf(m, "  pages high watermark: %zu\n", alloc->pages_high);
 }
 
 /**
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 2dd33b6df104..0b145307f1fd 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -92,6 +92,7 @@ struct binder_lru_page {
  * @pages:  array of binder_lru_page
  * @buffer_size:size of address space specified via mmap
  * @pid:pid for associated binder_proc (invariant after init)
+ * @pages_high: high watermark of offset in @pages
  *
  * Bookkeeping structure for per-proc address space management for binder
  * buffers. It is normally initialized during binder_init() and binder_mmap()
@@ -112,6 +113,7 @@ struct binder_alloc {
size_t buffer_size;
uint32_t buffer_free;
int pid;
+   size_t pages_high;
 };
 
 #ifdef CONFIG_ANDROID_BINDER_IPC_SELFTEST
-- 
2.15.0.448.gf294e3d99a-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: fix transaction leak.

[Martijn Coenen]

If a call to put_user() fails, we failed to
properly free a transaction and send a failed
reply (if necessary).

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 40 +++-
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 95a96a254e5d..e947b09607ed 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1947,6 +1947,26 @@ static void binder_send_failed_reply(struct 
binder_transaction *t,
}
 }
 
+/**
+ * binder_cleanup_transaction() - cleans up undelivered transaction
+ * @t: transaction that needs to be cleaned up
+ * @reason:reason the transaction wasn't delivered
+ * @error_code:error to return to caller (if synchronous call)
+ */
+static void binder_cleanup_transaction(struct binder_transaction *t,
+  const char *reason,
+  uint32_t error_code)
+{
+   if (t->buffer->target_node && !(t->flags & TF_ONE_WAY)) {
+   binder_send_failed_reply(t, error_code);
+   } else {
+   binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+   "undelivered transaction %d, %s\n",
+   t->debug_id, reason);
+   binder_free_transaction(t);
+   }
+}
+
 /**
  * binder_validate_object() - checks for a valid metadata object in a buffer.
  * @buffer:binder_buffer that we're parsing.
@@ -4015,12 +4035,20 @@ static int binder_thread_read(struct binder_proc *proc,
if (put_user(cmd, (uint32_t __user *)ptr)) {
if (t_from)
binder_thread_dec_tmpref(t_from);
+
+   binder_cleanup_transaction(t, "put_user failed",
+  BR_FAILED_REPLY);
+
return -EFAULT;
}
ptr += sizeof(uint32_t);
if (copy_to_user(ptr, , sizeof(tr))) {
if (t_from)
binder_thread_dec_tmpref(t_from);
+
+   binder_cleanup_transaction(t, "copy_to_user failed",
+  BR_FAILED_REPLY);
+
return -EFAULT;
}
ptr += sizeof(tr);
@@ -4090,15 +4118,9 @@ static void binder_release_work(struct binder_proc *proc,
struct binder_transaction *t;
 
t = container_of(w, struct binder_transaction, work);
-   if (t->buffer->target_node &&
-   !(t->flags & TF_ONE_WAY)) {
-   binder_send_failed_reply(t, BR_DEAD_REPLY);
-   } else {
-   binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
-   "undelivered transaction %d\n",
-   t->debug_id);
-   binder_free_transaction(t);
-   }
+
+   binder_cleanup_transaction(t, "process died.",
+  BR_DEAD_REPLY);
} break;
case BINDER_WORK_RETURN_ERROR: {
struct binder_error *e = container_of(
-- 
2.15.0.448.gf294e3d99a-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 6/6] ANDROID: binder: Add tracing for binder priority inheritance.

[Martijn Coenen]

This allows to easily trace and visualize priority inheritance
in the binder driver.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c   |  4 
 drivers/android/binder_trace.h | 24 
 2 files changed, 28 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index af1c3bb3f53e..9bdffa7f9529 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1203,6 +1203,10 @@ static void binder_do_set_priority(struct task_struct 
*task,
  task->pid, desired.prio,
  to_kernel_prio(policy, priority));
 
+   trace_binder_set_priority(task->tgid, task->pid, task->normal_prio,
+ to_kernel_prio(policy, priority),
+ desired.prio);
+
/* Set the actual priority */
if (task->policy != policy || is_rt_policy(policy)) {
struct sched_param params;
diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
index 76e3b9c8a8a2..b11dffc521e8 100644
--- a/drivers/android/binder_trace.h
+++ b/drivers/android/binder_trace.h
@@ -85,6 +85,30 @@ DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_ioctl_done);
 DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_write_done);
 DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_read_done);
 
+TRACE_EVENT(binder_set_priority,
+   TP_PROTO(int proc, int thread, unsigned int old_prio,
+unsigned int desired_prio, unsigned int new_prio),
+   TP_ARGS(proc, thread, old_prio, new_prio, desired_prio),
+
+   TP_STRUCT__entry(
+   __field(int, proc)
+   __field(int, thread)
+   __field(unsigned int, old_prio)
+   __field(unsigned int, new_prio)
+   __field(unsigned int, desired_prio)
+   ),
+   TP_fast_assign(
+   __entry->proc = proc;
+   __entry->thread = thread;
+   __entry->old_prio = old_prio;
+   __entry->new_prio = new_prio;
+   __entry->desired_prio = desired_prio;
+   ),
+   TP_printk("proc=%d thread=%d old=%d => new=%d desired=%d",
+ __entry->proc, __entry->thread, __entry->old_prio,
+ __entry->new_prio, __entry->desired_prio)
+);
+
 TRACE_EVENT(binder_wait_for_work,
TP_PROTO(bool proc_work, bool transaction_stack, bool thread_todo),
TP_ARGS(proc_work, transaction_stack, thread_todo),
-- 
2.15.0.rc2.357.g7e34df9404-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 4/6] ANDROID: binder: add RT inheritance flag to node.

[Martijn Coenen]

Allows a binder node to specify whether it wants to
inherit real-time scheduling policy from a caller. This
inheritance may not always be desirable, for example in
cases where the binder call runs untrusted and therefore
potentially unbounded code.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c| 21 +++--
 include/uapi/linux/android/binder.h |  8 
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4c42e268b1a5..5958a0876fe8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -358,6 +358,8 @@ struct binder_error {
  *(invariant after initialized)
  * @min_priority: minimum scheduling priority
  *(invariant after initialized)
+ * @inherit_rt:   inherit RT scheduling policy from caller
+ *(invariant after initialized)
  * @async_todo:   list of async work items
  *(protected by @proc->inner_lock)
  *
@@ -394,6 +396,7 @@ struct binder_node {
 * invariant after initialization
 */
u8 sched_policy:2;
+   u8 inherit_rt:1;
u8 accept_fds:1;
u8 min_priority;
};
@@ -1214,9 +1217,10 @@ static void binder_set_priority(struct task_struct *task,
 
 static void binder_transaction_priority(struct task_struct *task,
struct binder_transaction *t,
-   struct binder_priority node_prio)
+   struct binder_priority node_prio,
+   bool inherit_rt)
 {
-   struct binder_priority desired_prio;
+   struct binder_priority desired_prio = t->priority;
 
if (t->set_priority_called)
return;
@@ -1225,8 +1229,10 @@ static void binder_transaction_priority(struct 
task_struct *task,
t->saved_priority.sched_policy = task->policy;
t->saved_priority.prio = task->normal_prio;
 
-   desired_prio.prio = t->priority.prio;
-   desired_prio.sched_policy = t->priority.sched_policy;
+   if (!inherit_rt && is_rt_policy(desired_prio.sched_policy)) {
+   desired_prio.prio = NICE_TO_PRIO(0);
+   desired_prio.sched_policy = SCHED_NORMAL;
+   }
 
if (node_prio.prio < t->priority.prio ||
(node_prio.prio == t->priority.prio &&
@@ -1332,6 +1338,7 @@ static struct binder_node *binder_init_node_ilocked(
FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT;
node->min_priority = to_kernel_prio(node->sched_policy, priority);
node->accept_fds = !!(flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
+   node->inherit_rt = !!(flags & FLAT_BINDER_FLAG_INHERIT_RT);
spin_lock_init(>lock);
INIT_LIST_HEAD(>work.entry);
INIT_LIST_HEAD(>async_todo);
@@ -2749,7 +2756,8 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
 
if (thread) {
target_list = >todo;
-   binder_transaction_priority(thread->task, t, node_prio);
+   binder_transaction_priority(thread->task, t, node_prio,
+   node->inherit_rt);
} else if (!target_list) {
target_list = >todo;
} else {
@@ -4173,7 +4181,8 @@ static int binder_thread_read(struct binder_proc *proc,
tr.cookie =  target_node->cookie;
node_prio.sched_policy = target_node->sched_policy;
node_prio.prio = target_node->min_priority;
-   binder_transaction_priority(current, t, node_prio);
+   binder_transaction_priority(current, t, node_prio,
+   target_node->inherit_rt);
cmd = BR_TRANSACTION;
} else {
tr.target.ptr = 0;
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/binder.h
index b3bced80adea..5539933b3491 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -79,6 +79,14 @@ enum flat_binder_object_flags {
 */
FLAT_BINDER_FLAG_SCHED_POLICY_MASK =
3U << FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT,
+
+   /**
+* @FLAT_BINDER_FLAG_INHERIT_RT: whether the node inherits RT policy
+*
+* Only when set, calls into this node will inherit a real-time
+* scheduling policy from the caller (for synchronous transactions).
+*/
+   FLAT_BINDER_FLAG_INHERIT_RT = 0x800,
 };
 
 #ifdef BINDER_IPC_32BIT
-- 
2.15.0.rc2.357.g7e34df9404-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 5/6] ANDROID: binder: don't check prio permissions on restore.

[Martijn Coenen]

Because we have disabled RT priority inheritance for
the regular binder domain, the following can happen:

1) thread A (prio 98) calls into thread B
2) because RT prio inheritance is disabled, thread B
   runs at the lowest nice (prio 100) instead
3) thread B calls back into A; A will run at prio 100
   for the duration of the transaction
4) When thread A is done with the call from B, we will
   try to restore the prio back to 98. But, we fail
   because the process doesn't hold CAP_SYS_NICE,
   neither is RLIMIT_RT_PRIO set.

While the proper fix going forward will be to
correctly apply CAP_SYS_NICE or RLIMIT_RT_PRIO,
for now it seems reasonable to not check permissions
on the restore path.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 30 ++
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 5958a0876fe8..af1c3bb3f53e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1109,9 +1109,10 @@ static int to_kernel_prio(int policy, int user_priority)
 }
 
 /**
- * binder_set_priority() - sets the scheduler priority of a task
+ * binder_do_set_priority() - sets the scheduler priority of a task
  * @task:  task to set priority on
  * @desired:   desired priority to run at
+ * @verify:verify whether @task is allowed to run at @desired prio
  *
  * The scheduler policy of tasks is changed explicitly, because we want to
  * support a few distinct features:
@@ -1145,8 +1146,9 @@ static int to_kernel_prio(int policy, int user_priority)
  *temporarily runs at its original priority.
  * 5) rt_mutex does not currently support PI for CFS tasks.
  */
-static void binder_set_priority(struct task_struct *task,
-   struct binder_priority desired)
+static void binder_do_set_priority(struct task_struct *task,
+  struct binder_priority desired,
+  bool verify)
 {
int priority; /* user-space prio value */
bool has_cap_nice;
@@ -1170,7 +1172,7 @@ static void binder_set_priority(struct task_struct *task,
 
priority = to_userspace_prio(policy, desired.prio);
 
-   if (is_rt_policy(policy) && !has_cap_nice) {
+   if (verify && is_rt_policy(policy) && !has_cap_nice) {
long max_rtprio = task_rlimit(task, RLIMIT_RTPRIO);
 
if (max_rtprio == 0) {
@@ -1182,7 +1184,7 @@ static void binder_set_priority(struct task_struct *task,
}
}
 
-   if (is_fair_policy(policy) && !has_cap_nice) {
+   if (verify && is_fair_policy(policy) && !has_cap_nice) {
long min_nice = rlimit_to_nice(task_rlimit(task, RLIMIT_NICE));
 
if (min_nice > MAX_NICE) {
@@ -1215,6 +1217,18 @@ static void binder_set_priority(struct task_struct *task,
set_user_nice(task, priority);
 }
 
+static void binder_set_priority(struct task_struct *task,
+   struct binder_priority desired)
+{
+   binder_do_set_priority(task, desired, /* verify = */ true);
+}
+
+static void binder_restore_priority(struct task_struct *task,
+   struct binder_priority desired)
+{
+   binder_do_set_priority(task, desired, /* verify = */ false);
+}
+
 static void binder_transaction_priority(struct task_struct *task,
struct binder_transaction *t,
struct binder_priority node_prio,
@@ -3282,7 +3296,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_enqueue_work_ilocked(>work, _thread->todo);
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
-   binder_set_priority(current, in_reply_to->saved_priority);
+   binder_restore_priority(current, in_reply_to->saved_priority);
binder_free_transaction(in_reply_to);
} else if (!(t->flags & TF_ONE_WAY)) {
BUG_ON(t->buffer->async_transaction != 0);
@@ -3377,7 +3391,7 @@ static void binder_transaction(struct binder_proc *proc,
 
BUG_ON(thread->return_error.cmd != BR_OK);
if (in_reply_to) {
-   binder_set_priority(current, in_reply_to->saved_priority);
+   binder_restore_priority(current, in_reply_to->saved_priority);
thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
binder_enqueue_work(thread->proc,
>return_error.work,
@@ -3963,7 +3977,7 @@ static int binder_thread_read(struct binder_proc *proc,
wait_event_interruptible(binder_user_error_wait,
 binder_stop_on

[PATCH v3 3/6] ANDROID: binder: improve priority inheritance.

[Martijn Coenen]

By raising the priority of a thread selected for
a transaction *before* we wake it up.

Delay restoring the priority when doing a reply
until after we wake-up the process receiving
the reply.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 74 ++--
 1 file changed, 53 insertions(+), 21 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 5d51a17dba12..4c42e268b1a5 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -610,6 +610,7 @@ enum {
  * @is_dead:  thread is dead and awaiting free
  *when outstanding transactions are cleaned up
  *(protected by @proc->inner_lock)
+ * @task: struct task_struct for this thread
  *
  * Bookkeeping structure for binder threads.
  */
@@ -628,6 +629,7 @@ struct binder_thread {
struct binder_stats stats;
atomic_t tmp_ref;
bool is_dead;
+   struct task_struct *task;
 };
 
 struct binder_transaction {
@@ -646,6 +648,7 @@ struct binder_transaction {
unsigned intflags;
struct binder_priority  priority;
struct binder_priority  saved_priority;
+   boolset_priority_called;
kuid_t  sender_euid;
/**
 * @lock:  protects @from, @to_proc, and @to_thread
@@ -1209,6 +1212,38 @@ static void binder_set_priority(struct task_struct *task,
set_user_nice(task, priority);
 }
 
+static void binder_transaction_priority(struct task_struct *task,
+   struct binder_transaction *t,
+   struct binder_priority node_prio)
+{
+   struct binder_priority desired_prio;
+
+   if (t->set_priority_called)
+   return;
+
+   t->set_priority_called = true;
+   t->saved_priority.sched_policy = task->policy;
+   t->saved_priority.prio = task->normal_prio;
+
+   desired_prio.prio = t->priority.prio;
+   desired_prio.sched_policy = t->priority.sched_policy;
+
+   if (node_prio.prio < t->priority.prio ||
+   (node_prio.prio == t->priority.prio &&
+node_prio.sched_policy == SCHED_FIFO)) {
+   /*
+* In case the minimum priority on the node is
+* higher (lower value), use that priority. If
+* the priority is the same, but the node uses
+* SCHED_FIFO, prefer SCHED_FIFO, since it can
+* run unbounded, unlike SCHED_RR.
+*/
+   desired_prio = node_prio;
+   }
+
+   binder_set_priority(task, desired_prio);
+}
+
 static struct binder_node *binder_get_node_ilocked(struct binder_proc *proc,
   binder_uintptr_t ptr)
 {
@@ -2682,11 +2717,15 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
 {
struct list_head *target_list = NULL;
struct binder_node *node = t->buffer->target_node;
+   struct binder_priority node_prio;
bool oneway = !!(t->flags & TF_ONE_WAY);
bool wakeup = true;
 
BUG_ON(!node);
binder_node_lock(node);
+   node_prio.prio = node->min_priority;
+   node_prio.sched_policy = node->sched_policy;
+
if (oneway) {
BUG_ON(thread);
if (node->has_async_transaction) {
@@ -2708,12 +2747,14 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
if (!thread && !target_list)
thread = binder_select_thread_ilocked(proc);
 
-   if (thread)
+   if (thread) {
target_list = >todo;
-   else if (!target_list)
+   binder_transaction_priority(thread->task, t, node_prio);
+   } else if (!target_list) {
target_list = >todo;
-   else
+   } else {
BUG_ON(target_list != >async_todo);
+   }
 
binder_enqueue_work_ilocked(>work, target_list);
 
@@ -2832,7 +2873,6 @@ static void binder_transaction(struct binder_proc *proc,
}
thread->transaction_stack = in_reply_to->to_parent;
binder_inner_proc_unlock(proc);
-   binder_set_priority(current, in_reply_to->saved_priority);
target_thread = binder_get_txn_from_and_acq_inner(in_reply_to);
if (target_thread == NULL) {
return_error = BR_DEAD_REPLY;
@@ -3234,6 +3274,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_enqueue_work_ilocked(>work, _thread->todo);
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
+   binder_set_priority(current, in_reply_to->saved_priority);
binder_free_transaction(i

[PATCH v3 2/6] ANDROID: binder: add min sched_policy to node.

[Martijn Coenen]

This change adds flags to flat_binder_object.flags
to allow indicating a minimum scheduling policy for
the node. It also clarifies the valid value range
for the priority bits in the flags.

Internally, we use the priority map that the kernel
uses, e.g. [0..99] for real-time policies and [100..139]
for the SCHED_NORMAL/SCHED_BATCH policies.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c| 28 +
 include/uapi/linux/android/binder.h | 41 -
 2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index be6e7e753013..5d51a17dba12 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -352,6 +352,8 @@ struct binder_error {
  *and by @lock)
  * @has_async_transaction: async transaction to node in progress
  *(protected by @lock)
+ * @sched_policy: minimum scheduling policy for node
+ *(invariant after initialized)
  * @accept_fds:   file descriptor operations supported for node
  *(invariant after initialized)
  * @min_priority: minimum scheduling priority
@@ -391,6 +393,7 @@ struct binder_node {
/*
 * invariant after initialization
 */
+   u8 sched_policy:2;
u8 accept_fds:1;
u8 min_priority;
};
@@ -1256,6 +1259,7 @@ static struct binder_node *binder_init_node_ilocked(
binder_uintptr_t ptr = fp ? fp->binder : 0;
binder_uintptr_t cookie = fp ? fp->cookie : 0;
__u32 flags = fp ? fp->flags : 0;
+   s8 priority;
 
assert_spin_locked(>inner_lock);
 
@@ -1288,8 +1292,10 @@ static struct binder_node *binder_init_node_ilocked(
node->ptr = ptr;
node->cookie = cookie;
node->work.type = BINDER_WORK_NODE;
-   node->min_priority = NICE_TO_PRIO(
-   flags & FLAT_BINDER_FLAG_PRIORITY_MASK);
+   priority = flags & FLAT_BINDER_FLAG_PRIORITY_MASK;
+   node->sched_policy = (flags & FLAT_BINDER_FLAG_SCHED_POLICY_MASK) >>
+   FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT;
+   node->min_priority = to_kernel_prio(node->sched_policy, priority);
node->accept_fds = !!(flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
spin_lock_init(>lock);
INIT_LIST_HEAD(>work.entry);
@@ -4125,12 +4131,17 @@ static int binder_thread_read(struct binder_proc *proc,
tr.cookie =  target_node->cookie;
t->saved_priority.sched_policy = current->policy;
t->saved_priority.prio = current->normal_prio;
-   if (target_node->min_priority < t->priority.prio) {
-   /* The min_priority on a node can currently
-* only use SCHED_NORMAL, so we can just
-* hardcode this here.
+   if (target_node->min_priority < t->priority.prio ||
+   (target_node->min_priority == t->priority.prio &&
+target_node->sched_policy == SCHED_FIFO)) {
+   /*
+* In case the minimum priority on the node is
+* higher (lower value), use that priority. If
+* the priority is the same, but the node uses
+* SCHED_FIFO, prefer SCHED_FIFO, since it can
+* run unbounded, unlike SCHED_RR.
 */
-   prio.sched_policy = SCHED_NORMAL;
+   prio.sched_policy = target_node->sched_policy;
prio.prio = target_node->min_priority;
}
binder_set_priority(current, prio);
@@ -5205,8 +5216,9 @@ static void print_binder_node_nilocked(struct seq_file *m,
hlist_for_each_entry(ref, >refs, node_entry)
count++;
 
-   seq_printf(m, "  node %d: u%016llx c%016llx hs %d hw %d ls %d lw %d is 
%d iw %d tr %d",
+   seq_printf(m, "  node %d: u%016llx c%016llx pri %d:%d hs %d hw %d ls %d 
lw %d is %d iw %d tr %d",
   node->debug_id, (u64)node->ptr, (u64)node->cookie,
+  node->sched_policy, node->min_priority,
   node->has_strong_ref, node->has_weak_ref,
   node->local_strong_refs, node->local_weak_refs,
   node->internal_strong_refs, count, node->tmp_refs);
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/

[PATCH v3 0/6] ANDROID: binder: RT priority inheritance

[Martijn Coenen]

Changes since v2 [1]:
- All patches in v2 not related to priority inheritance were merged,
  and hence removed from this series
- Fixed using the wrong mask in node scheduler policy calculation,
  originally reported by Ganesh Mahendran <opensource.gan...@gmail.com>
- Fixed using an uninitialized value for desired_prio,
  originally reported by Ganesh Mahendran <opensource.gan...@gmail.com>
  
Changes since v1 [2]:
- added more detailed commit messages and comments to the priority
  inheritance patches, including rationale for not using
  schet_setscheduler() directly, or rt_mutex prio inheritance.
  No functional changes.

[1]: https://lkml.kernel.org/r/20170831080430.118765-1-m...@android.com
[2]: https://lkml.kernel.org/r/20170825093335.100892-1-m...@android.com

---

This patch series introduces support for priority inheritance of real-time
scheduling policies in binder. With the introduction of Android Treble,
functionality that used to be in a single process is now split over two or
more processes, which communicate using binder IPC. For latency sensitive
operations such as sensor events, Bluetooth audio and rendering, inheriting
the (real-time) priority of the caller is crucial to meet requirements.

The implementation in this series directly calls into the scheduler to
modify priorities, since I haven't found a way to make this work correctly
with rt_mutex or other existing priority inheritance mechanisms. The main
reasons a PI rt_mutex doesn't work well are the following:
1) Binder supports asynchronous transactions, where a caller isn't blocked
   on a result; therefore, the caller also couldn't block on a rt_mutex.
2) Binder supports the concept of 'node priority', where the priority of a
   call is not determined by the caller, but by the endpoint the caller
   calls in to.
3) There may not necessarily be any binder threads waiting to handle a
   transaction, so the caller doesn't always have a thread to change the
   priority of; instead, the first thread to pick up the work changes its
   own priority.
4) rt_mutex doesn't support non-RT policies (though a patch was sent to
   LKML once to address this).   

More details in the patches themselves. I have found the current approach
to be reliable, but I'm happy to look into suggestions to make this work
with rt_mutex, or use other infrastructure.

All patches have already been reviewed by Android engineers and are
merged in Android's common kernel trees.

Martijn Coenen (6):
  ANDROID: binder: add support for RT prio inheritance.
  ANDROID: binder: add min sched_policy to node.
  ANDROID: binder: improve priority inheritance.
  ANDROID: binder: add RT inheritance flag to node.
  ANDROID: binder: don't check prio permissions on restore.
  ANDROID: binder: Add tracing for binder priority inheritance.

 drivers/android/binder.c| 294 
 drivers/android/binder_trace.h  |  24 +++
 include/uapi/linux/android/binder.h |  49 +-
 3 files changed, 334 insertions(+), 33 deletions(-)

-- 
2.15.0.rc2.357.g7e34df9404-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 1/6] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

Adds support for SCHED_BATCH/SCHED_FIFO/SCHED_RR priority inheritance
to the binder driver. The desired behavior is as follows:

Each thread in the binder threadpool runs at a default priority, which is
typically nice 0.

Binder nodes (endpoints that can receive binder transactions) can have a
minimum priority associated with them, which means that all transactions
on this node must run at least at this priority.

Let's say a synchronous transaction is made from task T1 in process P1
to process P2, into node N1 which has a 'N1_min_priority':
1) T1 wakes up a task T2 in P2, then blocks on a waitqueue for reply
2) T2 determines prio=max(prio_of_T1, N1_min_priority);
3) T2 sets its own priority to prio, and stores its old prio
4) T2 returns to userspace, does work
5) Eventually, T2 returns a reply transaction to the driver
6) T2 queues the reply to T1 and wakes it up
7) T2 restores its own priority to what it was

For an asynchronous transaction:
1) T1 wakes up a task T2 in P2, returns to userspace (no work left)
2) T2 determines prio=max(default_prio, N1_min_priority)
3) T2 sets its own priority to prio, and stores its old prio
4) T2 returns to userspace, does work, completes
5) T2 calls back into kernel for more work
6) T2 restores its own priority to its default priority

This still leaves a race condition, where T2 wakes up and gets preempted
before it has a chance to change its own priority. This is addressed in
one of the next patches in the series, by letting T1 change the priority
of T2 *before* waking it up.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 217 ---
 1 file changed, 188 insertions(+), 29 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 95a96a254e5d..be6e7e753013 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -77,6 +77,7 @@
 #endif
 
 #include 
+#include 
 #include "binder_alloc.h"
 #include "binder_trace.h"
 
@@ -463,6 +464,22 @@ enum binder_deferred_state {
BINDER_DEFERRED_RELEASE  = 0x04,
 };
 
+/**
+ * struct binder_priority - scheduler policy and priority
+ * @sched_policyscheduler policy
+ * @prio[100..139] for SCHED_NORMAL, [0..99] for FIFO/RT
+ *
+ * The binder driver supports inheriting the following scheduler policies:
+ * SCHED_NORMAL
+ * SCHED_BATCH
+ * SCHED_FIFO
+ * SCHED_RR
+ */
+struct binder_priority {
+   unsigned int sched_policy;
+   int prio;
+};
+
 /**
  * struct binder_proc - binder process bookkeeping
  * @proc_node:element for binder_procs list
@@ -542,7 +559,7 @@ struct binder_proc {
int requested_threads;
int requested_threads_started;
int tmp_ref;
-   long default_priority;
+   struct binder_priority default_priority;
struct dentry *debugfs_entry;
struct binder_alloc alloc;
struct binder_context *context;
@@ -624,8 +641,8 @@ struct binder_transaction {
struct binder_buffer *buffer;
unsigned intcode;
unsigned intflags;
-   longpriority;
-   longsaved_priority;
+   struct binder_priority  priority;
+   struct binder_priority  saved_priority;
kuid_t  sender_euid;
/**
 * @lock:  protects @from, @to_proc, and @to_thread
@@ -1051,22 +1068,142 @@ static void binder_wakeup_proc_ilocked(struct 
binder_proc *proc)
binder_wakeup_thread_ilocked(proc, thread, /* sync = */false);
 }
 
-static void binder_set_nice(long nice)
+static bool is_rt_policy(int policy)
+{
+   return policy == SCHED_FIFO || policy == SCHED_RR;
+}
+
+static bool is_fair_policy(int policy)
+{
+   return policy == SCHED_NORMAL || policy == SCHED_BATCH;
+}
+
+static bool binder_supported_policy(int policy)
+{
+   return is_fair_policy(policy) || is_rt_policy(policy);
+}
+
+static int to_userspace_prio(int policy, int kernel_priority)
+{
+   if (is_fair_policy(policy))
+   return PRIO_TO_NICE(kernel_priority);
+   else
+   return MAX_USER_RT_PRIO - 1 - kernel_priority;
+}
+
+static int to_kernel_prio(int policy, int user_priority)
+{
+   if (is_fair_policy(policy))
+   return NICE_TO_PRIO(user_priority);
+   else
+   return MAX_USER_RT_PRIO - 1 - user_priority;
+}
+
+/**
+ * binder_set_priority() - sets the scheduler priority of a task
+ * @task:  task to set priority on
+ * @desired:   desired priority to run at
+ *
+ * The scheduler policy of tasks is changed explicitly, because we want to
+ * support a few distinct features:
+ * 1) If the requested priority is higher than the maximum allowed priority,
+ *we want to "clip" at the highest supported priority.
+ * 2) For a future patch, we need to allow changing the priority of a task
+ *with a different UID; when we make a binder transaction from process A
+ *to process B with diffe

Re: [PATCH] ANDROID: binder: call poll_wait() unconditionally.

[Martijn Coenen]

On Mon, Oct 9, 2017 at 2:37 PM, Greg KH  wrote:
> Does this need to get into 4.14-final, or is 4.15-rc1 ok?  I'm a bit
> lost as to which patches I applied to what tree...

This fixes a race that is somewhat hard to hit, I've only ever seen it
with test code that creates the right conditions. But when it does hit
it's pretty bad (process blocks forever), so 4.14 would be my
preference. I just looked at v4.14-rc4 and it applied cleanly.

>
> thanks,
>
> greg "drowning in trees" k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] ANDROID: binder: call poll_wait() unconditionally.

[Martijn Coenen]

Because we're not guaranteed that subsequent calls
to poll() will have a poll_table_struct parameter
with _qproc set. When _qproc is not set, poll_wait()
is a noop, and we won't be woken up correctly.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d055b3f2a207..99f77fec1aaa 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3623,12 +3623,6 @@ static void binder_stat_br(struct binder_proc *proc,
}
 }
 
-static int binder_has_thread_work(struct binder_thread *thread)
-{
-   return !binder_worklist_empty(thread->proc, >todo) ||
-   thread->looper_need_return;
-}
-
 static int binder_put_node_cmd(struct binder_proc *proc,
   struct binder_thread *thread,
   void __user **ptrp,
@@ -4258,12 +4252,9 @@ static unsigned int binder_poll(struct file *filp,
 
binder_inner_proc_unlock(thread->proc);
 
-   if (binder_has_work(thread, wait_for_proc_work))
-   return POLLIN;
-
poll_wait(filp, >wait, wait);
 
-   if (binder_has_thread_work(thread))
+   if (binder_has_work(thread, wait_for_proc_work))
return POLLIN;
 
return 0;
-- 
2.14.2.920.gcf0c67979c-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 03/13] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

On Fri, Sep 1, 2017 at 9:24 AM, Greg KH  wrote:
>
> I've now applied patches 1, 2, 7, 9, 11, and 12 from this series to my
> tree, so feel free to rebase on it for the next round of these patches.

Thanks Greg. You should also be able to apply patch 10 from this
series ("ANDROID: binder: call poll_wait() unconditionally.") as it is
not related to the priority inheritance work. Let me know if you
prefer me to send this one as a separate patch.

Thanks,
Martijn

> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: fix type mismatch warning

[Martijn Coenen]

On Fri, Sep 22, 2017 at 11:12 AM, Arnd Bergmann  wrote:
> How would waiting help?

Once P drops support for v7, all P userspaces (including containerized
ones) need to be v8. After a while, the number of non-Android
userspaces < P with v7 would become practically zero. But it's really
hard to draw a line when it is "good enough", so I agree with you it
doesn't really help, it just reduces the chance that somebody will be
affected by it.

> The relevant cases would seem to be:
>
> 1. updating a 32-bit Android Oreo (or earlier) OS from linux-4.9
> (or earlier) to linux-4.14 (or later) without updating libbinder:
> This would require keeping around compile-time option
> at least, for as long new kernels are supported on Oreo.

I think it is unlikely a vendor would upgrade their kernel from
linux-4.9 to linux-4.14 on an already launched Android device - it is
just too much work. Vendors ideally should pick an LTS kernel and
stick with it. But of course we can't rule it out. If it does happen,
I don't think it would be strange to expect vendors to upgrade their
userspace along with it. Many vendors (sadly) have a ton of patches on
top of mainline, that may contain non-ABI-stable interfaces in the
first place, and therefore would require a userspace push anyway.

> 2. updating a 32-bit kernel to a 64-bit kernel on hardware
> that allows this, without changing anything else:
> Theoretically fixable, but this would require significant
> code changes in the kernel module, similar to what I
> described above.

This is an issue independent of what we do here; if you used the v7
interface in your pre-4.14 32-bit kernel, you have to migrate to v8
simply because the v7 interface can't work with a (pre-4.14) 64-bit
kernel. If you upgrade to a 4.14 kernel from which v7 is removed, you
fall back to the previous case. Such a change would anyway require
building and deploying a new Android userspace, assuming you'd want to
allow running 64-bit userspace applications on such a kernel. I'm not
sure whether there are good reasons to switch to a 64-bit kernel while
maintaining a 32-bit only userspace.

> What assumptions can we make about the two problematic
> cases (1 and 2) here? Is it reasonable to require device
> makes to always update libbinder together with the kernel?

I personally think this is reasonable. Rolling out an update to an
Android device involves a lot of work; if you're going to update the
kernel, you'll often want to use the opportunity to update userspace
as well for things like security fixes. Project Treble has made it
significantly easier to update individual parts of Android.
Unfortunately binder is so pervasive that it touches pretty much
everything - migrating from v7 to v8 will require a full userspace
push. Again, I don't think this is a problem, because:
1) Launched Android devices tend to stick with the kernel version they
launched with (so v7 will stay available to them)
2) If they do want to upgrade, I don't think it's unreasonable to ask
them to do a userspace push along with it
3) New devices launching with 4.14 (and P) will have no choice to use
anything but v8

> Could that run into problems after a botched upgrade that
> reverts back to an older kernel but keeps the new user space
> or vice versa?

Our current A/B partition scheme keeps a complete copy of both the old
kernel and userspace when upgrading; if the upgrade doesn't boot, we
revert both back to the old state.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: fix type mismatch warning

[Martijn Coenen]

On Wed, Sep 20, 2017 at 3:37 PM, Arnd Bergmann  wrote:
> I'm not really worried about shipping Android products, for those
> there is no big problem using the compile-time option as they build
> everything together.

Ack.

> The case that gets interesting is a any kind of user that wants to
> run an Android application on a regular Linux box without
> using virtual machines or emulation, e.g. a an app developer,
> or a user that wants to run some Android app on their desktop
> using anbox.

I don't think the binder driver is present on any regular Linux box,
so things like anbox actually have to provide it as a module. It looks
like the current version of anbox is actually using the v8 interface,
and is also using a modified version of the binder driver (eg for
dkms, but also other hacks). The other popular one is Shashlik, but
looks like it uses a VM (just for binder). The v7 interface doesn't
work on 64-bit machines at all, so any container userspace using v7
would be seriously limiting the number of machines it could be run on,
whereas v8 runs on both 32 and 64. So I'm not sure we have to be
concerned about things like this, certainly if we have communicated
earlier (as Greg said) that binder shouldn't be used outside Android
pre 4.14.

> The scenario I had in mind is like this:

Thanks for clarifying! I think this works, but it's additional code to
maintain and support for a pretty rare (and unsupported?) scenario. I
understand that hiding ABI behind a config option is not a good
design, and that it must be removed. If we really can't remove v7 from
4.14, I would actually prefer to leave the config option (but flip the
default to v8), and remove it as soon as we think it can be (eg once P
has been in the field for a while).

Thanks,
Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: fix type mismatch warning

[Martijn Coenen]

On Wed, Sep 20, 2017 at 11:58 AM, Arnd Bergmann  wrote:
> - On stable mainline kernels (unlike android-common), the v8
>   interface has never been available as a build option, and making
>   it user-selectable will required additional patches to make it
>   actually build on 32-bit ARM. This is fixable if Greg is willing
>   to take the backports into stable kernels

Yeah that would need to be fixed indeed.

>
> - Having a compile-time option to pick between two incompatible
>   ABIs is a really bad idea, we don't do that anywhere else in
>   the kernel except for things like the choice between big-endian
>   and little-endian kernels that can't be done otherwise. If we
>   want to keep supporting both ABIs going forward, I think it
>   needs to be a runtime option instead, to allow users to migrate
>   between kernel versions.

I definitely don't want to keep it going forward, just maintain the
option for older kernels.

>
> - Since you say there are existing users of recent 32-bit Android
>   including Oreo, I also think that removing support for the v7 ABI
>   is no longer an option. I only made that suggestion under the
>   assumption that there is no user space requiring that. Linux
>   has no real way of "deprecating" an existing ABI, either it is
>   currently used and has to stay around, or it is not used at all
>   and can be removed without anybody noticing.

I don't know of any Android devices shipping with 4.14 - we don't even
have a common tree for 4.14 (4.9 is the latest). So I don't think
we're hurting anyone in the Android ecosystem if we remove v7 from
4.14. From what I know, it's extremely rare for an Android device to
change their kernel version after a device ships, but even if someone
hypothetically did update their Android device to use 4.14, they can
pretty easily switch the build-time config option. I understand that
this is against Linux' philosophy around not breaking userspace, I
just want to point out that I think from an Android POV, removing v7
from 4.14 is not an issue. I'm not sure if that is good enough.

> If we end up doing a runtime switch between the ABIs instead,
> I see two ways of doing that:
>
> The easiest implementation would make it a module parameter:
> Since there is only one instance of the binder in any given
> system, and presumably all processes talking to it have to use
> the same ABI, this should be sufficient. The main downside is
> that it prevents us from having incompatible versions per
> namespace if we ever get a containerized version of binder.

Namespace support for binder is currently being investigated, but I'm
not sure we'd ever have a system where we want to mix v7 and v8. There
really is no good reason to use v7 anymore (even on 32-bit mahcines),
we just should have removed it from our userspace a lot earlier.

>
> The correct way to do it would be to unify the two versions of
> the ABI so they can be used interchangeably: any 32-bit
> process would start out with the v7 interface and a 64-bit
> process would start out with the v8 interface

This wouldn't work with the current implementation - if a 32-bit and
64-bit process communicate, then userspace would make wrong
assumptions about the sizes of transactions. Or is this what you're
proposing to fix?

> and any new
> version of the 32-bit binder user space would issue a special
> ioctl to switch to the v8 version. If we ever get a v9 version
> of the ABI, that would then add another set of ioctl commands
> with different numbers that don't conflict with the v8 interface
> but are implemented by the same kernel module.
> Actually implementing the combined v7/v8 ioctl stuff is
> of course nontrivial.

Yes, this would be a lot of work. I'll let others chime in, but I
would still prefer to remove v7 from 4.14 onward altogether.

Thanks,
Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: fix type mismatch warning

[Martijn Coenen]

On Mon, Sep 18, 2017 at 9:49 PM, Arnd Bergmann  wrote:
> The current Kconfig comment says that v7 of the ABI is also
> incompatible with Android 4.5 and later user space. Can someone
> confirm that?

That is not actually true - v7 does work with all versions of Android
(up to and including Oreo). In fact, we've polled some vendors, and
all vendors with a 32-bit SoC are using the v7 interface, even on
recent Android versions.

> The code looks like it's written under the assumption
> that user space would dynamically pick between v7 and v8 based
> on the return value of ioctl(..., BINDER_VERSION).

It's a build-time configuration option in Android userspace.

> b) Treat the fact that we implemented the v7 binder ABI as a bug,
> since real Android uses v8, removing all traces of the v7 code from
> the kernel, and requiring users of Android 4.4 and earlier to upgrade
> their binder to a version that runs on modern kernels.

This would be my proposal as well. In fact, we have already planned to
officially remove support for the v7 interface in Android P, and
require all devices using Android P to use the 64-bit v8 interface
(regardless of whether the machine is 32-bit or 64-bit). The one
caveat is that for stable/longterm, I think we want to leave the
config option, but perhaps flip the default (to v8). The main reason
is that some vendors may have existing devices on the v7 interface,
and maybe they're just pushing security updates. We don't want to
force them to switch to 64-bit just by pulling in the latest stable.
Does that sound reasonable?

Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 03/13] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

On Thu, Aug 31, 2017 at 1:32 PM, Peter Zijlstra  wrote:
> AFAIK people are actively working on fixing that.

SCHED_DEADLINE was definitely looked at in the past. We certainly
don't use it on our own devices in Android Oreo, and I am not aware of
any current plans to use it. But the Android org is big, so I may
simply not be aware. In either case I'd be happy to support it if
there is a need for it.

>
> I still have to look at the actual patches, but it all sounds very
> dodgy. Probably won't have time until after LPC.

Appreciate if you could take a look! In particular I'm curious why
this approach is considered "dodgy", or "engineered sideways" as
Thomas pointed out earlier. From what I can tell the reason why we
want to change task priorities (potentially from a different task) is
understood. So is the main concern using sched_setscheduler()
directly, instead of having a prio "modifier" much like rt_mutex has?
We (and our partners) are using these patches without issues, though
it does come with the caveat that threads in the binder threadpool
should not be changing their own priority from userspace for this to
work correctly (though they shouldn't have been doing that before
these patches either).

Either way, it sounds like this discussion will take a bit longer, so
I will pull out the few unrelated patches from this series and post
them separately.

Thanks,
Martijn
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 03/13] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

On Thu, Aug 31, 2017 at 10:18 AM, Peter Zijlstra  wrote:
> You fail to support SCHED_DEADLINE, that's not optional.

The reason I didn't include it is that we don't use SCHED_DEADLINE in
Android userspace. Can we add support for this in a follow-up patch,
or do you consider it necessary for accepting this series? If so I'm
curious why, since the driver will still work correctly even for
SCHED_DEADLINE tasks; it just won't inherit priority in that case
(much like it didn't inherit FIFO/RR priorities before this patch).
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 11/13] ANDROID: binder: don't enqueue death notifications to thread todo.

[Martijn Coenen]

This allows userspace to request death notifications without
having to worry about getting an immediate callback on the same
thread; one scenario where this would be problematic is if the
death recipient handler grabs a lock that was already taken
earlier (eg as part of a nested transaction).

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 22 ++
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 68780b8e856c..2d23f8699d40 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3718,22 +3718,12 @@ static int binder_thread_write(struct binder_proc *proc,
ref->death = death;
if (ref->node->proc == NULL) {
ref->death->work.type = 
BINDER_WORK_DEAD_BINDER;
-   if (thread->looper &
-   (BINDER_LOOPER_STATE_REGISTERED |
-BINDER_LOOPER_STATE_ENTERED))
-   binder_enqueue_work(
-   proc,
-   >death->work,
-   >todo);
-   else {
-   binder_inner_proc_lock(proc);
-   binder_enqueue_work_ilocked(
-   >death->work,
-   >todo);
-   binder_wakeup_proc_ilocked(
-   proc);
-   binder_inner_proc_unlock(proc);
-   }
+
+   binder_inner_proc_lock(proc);
+   binder_enqueue_work_ilocked(
+   >death->work, >todo);
+   binder_wakeup_proc_ilocked(proc);
+   binder_inner_proc_unlock(proc);
}
} else {
if (ref->death == NULL) {
-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 13/13] ANDROID: binder: Add tracing for binder priority inheritance.

[Martijn Coenen]

This allows to easily trace and visualize priority inheritance
in the binder driver.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c   |  4 
 drivers/android/binder_trace.h | 24 
 2 files changed, 28 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 7768ba280177..9e13711acd14 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1203,6 +1203,10 @@ static void binder_do_set_priority(struct task_struct 
*task,
  task->pid, desired.prio,
  to_kernel_prio(policy, priority));
 
+   trace_binder_set_priority(task->tgid, task->pid, task->normal_prio,
+ to_kernel_prio(policy, priority),
+ desired.prio);
+
/* Set the actual priority */
if (task->policy != policy || is_rt_policy(policy)) {
struct sched_param params;
diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
index 76e3b9c8a8a2..b11dffc521e8 100644
--- a/drivers/android/binder_trace.h
+++ b/drivers/android/binder_trace.h
@@ -85,6 +85,30 @@ DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_ioctl_done);
 DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_write_done);
 DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_read_done);
 
+TRACE_EVENT(binder_set_priority,
+   TP_PROTO(int proc, int thread, unsigned int old_prio,
+unsigned int desired_prio, unsigned int new_prio),
+   TP_ARGS(proc, thread, old_prio, new_prio, desired_prio),
+
+   TP_STRUCT__entry(
+   __field(int, proc)
+   __field(int, thread)
+   __field(unsigned int, old_prio)
+   __field(unsigned int, new_prio)
+   __field(unsigned int, desired_prio)
+   ),
+   TP_fast_assign(
+   __entry->proc = proc;
+   __entry->thread = thread;
+   __entry->old_prio = old_prio;
+   __entry->new_prio = new_prio;
+   __entry->desired_prio = desired_prio;
+   ),
+   TP_printk("proc=%d thread=%d old=%d => new=%d desired=%d",
+ __entry->proc, __entry->thread, __entry->old_prio,
+ __entry->new_prio, __entry->desired_prio)
+);
+
 TRACE_EVENT(binder_wait_for_work,
TP_PROTO(bool proc_work, bool transaction_stack, bool thread_todo),
TP_ARGS(proc_work, transaction_stack, thread_todo),
-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 10/13] ANDROID: binder: call poll_wait() unconditionally.

[Martijn Coenen]

Because we're not guaranteed that subsequent calls
to poll() will have a poll_table_struct parameter
with _qproc set. When _qproc is not set, poll_wait()
is a noop, and we won't be woken up correctly.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 7da9c42583f7..68780b8e856c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3850,12 +3850,6 @@ static void binder_stat_br(struct binder_proc *proc,
}
 }
 
-static int binder_has_thread_work(struct binder_thread *thread)
-{
-   return !binder_worklist_empty(thread->proc, >todo) ||
-   thread->looper_need_return;
-}
-
 static int binder_put_node_cmd(struct binder_proc *proc,
   struct binder_thread *thread,
   void __user **ptrp,
@@ -4486,12 +4480,9 @@ static unsigned int binder_poll(struct file *filp,
 
binder_inner_proc_unlock(thread->proc);
 
-   if (binder_has_work(thread, wait_for_proc_work))
-   return POLLIN;
-
poll_wait(filp, >wait, wait);
 
-   if (binder_has_thread_work(thread))
+   if (binder_has_work(thread, wait_for_proc_work))
return POLLIN;
 
return 0;
-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 12/13] ANDROID: binder: don't queue async transactions to thread.

[Martijn Coenen]

This can cause issues with processes using the poll()
interface:

1) client sends two oneway transactions
2) the second one gets queued on async_todo
   (because the server didn't handle the first one
yet)
3) server returns from poll(), picks up the
   first transaction and does transaction work
4) server is done with the transaction, sends
   BC_FREE_BUFFER, and the second transaction gets
   moved to thread->todo
5) libbinder's handlePolledCommands() only handles
   the commands in the current data buffer, so
   doesn't see the new transaction
6) the server continues running and issues a new
   outgoing transaction. Now, it suddenly finds
   the incoming oneway transaction on its thread
   todo, and returns that to userspace.
7) userspace does not expect this to happen; it
   may be holding a lock while making the outgoing
   transaction, and if handling the incoming
   trasnaction requires taking the same lock,
   userspace will deadlock.

By queueing the async transaction to the proc
workqueue, we make sure it's only picked up when
a thread is ready for proc work.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 2d23f8699d40..7768ba280177 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3570,11 +3570,13 @@ static int binder_thread_write(struct binder_proc *proc,
BUG_ON(buf_node->proc != proc);
w = binder_dequeue_work_head_ilocked(
_node->async_todo);
-   if (!w)
+   if (!w) {
buf_node->has_async_transaction = 0;
-   else
+   } else {
binder_enqueue_work_ilocked(
-   w, >todo);
+   w, >todo);
+   binder_wakeup_proc_ilocked(proc);
+   }
binder_node_inner_unlock(buf_node);
}
trace_binder_transaction_buffer_release(buffer);
-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 05/13] ANDROID: binder: improve priority inheritance.

[Martijn Coenen]

By raising the priority of a thread selected for
a transaction *before* we wake it up.

Delay restoring the priority when doing a reply
until after we wake-up the process receiving
the reply.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 74 ++--
 1 file changed, 53 insertions(+), 21 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index afb3297ae520..196676729521 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -610,6 +610,7 @@ enum {
  * @is_dead:  thread is dead and awaiting free
  *when outstanding transactions are cleaned up
  *(protected by @proc->inner_lock)
+ * @task: struct task_struct for this thread
  *
  * Bookkeeping structure for binder threads.
  */
@@ -628,6 +629,7 @@ struct binder_thread {
struct binder_stats stats;
atomic_t tmp_ref;
bool is_dead;
+   struct task_struct *task;
 };
 
 struct binder_transaction {
@@ -646,6 +648,7 @@ struct binder_transaction {
unsigned intflags;
struct binder_priority  priority;
struct binder_priority  saved_priority;
+   boolset_priority_called;
kuid_t  sender_euid;
/**
 * @lock:  protects @from, @to_proc, and @to_thread
@@ -1209,6 +1212,38 @@ static void binder_set_priority(struct task_struct *task,
set_user_nice(task, priority);
 }
 
+static void binder_transaction_priority(struct task_struct *task,
+   struct binder_transaction *t,
+   struct binder_priority node_prio)
+{
+   struct binder_priority desired_prio;
+
+   if (t->set_priority_called)
+   return;
+
+   t->set_priority_called = true;
+   t->saved_priority.sched_policy = task->policy;
+   t->saved_priority.prio = task->normal_prio;
+
+   desired_prio.prio = t->priority.prio;
+   desired_prio.sched_policy = t->priority.sched_policy;
+
+   if (node_prio.prio < t->priority.prio ||
+   (node_prio.prio == t->priority.prio &&
+node_prio.sched_policy == SCHED_FIFO)) {
+   /*
+* In case the minimum priority on the node is
+* higher (lower value), use that priority. If
+* the priority is the same, but the node uses
+* SCHED_FIFO, prefer SCHED_FIFO, since it can
+* run unbounded, unlike SCHED_RR.
+*/
+   desired_prio = node_prio;
+   }
+
+   binder_set_priority(task, desired_prio);
+}
+
 static struct binder_node *binder_get_node_ilocked(struct binder_proc *proc,
   binder_uintptr_t ptr)
 {
@@ -2682,11 +2717,15 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
 {
struct list_head *target_list = NULL;
struct binder_node *node = t->buffer->target_node;
+   struct binder_priority node_prio;
bool oneway = !!(t->flags & TF_ONE_WAY);
bool wakeup = true;
 
BUG_ON(!node);
binder_node_lock(node);
+   node_prio.prio = node->min_priority;
+   node_prio.sched_policy = node->sched_policy;
+
if (oneway) {
BUG_ON(thread);
if (node->has_async_transaction) {
@@ -2708,12 +2747,14 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
if (!thread && !target_list)
thread = binder_select_thread_ilocked(proc);
 
-   if (thread)
+   if (thread) {
target_list = >todo;
-   else if (!target_list)
+   binder_transaction_priority(thread->task, t, node_prio);
+   } else if (!target_list) {
target_list = >todo;
-   else
+   } else {
BUG_ON(target_list != >async_todo);
+   }
 
binder_enqueue_work_ilocked(>work, target_list);
 
@@ -2790,7 +2831,6 @@ static void binder_transaction(struct binder_proc *proc,
}
thread->transaction_stack = in_reply_to->to_parent;
binder_inner_proc_unlock(proc);
-   binder_set_priority(current, in_reply_to->saved_priority);
target_thread = binder_get_txn_from_and_acq_inner(in_reply_to);
if (target_thread == NULL) {
return_error = BR_DEAD_REPLY;
@@ -3200,6 +3240,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_enqueue_work_ilocked(>work, _thread->todo);
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
+   binder_set_priority(current, in_reply_to->saved_priority);
binder_free_transaction(i

[PATCH v2 02/13] ANDROID: binder: push new transactions to waiting threads.

[Martijn Coenen]

Instead of pushing new transactions to the process
waitqueue, select a thread that is waiting on proc
work to handle the transaction. This will make it
easier to improve priority inheritance in future
patches, by setting the priority before we wake up
a thread.

If we can't find a waiting thread, submit the work
to the proc waitqueue instead as we did previously.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 181 +--
 1 file changed, 127 insertions(+), 54 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 56211d025c2d..5366726db968 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -970,7 +970,20 @@ static void binder_wakeup_poll_threads_ilocked(struct 
binder_proc *proc,
}
 }
 
-static void binder_wakeup_proc_ilocked(struct binder_proc *proc, bool sync)
+/**
+ * binder_select_thread_ilocked() - selects a thread for doing proc work.
+ * @proc:  process to select a thread from
+ *
+ * Note that calling this function moves the thread off the waiting_threads
+ * list, so it can only be woken up by the caller of this function, or a
+ * signal. Therefore, callers *should* always wake up the thread this function
+ * returns.
+ *
+ * Return: If there's a thread currently waiting for process work,
+ * returns that thread. Otherwise returns NULL.
+ */
+static struct binder_thread *
+binder_select_thread_ilocked(struct binder_proc *proc)
 {
struct binder_thread *thread;
 
@@ -979,8 +992,35 @@ static void binder_wakeup_proc_ilocked(struct binder_proc 
*proc, bool sync)
  struct binder_thread,
  waiting_thread_node);
 
-   if (thread) {
+   if (thread)
list_del_init(>waiting_thread_node);
+
+   return thread;
+}
+
+/**
+ * binder_wakeup_thread_ilocked() - wakes up a thread for doing proc work.
+ * @proc:  process to wake up a thread in
+ * @thread:specific thread to wake-up (may be NULL)
+ * @sync:  whether to do a synchronous wake-up
+ *
+ * This function wakes up a thread in the @proc process.
+ * The caller may provide a specific thread to wake-up in
+ * the @thread parameter. If @thread is NULL, this function
+ * will wake up threads that have called poll().
+ *
+ * Note that for this function to work as expected, callers
+ * should first call binder_select_thread() to find a thread
+ * to handle the work (if they don't have a thread already),
+ * and pass the result into the @thread parameter.
+ */
+static void binder_wakeup_thread_ilocked(struct binder_proc *proc,
+struct binder_thread *thread,
+bool sync)
+{
+   BUG_ON(!spin_is_locked(>inner_lock));
+
+   if (thread) {
if (sync)
wake_up_interruptible_sync(>wait);
else
@@ -1004,6 +1044,13 @@ static void binder_wakeup_proc_ilocked(struct 
binder_proc *proc, bool sync)
binder_wakeup_poll_threads_ilocked(proc, sync);
 }
 
+static void binder_wakeup_proc_ilocked(struct binder_proc *proc)
+{
+   struct binder_thread *thread = binder_select_thread_ilocked(proc);
+
+   binder_wakeup_thread_ilocked(proc, thread, /* sync = */false);
+}
+
 static void binder_set_nice(long nice)
 {
long min_nice;
@@ -1222,7 +1269,7 @@ static bool binder_dec_node_nilocked(struct binder_node 
*node,
if (proc && (node->has_strong_ref || node->has_weak_ref)) {
if (list_empty(>work.entry)) {
binder_enqueue_work_ilocked(>work, >todo);
-   binder_wakeup_proc_ilocked(proc, false);
+   binder_wakeup_proc_ilocked(proc);
}
} else {
if (hlist_empty(>refs) && !node->local_strong_refs &&
@@ -2468,6 +2515,73 @@ static int binder_fixup_parent(struct binder_transaction 
*t,
return 0;
 }
 
+/**
+ * binder_proc_transaction() - sends a transaction to a process and wakes it up
+ * @t: transaction to send
+ * @proc:  process to send the transaction to
+ * @thread:thread in @proc to send the transaction to (may be NULL)
+ *
+ * This function queues a transaction to the specified process. It will try
+ * to find a thread in the target process to handle the transaction and
+ * wake it up. If no thread is found, the work is queued to the proc
+ * waitqueue.
+ *
+ * If the @thread parameter is not NULL, the transaction is always queued
+ * to the waitlist of that specific thread.
+ *
+ * Return: true if the transactions was successfully queued
+ * false if the target process or thread is dead
+ */
+static bool binder_proc_transaction(struct binder_transaction *t,
+   struct binder_proc *proc,
+   

[PATCH v2 04/13] ANDROID: binder: add min sched_policy to node.

[Martijn Coenen]

This change adds flags to flat_binder_object.flags
to allow indicating a minimum scheduling policy for
the node. It also clarifies the valid value range
for the priority bits in the flags.

Internally, we use the priority map that the kernel
uses, e.g. [0..99] for real-time policies and [100..139]
for the SCHED_NORMAL/SCHED_BATCH policies.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c| 28 +
 include/uapi/linux/android/binder.h | 41 -
 2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 282c2a89ab2e..afb3297ae520 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -352,6 +352,8 @@ struct binder_error {
  *and by @lock)
  * @has_async_transaction: async transaction to node in progress
  *(protected by @lock)
+ * @sched_policy: minimum scheduling policy for node
+ *(invariant after initialized)
  * @accept_fds:   file descriptor operations supported for node
  *(invariant after initialized)
  * @min_priority: minimum scheduling priority
@@ -391,6 +393,7 @@ struct binder_node {
/*
 * invariant after initialization
 */
+   u8 sched_policy:2;
u8 accept_fds:1;
u8 min_priority;
};
@@ -1256,6 +1259,7 @@ static struct binder_node *binder_init_node_ilocked(
binder_uintptr_t ptr = fp ? fp->binder : 0;
binder_uintptr_t cookie = fp ? fp->cookie : 0;
__u32 flags = fp ? fp->flags : 0;
+   s8 priority;
 
BUG_ON(!spin_is_locked(>inner_lock));
while (*p) {
@@ -1287,8 +1291,10 @@ static struct binder_node *binder_init_node_ilocked(
node->ptr = ptr;
node->cookie = cookie;
node->work.type = BINDER_WORK_NODE;
-   node->min_priority = NICE_TO_PRIO(
-   flags & FLAT_BINDER_FLAG_PRIORITY_MASK);
+   priority = flags & FLAT_BINDER_FLAG_PRIORITY_MASK;
+   node->sched_policy = (flags & FLAT_BINDER_FLAG_PRIORITY_MASK) >>
+   FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT;
+   node->min_priority = to_kernel_prio(node->sched_policy, priority);
node->accept_fds = !!(flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
spin_lock_init(>lock);
INIT_LIST_HEAD(>work.entry);
@@ -4099,12 +4105,17 @@ static int binder_thread_read(struct binder_proc *proc,
tr.cookie =  target_node->cookie;
t->saved_priority.sched_policy = current->policy;
t->saved_priority.prio = current->normal_prio;
-   if (target_node->min_priority < t->priority.prio) {
-   /* The min_priority on a node can currently
-* only use SCHED_NORMAL, so we can just
-* hardcode this here.
+   if (target_node->min_priority < t->priority.prio ||
+   (target_node->min_priority == t->priority.prio &&
+target_node->sched_policy == SCHED_FIFO)) {
+   /*
+* In case the minimum priority on the node is
+* higher (lower value), use that priority. If
+* the priority is the same, but the node uses
+* SCHED_FIFO, prefer SCHED_FIFO, since it can
+* run unbounded, unlike SCHED_RR.
 */
-   prio.sched_policy = SCHED_NORMAL;
+   prio.sched_policy = target_node->sched_policy;
prio.prio = target_node->min_priority;
}
binder_set_priority(current, prio);
@@ -5145,8 +5156,9 @@ static void print_binder_node_nilocked(struct seq_file *m,
hlist_for_each_entry(ref, >refs, node_entry)
count++;
 
-   seq_printf(m, "  node %d: u%016llx c%016llx hs %d hw %d ls %d lw %d is 
%d iw %d tr %d",
+   seq_printf(m, "  node %d: u%016llx c%016llx pri %d:%d hs %d hw %d ls %d 
lw %d is %d iw %d tr %d",
   node->debug_id, (u64)node->ptr, (u64)node->cookie,
+  node->sched_policy, node->min_priority,
   node->has_strong_ref, node->has_weak_ref,
   node->local_strong_refs, node->local_weak_refs,
   node->internal_strong_refs, count, node->tmp_refs);
diff --git a/include/uapi/linux/android/binder.h 
b/i

[PATCH v2 07/13] ANDROID: binder: Add BINDER_GET_NODE_DEBUG_INFO ioctl

[Martijn Coenen]

From: Colin Cross <ccr...@android.com>

The BINDER_GET_NODE_DEBUG_INFO ioctl will return debug info on
a node.  Each successive call reusing the previous return value
will return the next node.  The data will be used by
libmemunreachable to mark the pointers with kernel references
as reachable.

Signed-off-by: Colin Cross <ccr...@android.com>
Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c| 43 +
 include/uapi/linux/android/binder.h | 14 
 2 files changed, 57 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 5edde38a77b3..017693dd4ec1 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4595,6 +4595,31 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
return ret;
 }
 
+static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
+   struct binder_node_debug_info *info)
+{
+   struct rb_node *n;
+   binder_uintptr_t ptr = info->ptr;
+
+   memset(info, 0, sizeof(*info));
+
+   binder_inner_proc_lock(proc);
+   for (n = rb_first(>nodes); n != NULL; n = rb_next(n)) {
+   struct binder_node *node = rb_entry(n, struct binder_node,
+   rb_node);
+   if (node->ptr > ptr) {
+   info->ptr = node->ptr;
+   info->cookie = node->cookie;
+   info->has_strong_ref = node->has_strong_ref;
+   info->has_weak_ref = node->has_weak_ref;
+   break;
+   }
+   }
+   binder_inner_proc_unlock(proc);
+
+   return 0;
+}
+
 static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long 
arg)
 {
int ret;
@@ -4664,6 +4689,24 @@ static long binder_ioctl(struct file *filp, unsigned int 
cmd, unsigned long arg)
}
break;
}
+   case BINDER_GET_NODE_DEBUG_INFO: {
+   struct binder_node_debug_info info;
+
+   if (copy_from_user(, ubuf, sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+
+   ret = binder_ioctl_get_node_debug_info(proc, );
+   if (ret < 0)
+   goto err;
+
+   if (copy_to_user(ubuf, , sizeof(info))) {
+   ret = -EFAULT;
+   goto err;
+   }
+   break;
+   }
default:
ret = -EINVAL;
goto err;
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/binder.h
index 70e252bf0be0..5539933b3491 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -233,6 +233,19 @@ struct binder_version {
 #define BINDER_CURRENT_PROTOCOL_VERSION 8
 #endif
 
+/*
+ * Use with BINDER_GET_NODE_DEBUG_INFO, driver reads ptr, writes to all fields.
+ * Set ptr to NULL for the first call to get the info for the first node, and
+ * then repeat the call passing the previously returned value to get the next
+ * nodes.  ptr will be 0 when there are no more nodes.
+ */
+struct binder_node_debug_info {
+   binder_uintptr_t ptr;
+   binder_uintptr_t cookie;
+   __u32has_strong_ref;
+   __u32has_weak_ref;
+};
+
 #define BINDER_WRITE_READ  _IOWR('b', 1, struct binder_write_read)
 #define BINDER_SET_IDLE_TIMEOUT_IOW('b', 3, __s64)
 #define BINDER_SET_MAX_THREADS _IOW('b', 5, __u32)
@@ -240,6 +253,7 @@ struct binder_version {
 #define BINDER_SET_CONTEXT_MGR _IOW('b', 7, __s32)
 #define BINDER_THREAD_EXIT _IOW('b', 8, __s32)
 #define BINDER_VERSION _IOWR('b', 9, struct binder_version)
+#define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct 
binder_node_debug_info)
 
 /*
  * NOTE: Two special error codes you should check for when calling
-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 08/13] ANDROID: binder: don't check prio permissions on restore.

[Martijn Coenen]

Because we have disabled RT priority inheritance for
the regular binder domain, the following can happen:

1) thread A (prio 98) calls into thread B
2) because RT prio inheritance is disabled, thread B
   runs at the lowest nice (prio 100) instead
3) thread B calls back into A; A will run at prio 100
   for the duration of the transaction
4) When thread A is done with the call from B, we will
   try to restore the prio back to 98. But, we fail
   because the process doesn't hold CAP_SYS_NICE,
   neither is RLIMIT_RT_PRIO set.

While the proper fix going forward will be to
correctly apply CAP_SYS_NICE or RLIMIT_RT_PRIO,
for now it seems reasonable to not check permissions
on the restore path.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 30 ++
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 017693dd4ec1..0c0ecd78b9a7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1109,9 +1109,10 @@ static int to_kernel_prio(int policy, int user_priority)
 }
 
 /**
- * binder_set_priority() - sets the scheduler priority of a task
+ * binder_do_set_priority() - sets the scheduler priority of a task
  * @task:  task to set priority on
  * @desired:   desired priority to run at
+ * @verify:verify whether @task is allowed to run at @desired prio
  *
  * The scheduler policy of tasks is changed explicitly, because we want to
  * support a few distinct features:
@@ -1145,8 +1146,9 @@ static int to_kernel_prio(int policy, int user_priority)
  *temporarily runs at its original priority.
  * 5) rt_mutex does not currently support PI for CFS tasks.
  */
-static void binder_set_priority(struct task_struct *task,
-   struct binder_priority desired)
+static void binder_do_set_priority(struct task_struct *task,
+  struct binder_priority desired,
+  bool verify)
 {
int priority; /* user-space prio value */
bool has_cap_nice;
@@ -1170,7 +1172,7 @@ static void binder_set_priority(struct task_struct *task,
 
priority = to_userspace_prio(policy, desired.prio);
 
-   if (is_rt_policy(policy) && !has_cap_nice) {
+   if (verify && is_rt_policy(policy) && !has_cap_nice) {
long max_rtprio = task_rlimit(task, RLIMIT_RTPRIO);
 
if (max_rtprio == 0) {
@@ -1182,7 +1184,7 @@ static void binder_set_priority(struct task_struct *task,
}
}
 
-   if (is_fair_policy(policy) && !has_cap_nice) {
+   if (verify && is_fair_policy(policy) && !has_cap_nice) {
long min_nice = rlimit_to_nice(task_rlimit(task, RLIMIT_NICE));
 
if (min_nice > MAX_NICE) {
@@ -1215,6 +1217,18 @@ static void binder_set_priority(struct task_struct *task,
set_user_nice(task, priority);
 }
 
+static void binder_set_priority(struct task_struct *task,
+   struct binder_priority desired)
+{
+   binder_do_set_priority(task, desired, /* verify = */ true);
+}
+
+static void binder_restore_priority(struct task_struct *task,
+   struct binder_priority desired)
+{
+   binder_do_set_priority(task, desired, /* verify = */ false);
+}
+
 static void binder_transaction_priority(struct task_struct *task,
struct binder_transaction *t,
struct binder_priority node_prio,
@@ -3251,7 +3265,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_enqueue_work_ilocked(>work, _thread->todo);
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
-   binder_set_priority(current, in_reply_to->saved_priority);
+   binder_restore_priority(current, in_reply_to->saved_priority);
binder_free_transaction(in_reply_to);
} else if (!(t->flags & TF_ONE_WAY)) {
BUG_ON(t->buffer->async_transaction != 0);
@@ -3340,7 +3354,7 @@ static void binder_transaction(struct binder_proc *proc,
 
BUG_ON(thread->return_error.cmd != BR_OK);
if (in_reply_to) {
-   binder_set_priority(current, in_reply_to->saved_priority);
+   binder_restore_priority(current, in_reply_to->saved_priority);
thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
binder_enqueue_work(thread->proc,
>return_error.work,
@@ -3940,7 +3954,7 @@ static int binder_thread_read(struct binder_proc *proc,
wait_event_interruptible(binder_user_error_wait,
 binder_stop_on

[PATCH v2 09/13] ANDROID: binder: Don't BUG_ON(!spin_is_locked()).

[Martijn Coenen]

Because is_spin_locked() always returns false on UP
systems.

Use assert_spin_locked() instead, and remove the
WARN_ON() instances, since those were easy to verify.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 26 ++
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 0c0ecd78b9a7..7da9c42583f7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1013,7 +1013,7 @@ binder_select_thread_ilocked(struct binder_proc *proc)
 {
struct binder_thread *thread;
 
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
thread = list_first_entry_or_null(>waiting_threads,
  struct binder_thread,
  waiting_thread_node);
@@ -1044,7 +1044,7 @@ static void binder_wakeup_thread_ilocked(struct 
binder_proc *proc,
 struct binder_thread *thread,
 bool sync)
 {
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
 
if (thread) {
if (sync)
@@ -1273,7 +1273,7 @@ static struct binder_node *binder_get_node_ilocked(struct 
binder_proc *proc,
struct rb_node *n = proc->nodes.rb_node;
struct binder_node *node;
 
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
 
while (n) {
node = rb_entry(n, struct binder_node, rb_node);
@@ -1319,7 +1319,8 @@ static struct binder_node *binder_init_node_ilocked(
__u32 flags = fp ? fp->flags : 0;
s8 priority;
 
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
+
while (*p) {
 
parent = *p;
@@ -1398,9 +1399,9 @@ static int binder_inc_node_nilocked(struct binder_node 
*node, int strong,
 {
struct binder_proc *proc = node->proc;
 
-   BUG_ON(!spin_is_locked(>lock));
+   assert_spin_locked(>lock);
if (proc)
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
if (strong) {
if (internal) {
if (target_list == NULL &&
@@ -1451,9 +1452,9 @@ static bool binder_dec_node_nilocked(struct binder_node 
*node,
 {
struct binder_proc *proc = node->proc;
 
-   BUG_ON(!spin_is_locked(>lock));
+   assert_spin_locked(>lock);
if (proc)
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
if (strong) {
if (internal)
node->internal_strong_refs--;
@@ -1977,7 +1978,7 @@ static void binder_pop_transaction_ilocked(struct 
binder_thread *target_thread,
   struct binder_transaction *t)
 {
BUG_ON(!target_thread);
-   BUG_ON(!spin_is_locked(_thread->proc->inner_lock));
+   assert_spin_locked(_thread->proc->inner_lock);
BUG_ON(target_thread->transaction_stack != t);
BUG_ON(target_thread->transaction_stack->from != target_thread);
target_thread->transaction_stack =
@@ -5123,7 +5124,6 @@ static void print_binder_transaction_ilocked(struct 
seq_file *m,
struct binder_proc *to_proc;
struct binder_buffer *buffer = t->buffer;
 
-   WARN_ON(!spin_is_locked(>inner_lock));
spin_lock(>lock);
to_proc = t->to_proc;
seq_printf(m,
@@ -5212,7 +5212,6 @@ static void print_binder_thread_ilocked(struct seq_file 
*m,
size_t start_pos = m->count;
size_t header_pos;
 
-   WARN_ON(!spin_is_locked(>proc->inner_lock));
seq_printf(m, "  thread %d: l %02x need_return %d tr %d\n",
thread->pid, thread->looper,
thread->looper_need_return,
@@ -5249,10 +5248,6 @@ static void print_binder_node_nilocked(struct seq_file 
*m,
struct binder_work *w;
int count;
 
-   WARN_ON(!spin_is_locked(>lock));
-   if (node->proc)
-   WARN_ON(!spin_is_locked(>proc->inner_lock));
-
count = 0;
hlist_for_each_entry(ref, >refs, node_entry)
count++;
@@ -5279,7 +5274,6 @@ static void print_binder_node_nilocked(struct seq_file *m,
 static void print_binder_ref_olocked(struct seq_file *m,
 struct binder_ref *ref)
 {
-   WARN_ON(!spin_is_locked(>proc->outer_lock));
binder_node_lock(ref->node);
seq_printf(m, "  ref %d: desc %d %snode %d s %d w %d d %pK\n",
   ref->data.debug_id, ref->data.desc,
-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 06/13] ANDROID: binder: add RT inheritance flag to node.

[Martijn Coenen]

Allows a binder node to specify whether it wants to
inherit real-time scheduling policy from a caller. This
inheritance may not always be desirable, for example in
cases where the binder call runs untrusted and therefore
potentially unbounded code.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c| 22 +-
 include/uapi/linux/android/binder.h |  8 
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 196676729521..5edde38a77b3 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -358,6 +358,8 @@ struct binder_error {
  *(invariant after initialized)
  * @min_priority: minimum scheduling priority
  *(invariant after initialized)
+ * @inherit_rt:   inherit RT scheduling policy from caller
+ *(invariant after initialized)
  * @async_todo:   list of async work items
  *(protected by @proc->inner_lock)
  *
@@ -394,6 +396,7 @@ struct binder_node {
 * invariant after initialization
 */
u8 sched_policy:2;
+   u8 inherit_rt:1;
u8 accept_fds:1;
u8 min_priority;
};
@@ -1214,7 +1217,8 @@ static void binder_set_priority(struct task_struct *task,
 
 static void binder_transaction_priority(struct task_struct *task,
struct binder_transaction *t,
-   struct binder_priority node_prio)
+   struct binder_priority node_prio,
+   bool inherit_rt)
 {
struct binder_priority desired_prio;
 
@@ -1225,8 +1229,13 @@ static void binder_transaction_priority(struct 
task_struct *task,
t->saved_priority.sched_policy = task->policy;
t->saved_priority.prio = task->normal_prio;
 
-   desired_prio.prio = t->priority.prio;
-   desired_prio.sched_policy = t->priority.sched_policy;
+   if (!inherit_rt && is_rt_policy(desired_prio.sched_policy)) {
+   desired_prio.prio = NICE_TO_PRIO(0);
+   desired_prio.sched_policy = SCHED_NORMAL;
+   } else {
+   desired_prio.prio = t->priority.prio;
+   desired_prio.sched_policy = t->priority.sched_policy;
+   }
 
if (node_prio.prio < t->priority.prio ||
(node_prio.prio == t->priority.prio &&
@@ -1331,6 +1340,7 @@ static struct binder_node *binder_init_node_ilocked(
FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT;
node->min_priority = to_kernel_prio(node->sched_policy, priority);
node->accept_fds = !!(flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
+   node->inherit_rt = !!(flags & FLAT_BINDER_FLAG_INHERIT_RT);
spin_lock_init(>lock);
INIT_LIST_HEAD(>work.entry);
INIT_LIST_HEAD(>async_todo);
@@ -2749,7 +2759,8 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
 
if (thread) {
target_list = >todo;
-   binder_transaction_priority(thread->task, t, node_prio);
+   binder_transaction_priority(thread->task, t, node_prio,
+   node->inherit_rt);
} else if (!target_list) {
target_list = >todo;
} else {
@@ -4147,7 +4158,8 @@ static int binder_thread_read(struct binder_proc *proc,
tr.cookie =  target_node->cookie;
node_prio.sched_policy = target_node->sched_policy;
node_prio.prio = target_node->min_priority;
-   binder_transaction_priority(current, t, node_prio);
+   binder_transaction_priority(current, t, node_prio,
+   target_node->inherit_rt);
cmd = BR_TRANSACTION;
} else {
tr.target.ptr = 0;
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/binder.h
index 026558ac254d..70e252bf0be0 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -79,6 +79,14 @@ enum flat_binder_object_flags {
 */
FLAT_BINDER_FLAG_SCHED_POLICY_MASK =
3U << FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT,
+
+   /**
+* @FLAT_BINDER_FLAG_INHERIT_RT: whether the node inherits RT policy
+*
+* Only when set, calls into this node will inherit a real-time
+* scheduling policy from the caller (for synchronous transactions).
+*/
+   FLAT_BINDER_FLAG_INHERIT_RT = 0x800,
 };
 
 #ifdef BINDER_IPC_32BIT
-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 01/13] ANDROID: binder: remove proc waitqueue

[Martijn Coenen]

Removes the process waitqueue, so that threads
can only wait on the thread waitqueue. Whenever
there is process work to do, pick a thread and
wake it up. Having the caller pick a thread is
helpful for things like priority inheritance.

This also fixes an issue with using epoll(),
since we no longer have to block on different
waitqueues.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 255 +--
 1 file changed, 181 insertions(+), 74 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ba9e613b42d6..56211d025c2d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -28,10 +28,10 @@
  *binder_node_lock() and binder_node_unlock() are
  *used to acq/rel
  * 3) proc->inner_lock : protects the thread and node lists
- *(proc->threads, proc->nodes) and all todo lists associated
- *with the binder_proc (proc->todo, thread->todo,
- *proc->delivered_death and node->async_todo), as well as
- *thread->transaction_stack
+ *(proc->threads, proc->waiting_threads, proc->nodes)
+ *and all todo lists associated with the binder_proc
+ *(proc->todo, thread->todo, proc->delivered_death and
+ *node->async_todo), as well as thread->transaction_stack
  *binder_inner_proc_lock() and binder_inner_proc_unlock()
  *are used to acq/rel
  *
@@ -475,6 +475,8 @@ enum binder_deferred_state {
  *(protected by @outer_lock)
  * @refs_by_node: rbtree of refs ordered by ref->node
  *(protected by @outer_lock)
+ * @waiting_threads:  threads currently waiting for proc work
+ *(protected by @inner_lock)
  * @pid   PID of group_leader of process
  *(invariant after initialized)
  * @tsk   task_struct for group_leader of process
@@ -504,8 +506,6 @@ enum binder_deferred_state {
  *(protected by @inner_lock)
  * @requested_threads_started: number binder threads started
  *(protected by @inner_lock)
- * @ready_threads:number of threads waiting for proc work
- *(protected by @inner_lock)
  * @tmp_ref:  temporary reference to indicate proc is in use
  *(protected by @inner_lock)
  * @default_priority: default scheduler priority
@@ -526,6 +526,7 @@ struct binder_proc {
struct rb_root nodes;
struct rb_root refs_by_desc;
struct rb_root refs_by_node;
+   struct list_head waiting_threads;
int pid;
struct task_struct *tsk;
struct files_struct *files;
@@ -540,7 +541,6 @@ struct binder_proc {
int max_threads;
int requested_threads;
int requested_threads_started;
-   int ready_threads;
int tmp_ref;
long default_priority;
struct dentry *debugfs_entry;
@@ -556,6 +556,7 @@ enum {
BINDER_LOOPER_STATE_EXITED  = 0x04,
BINDER_LOOPER_STATE_INVALID = 0x08,
BINDER_LOOPER_STATE_WAITING = 0x10,
+   BINDER_LOOPER_STATE_POLL= 0x20,
 };
 
 /**
@@ -564,6 +565,8 @@ enum {
  *(invariant after initialization)
  * @rb_node:  element for proc->threads rbtree
  *(protected by @proc->inner_lock)
+ * @waiting_thread_node:  element for @proc->waiting_threads list
+ *(protected by @proc->inner_lock)
  * @pid:  PID for this thread
  *(invariant after initialization)
  * @looper:   bitmap of looping state
@@ -593,6 +596,7 @@ enum {
 struct binder_thread {
struct binder_proc *proc;
struct rb_node rb_node;
+   struct list_head waiting_thread_node;
int pid;
int looper;  /* only modified by this thread */
bool looper_need_return; /* can be written by other thread */
@@ -920,6 +924,86 @@ static long task_close_fd(struct binder_proc *proc, 
unsigned int fd)
return retval;
 }
 
+static bool binder_has_work_ilocked(struct binder_thread *thread,
+   bool do_proc_work)
+{
+   return !binder_worklist_empty_ilocked(>todo) ||
+   thread->looper_need_return ||
+   (do_proc_work &&
+!binder_worklist_empty_ilocked(>proc->todo));
+}
+
+static bool binder_has_work(struct binder_thread *thread, bool do_proc_work)
+{
+   bool has_work;
+
+   binder_inner_proc_lock(thread->proc);
+   has_work = binder_has_work_ilocked(thread, do_proc_work);
+   binder_inner_proc_unlock(thread->proc);
+
+   return has_work;
+}
+
+static bool binder_available_for_proc_work_ilocked(struct binder_thread 
*thread)
+{
+   return !thread->transactio

[PATCH v2 03/13] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

Adds support for SCHED_BATCH/SCHED_FIFO/SCHED_RR priority inheritance
to the binder driver. The desired behavior is as follows:

Each thread in the binder threadpool runs at a default priority, which is
typically nice 0.

Binder nodes (endpoints that can receive binder transactions) can have a
minimum priority associated with them, which means that all transactions
on this node must run at least at this priority.

Let's say a synchronous transaction is made from task T1 in process P1
to process P2, into node N1 which has a 'N1_min_priority':
1) T1 wakes up a task T2 in P2, then blocks on a waitqueue for reply
2) T2 determines prio=max(prio_of_T1, N1_min_priority);
3) T2 sets its own priority to prio, and stores its old prio
4) T2 returns to userspace, does work
5) Eventually, T2 returns a reply transaction to the driver
6) T2 queues the reply to T1 and wakes it up
7) T2 restores its own priority to what it was

For an asynchronous transaction:
1) T1 wakes up a task T2 in P2, returns to userspace (no work left)
2) T2 determines prio=max(default_prio, N1_min_priority)
3) T2 sets its own priority to prio, and stores its old prio
4) T2 returns to userspace, does work, completes
5) T2 calls back into kernel for more work
6) T2 restores its own priority to its default priority

This still leaves a race condition, where T2 wakes up and gets preempted
before it has a chance to change its own priority. This is addressed in
one of the next patches in the series, by letting T1 change the priority
of T2 *before* waking it up.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 217 ---
 1 file changed, 188 insertions(+), 29 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 5366726db968..282c2a89ab2e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -77,6 +77,7 @@
 #endif
 
 #include 
+#include 
 #include "binder_alloc.h"
 #include "binder_trace.h"
 
@@ -463,6 +464,22 @@ enum binder_deferred_state {
BINDER_DEFERRED_RELEASE  = 0x04,
 };
 
+/**
+ * struct binder_priority - scheduler policy and priority
+ * @sched_policyscheduler policy
+ * @prio[100..139] for SCHED_NORMAL, [0..99] for FIFO/RT
+ *
+ * The binder driver supports inheriting the following scheduler policies:
+ * SCHED_NORMAL
+ * SCHED_BATCH
+ * SCHED_FIFO
+ * SCHED_RR
+ */
+struct binder_priority {
+   unsigned int sched_policy;
+   int prio;
+};
+
 /**
  * struct binder_proc - binder process bookkeeping
  * @proc_node:element for binder_procs list
@@ -542,7 +559,7 @@ struct binder_proc {
int requested_threads;
int requested_threads_started;
int tmp_ref;
-   long default_priority;
+   struct binder_priority default_priority;
struct dentry *debugfs_entry;
struct binder_alloc alloc;
struct binder_context *context;
@@ -624,8 +641,8 @@ struct binder_transaction {
struct binder_buffer *buffer;
unsigned intcode;
unsigned intflags;
-   longpriority;
-   longsaved_priority;
+   struct binder_priority  priority;
+   struct binder_priority  saved_priority;
kuid_t  sender_euid;
/**
 * @lock:  protects @from, @to_proc, and @to_thread
@@ -1051,22 +1068,142 @@ static void binder_wakeup_proc_ilocked(struct 
binder_proc *proc)
binder_wakeup_thread_ilocked(proc, thread, /* sync = */false);
 }
 
-static void binder_set_nice(long nice)
+static bool is_rt_policy(int policy)
+{
+   return policy == SCHED_FIFO || policy == SCHED_RR;
+}
+
+static bool is_fair_policy(int policy)
+{
+   return policy == SCHED_NORMAL || policy == SCHED_BATCH;
+}
+
+static bool binder_supported_policy(int policy)
+{
+   return is_fair_policy(policy) || is_rt_policy(policy);
+}
+
+static int to_userspace_prio(int policy, int kernel_priority)
+{
+   if (is_fair_policy(policy))
+   return PRIO_TO_NICE(kernel_priority);
+   else
+   return MAX_USER_RT_PRIO - 1 - kernel_priority;
+}
+
+static int to_kernel_prio(int policy, int user_priority)
+{
+   if (is_fair_policy(policy))
+   return NICE_TO_PRIO(user_priority);
+   else
+   return MAX_USER_RT_PRIO - 1 - user_priority;
+}
+
+/**
+ * binder_set_priority() - sets the scheduler priority of a task
+ * @task:  task to set priority on
+ * @desired:   desired priority to run at
+ *
+ * The scheduler policy of tasks is changed explicitly, because we want to
+ * support a few distinct features:
+ * 1) If the requested priority is higher than the maximum allowed priority,
+ *we want to "clip" at the highest supported priority.
+ * 2) For a future patch, we need to allow changing the priority of a task
+ *with a different UID; when we make a binder transaction from process A
+ *to process B with diffe

[PATCH v2 00/13] ANDROID: binder: RT priority inheritance and small fixes.

[Martijn Coenen]

Changes since v1 [1]:
- added more detailed commit messages and comments to the priority
  inheritance patches, including rationale for not using
  schet_setscheduler() directly, or rt_mutex prio inheritance.
  No functional changes.

[1]: https://lkml.kernel.org/r/20170825093335.100892-1-m...@android.com

---

The first six patches in this set introduce support for priority
inheritance of real-time scheduling policies in binder. With the
introduction of Android Treble, functionality that used to be in a
single process is now split over two or more processes, which
communicate using binder IPC. For latency sensitive operations such as
sensor events, Bluetooth audio and rendering, inheriting the
(real-time) priority of the caller is crucial to meet requirements.

The implementation in this series directly calls into the scheduler to
modify priorities, since I haven't found a way to make this work
correctly with rt_mutex or other existing priority inheritance
mechanisms. I have found the current approach to be reliable, but I'm
happy to look into suggestions to make this work with existing
infrastructure. More details in the patches themselves.

Colin's patch adds a debug ioctl that allows us to more accurately track
memory leaks, as it allows us to identify objects to which only remote
processes have a reference.

The subsequent patches are mostly small fixes and (hopefully) well
explained in the commit messages.

All patches except 'Add tracing for binder priority inheritance' have
already been reviewed by Android engineers and are merged in Android's
common kernel trees.

---

Colin Cross (1):
  ANDROID: binder: Add BINDER_GET_NODE_DEBUG_INFO ioctl

Martijn Coenen (12):
  ANDROID: binder: remove proc waitqueue
  ANDROID: binder: push new transactions to waiting threads.
  ANDROID: binder: add support for RT prio inheritance.
  ANDROID: binder: add min sched_policy to node.
  ANDROID: binder: improve priority inheritance.
  ANDROID: binder: add RT inheritance flag to node.
  ANDROID: binder: don't check prio permissions on restore.
  ANDROID: binder: Don't BUG_ON(!spin_is_locked()).
  ANDROID: binder: call poll_wait() unconditionally.
  ANDROID: binder: don't enqueue death notifications to thread todo.
  ANDROID: binder: don't queue async transactions to thread.
  ANDROID: binder: Add tracing for binder priority inheritance.

 drivers/android/binder.c| 773 
 drivers/android/binder_trace.h  |  24 ++
 include/uapi/linux/android/binder.h |  63 ++-
 3 files changed, 689 insertions(+), 171 deletions(-)

-- 
2.14.1.581.gf28d330327-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/13] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

Hi Thomas,

On Fri, Aug 25, 2017 at 5:08 PM, Thomas Gleixner  wrote:
> Sorry, but this has not much to do with real priority inheritance.

Can you clarify what "real priority inheritance" is, or are you more
concerned about this particular implementation of it?

>
> It's a poor mans pseudo PI implementation. What I can't see from the sparse
> changelog is how all of this is supposed to work.

Ok, guess the latter :-) I agree with you I should have included a
more verbose description of how this works, and I should also have
mentioned I did look at rtmutex and didn't think it could be easily
used for our purposes (more below).

>
> My interpretation of it is, that you need to make sure that the other
> thread in that IPC mechanism gets boosted enough to not block the thread
> which needs a reply or action.

This is true, but it's not the whole picture - more below.

>
> Therefor you create a unreadable maze of capability checks and other things
> which do not make any sense to me due to lack of comments and something
> which explains the big picture.

I thought the code itself was reasonably self-explanatory, but I'm
happy to comment it more. The binder driver has done "priority
inheritance" with nice values for a long time, and this is an
extension of that that more or less works in the same way.

>
> The whole thing looks wrong and engineered sideways circumventing the
> existing facilities and making weird assumptions about priority settings.

I'd be happy to use existing facilities if they can do what we need.
Can you describe the "weird assumptions" in more detail?


> Current state:
>
>   1) thread queues work to worker via binder
>
>   2) thread waits for the work to complete (I deduced that from a stray
>  comment about sync work)
>
> If the waiting thread is a high priority thread it might wait for a long
> time if the worker thread is low priority or SCHED_OTHER.
>
> Desired state:
>
>   1) thread queues work to worker thread via binder
>
>   1a) thread boosts the worker to its own priority
>
>   2) thread waits for the work to complete
>
>   2a) worker completes and drops priority boost
>
> Is that about right?

It is correct for synchronous transactions. Synchronous transactions
are transactions for which the caller blocks until they are completed
(eg a reply has been received). The receiving process has a threadpool
of one or more threads waiting for transactions. Typically those
receiving threads call into the binder driver with an ioctl and are
then waiting for work on a waitqueue. Before this patch series, all
threads (available for new transactions) would wait on the same
waitqueue, and so it was hard to do any sort of PI, because we didn't
know which thread was going to wake up. The first set in this series
changes this behavior by getting rid of this process-wide wait-queue -
the caller itself picks a thread to wake up. Anyway, after that things
more or less go as you describe: the thread picks up the work, returns
back to userspace to have it do the work, and eventually a reply comes
back, for which we unblock the caller (which is blocked on its own
waitqueue).

One place where using an rtmutex becomes tricky is that there may be
no threads waiting for work: all threads could be busy handling
transactions, so we can't pick one to wake up and do the work. In that
case, we push the work to a process-wide workqueue, and the first
thread to become available will pick up the work and do it. In that
case a "proxy rtmutex" doesn't really seem to work, because the proxy
doesn't know the owner when the work is queued. The current
implementation in this patchset just has the thread boost its own
priority in this scenario.

Another reason rtmutex is not straightforward is that we support
something called "node priority inheritance". A node is binder
terminology for an object that you can make binder transactions to.
For some nodes, we like to set a minimum scheduling priority. An
example of this are all the nodes in our system_server process. The
reason for this is that binder calls into the system server process
often take critical userspace locks. Say a thread calls into
system_server with priority 130; the system_server binder thread
inherits, runs at 130, takes a lock in userspace and gets preempted;
now, when somebody else tries to take the same lock, it can get
blocked for a long time. So, we set a minimum scheduling policy for
system_server at prio 120. This makes using rtmutex APIs hard, because
we don't necessarily want the binder thread to have the priority of
the caller. I guess you could say the proper way to fix this is that
our userspace mutexes should also support priority inheritance; I
don't think they do today, though I don't know the exact reason behind
it.

The final reason using rtmutex is not straightforward is asynchronous
transactions. Those are transactions for which the caller does not
block until completion. We push the work, wake up a thread, and 

[PATCH 12/13] ANDROID: binder: don't queue async transactions to thread.

[Martijn Coenen]

This can cause issues with processes using the poll()
interface:

1) client sends two oneway transactions
2) the second one gets queued on async_todo
   (because the server didn't handle the first one
yet)
3) server returns from poll(), picks up the
   first transaction and does transaction work
4) server is done with the transaction, sends
   BC_FREE_BUFFER, and the second transaction gets
   moved to thread->todo
5) libbinder's handlePolledCommands() only handles
   the commands in the current data buffer, so
   doesn't see the new transaction
6) the server continues running and issues a new
   outgoing transaction. Now, it suddenly finds
   the incoming oneway transaction on its thread
   todo, and returns that to userspace.
7) userspace does not expect this to happen; it
   may be holding a lock while making the outgoing
   transaction, and if handling the incoming
   trasnaction requires taking the same lock,
   userspace will deadlock.

By queueing the async transaction to the proc
workqueue, we make sure it's only picked up when
a thread is ready for proc work.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 12ab16bb676c..337c88fd675d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3520,11 +3520,13 @@ static int binder_thread_write(struct binder_proc *proc,
BUG_ON(buf_node->proc != proc);
w = binder_dequeue_work_head_ilocked(
_node->async_todo);
-   if (!w)
+   if (!w) {
buf_node->has_async_transaction = 0;
-   else
+   } else {
binder_enqueue_work_ilocked(
-   w, >todo);
+   w, >todo);
+   binder_wakeup_proc_ilocked(proc);
+   }
binder_node_inner_unlock(buf_node);
}
trace_binder_transaction_buffer_release(buffer);
-- 
2.14.1.480.gb18f417b89-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 03/13] ANDROID: binder: add support for RT prio inheritance.

[Martijn Coenen]

Adds support for SCHED_BATCH/SCHED_FIFO/SCHED_RR
priority inheritance.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 164 ++-
 1 file changed, 135 insertions(+), 29 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 2f27bce31baf..bb61116d1c0c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -77,6 +77,7 @@
 #endif
 
 #include 
+#include 
 #include "binder_alloc.h"
 #include "binder_trace.h"
 
@@ -463,6 +464,22 @@ enum binder_deferred_state {
BINDER_DEFERRED_RELEASE  = 0x04,
 };
 
+/**
+ * struct binder_priority - scheduler policy and priority
+ * @sched_policyscheduler policy
+ * @prio[100..139] for SCHED_NORMAL, [0..99] for FIFO/RT
+ *
+ * The binder driver supports inheriting the following scheduler policies:
+ * SCHED_NORMAL
+ * SCHED_BATCH
+ * SCHED_FIFO
+ * SCHED_RR
+ */
+struct binder_priority {
+   unsigned int sched_policy;
+   int prio;
+};
+
 /**
  * struct binder_proc - binder process bookkeeping
  * @proc_node:element for binder_procs list
@@ -542,7 +559,7 @@ struct binder_proc {
int requested_threads;
int requested_threads_started;
int tmp_ref;
-   long default_priority;
+   struct binder_priority default_priority;
struct dentry *debugfs_entry;
struct binder_alloc alloc;
struct binder_context *context;
@@ -624,8 +641,8 @@ struct binder_transaction {
struct binder_buffer *buffer;
unsigned intcode;
unsigned intflags;
-   longpriority;
-   longsaved_priority;
+   struct binder_priority  priority;
+   struct binder_priority  saved_priority;
kuid_t  sender_euid;
/**
 * @lock:  protects @from, @to_proc, and @to_thread
@@ -1051,22 +1068,93 @@ static void binder_wakeup_proc_ilocked(struct 
binder_proc *proc)
binder_wakeup_thread_ilocked(proc, thread, /* sync = */false);
 }
 
-static void binder_set_nice(long nice)
+static bool is_rt_policy(int policy)
+{
+   return policy == SCHED_FIFO || policy == SCHED_RR;
+}
+
+static bool is_fair_policy(int policy)
+{
+   return policy == SCHED_NORMAL || policy == SCHED_BATCH;
+}
+
+static bool binder_supported_policy(int policy)
+{
+   return is_fair_policy(policy) || is_rt_policy(policy);
+}
+
+static int to_userspace_prio(int policy, int kernel_priority)
+{
+   if (is_fair_policy(policy))
+   return PRIO_TO_NICE(kernel_priority);
+   else
+   return MAX_USER_RT_PRIO - 1 - kernel_priority;
+}
+
+static int to_kernel_prio(int policy, int user_priority)
+{
+   if (is_fair_policy(policy))
+   return NICE_TO_PRIO(user_priority);
+   else
+   return MAX_USER_RT_PRIO - 1 - user_priority;
+}
+
+static void binder_set_priority(struct task_struct *task,
+   struct binder_priority desired)
 {
-   long min_nice;
+   int priority; /* user-space prio value */
+   bool has_cap_nice;
+   unsigned int policy = desired.sched_policy;
 
-   if (can_nice(current, nice)) {
-   set_user_nice(current, nice);
+   if (task->policy == policy && task->normal_prio == desired.prio)
return;
+
+   has_cap_nice = has_capability_noaudit(task, CAP_SYS_NICE);
+
+   priority = to_userspace_prio(policy, desired.prio);
+
+   if (is_rt_policy(policy) && !has_cap_nice) {
+   long max_rtprio = task_rlimit(task, RLIMIT_RTPRIO);
+
+   if (max_rtprio == 0) {
+   policy = SCHED_NORMAL;
+   priority = MIN_NICE;
+   } else if (priority > max_rtprio) {
+   priority = max_rtprio;
+   }
}
-   min_nice = rlimit_to_nice(rlimit(RLIMIT_NICE));
-   binder_debug(BINDER_DEBUG_PRIORITY_CAP,
-"%d: nice value %ld not allowed use %ld instead\n",
- current->pid, nice, min_nice);
-   set_user_nice(current, min_nice);
-   if (min_nice <= MAX_NICE)
-   return;
-   binder_user_error("%d RLIMIT_NICE not set\n", current->pid);
+
+   if (is_fair_policy(policy) && !has_cap_nice) {
+   long min_nice = rlimit_to_nice(task_rlimit(task, RLIMIT_NICE));
+
+   if (min_nice > MAX_NICE) {
+   binder_user_error("%d RLIMIT_NICE not set\n",
+ task->pid);
+   return;
+   } else if (priority < min_nice) {
+   priority = min_nice;
+   }
+   }
+
+   if (policy != desired.sched_policy ||
+   to_kernel_prio(policy, priority) != desired.prio)
+   binder_deb

[PATCH 13/13] ANDROID: binder: Add tracing for binder priority inheritance.

[Martijn Coenen]

This allows to easily trace and visualize priority inheritance
in the binder driver.

Change-Id: I8449ae4b002e55c5e9517a47f3581e05eef051d8
Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c   |  4 
 drivers/android/binder_trace.h | 24 
 2 files changed, 28 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 337c88fd675d..b29af3de6c34 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1153,6 +1153,10 @@ static void binder_do_set_priority(struct task_struct 
*task,
  task->pid, desired.prio,
  to_kernel_prio(policy, priority));
 
+   trace_binder_set_priority(task->tgid, task->pid, task->normal_prio,
+ to_kernel_prio(policy, priority),
+ desired.prio);
+
/* Set the actual priority */
if (task->policy != policy || is_rt_policy(policy)) {
struct sched_param params;
diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
index 7967db16ba5a..1e0169796d81 100644
--- a/drivers/android/binder_trace.h
+++ b/drivers/android/binder_trace.h
@@ -85,6 +85,30 @@ DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_ioctl_done);
 DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_write_done);
 DEFINE_BINDER_FUNCTION_RETURN_EVENT(binder_read_done);
 
+TRACE_EVENT(binder_set_priority,
+   TP_PROTO(int proc, int thread, unsigned int old_prio,
+unsigned int desired_prio, unsigned int new_prio),
+   TP_ARGS(proc, thread, old_prio, new_prio, desired_prio),
+
+   TP_STRUCT__entry(
+   __field(int, proc)
+   __field(int, thread)
+   __field(unsigned int, old_prio)
+   __field(unsigned int, new_prio)
+   __field(unsigned int, desired_prio)
+   ),
+   TP_fast_assign(
+   __entry->proc = proc;
+   __entry->thread = thread;
+   __entry->old_prio = old_prio;
+   __entry->new_prio = new_prio;
+   __entry->desired_prio = desired_prio;
+   ),
+   TP_printk("proc=%d thread=%d old=%d => new=%d desired=%d",
+ __entry->proc, __entry->thread, __entry->old_prio,
+ __entry->new_prio, __entry->desired_prio)
+);
+
 TRACE_EVENT(binder_wait_for_work,
TP_PROTO(bool proc_work, bool transaction_stack, bool thread_todo),
TP_ARGS(proc_work, transaction_stack, thread_todo),
-- 
2.14.1.480.gb18f417b89-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/13] ANDROID: binder: push new transactions to waiting threads.

[Martijn Coenen]

Instead of pushing new transactions to the process
waitqueue, select a thread that is waiting on proc
work to handle the transaction. This will make it
easier to improve priority inheritance in future
patches, by setting the priority before we wake up
a thread.

If we can't find a waiting thread, submit the work
to the proc waitqueue instead as we did previously.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 181 +--
 1 file changed, 127 insertions(+), 54 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 6cc2d96be5d4..2f27bce31baf 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -970,7 +970,20 @@ static void binder_wakeup_poll_threads_ilocked(struct 
binder_proc *proc,
}
 }
 
-static void binder_wakeup_proc_ilocked(struct binder_proc *proc, bool sync)
+/**
+ * binder_select_thread_ilocked() - selects a thread for doing proc work.
+ * @proc:  process to select a thread from
+ *
+ * Note that calling this function moves the thread off the waiting_threads
+ * list, so it can only be woken up by the caller of this function, or a
+ * signal. Therefore, callers *should* always wake up the thread this function
+ * returns.
+ *
+ * Return: If there's a thread currently waiting for process work,
+ * returns that thread. Otherwise returns NULL.
+ */
+static struct binder_thread *
+binder_select_thread_ilocked(struct binder_proc *proc)
 {
struct binder_thread *thread;
 
@@ -979,8 +992,35 @@ static void binder_wakeup_proc_ilocked(struct binder_proc 
*proc, bool sync)
  struct binder_thread,
  waiting_thread_node);
 
-   if (thread) {
+   if (thread)
list_del_init(>waiting_thread_node);
+
+   return thread;
+}
+
+/**
+ * binder_wakeup_thread_ilocked() - wakes up a thread for doing proc work.
+ * @proc:  process to wake up a thread in
+ * @thread:specific thread to wake-up (may be NULL)
+ * @sync:  whether to do a synchronous wake-up
+ *
+ * This function wakes up a thread in the @proc process.
+ * The caller may provide a specific thread to wake-up in
+ * the @thread parameter. If @thread is NULL, this function
+ * will wake up threads that have called poll().
+ *
+ * Note that for this function to work as expected, callers
+ * should first call binder_select_thread() to find a thread
+ * to handle the work (if they don't have a thread already),
+ * and pass the result into the @thread parameter.
+ */
+static void binder_wakeup_thread_ilocked(struct binder_proc *proc,
+struct binder_thread *thread,
+bool sync)
+{
+   BUG_ON(!spin_is_locked(>inner_lock));
+
+   if (thread) {
if (sync)
wake_up_interruptible_sync(>wait);
else
@@ -1004,6 +1044,13 @@ static void binder_wakeup_proc_ilocked(struct 
binder_proc *proc, bool sync)
binder_wakeup_poll_threads_ilocked(proc, sync);
 }
 
+static void binder_wakeup_proc_ilocked(struct binder_proc *proc)
+{
+   struct binder_thread *thread = binder_select_thread_ilocked(proc);
+
+   binder_wakeup_thread_ilocked(proc, thread, /* sync = */false);
+}
+
 static void binder_set_nice(long nice)
 {
long min_nice;
@@ -1222,7 +1269,7 @@ static bool binder_dec_node_nilocked(struct binder_node 
*node,
if (proc && (node->has_strong_ref || node->has_weak_ref)) {
if (list_empty(>work.entry)) {
binder_enqueue_work_ilocked(>work, >todo);
-   binder_wakeup_proc_ilocked(proc, false);
+   binder_wakeup_proc_ilocked(proc);
}
} else {
if (hlist_empty(>refs) && !node->local_strong_refs &&
@@ -2468,6 +2515,73 @@ static int binder_fixup_parent(struct binder_transaction 
*t,
return 0;
 }
 
+/**
+ * binder_proc_transaction() - sends a transaction to a process and wakes it up
+ * @t: transaction to send
+ * @proc:  process to send the transaction to
+ * @thread:thread in @proc to send the transaction to (may be NULL)
+ *
+ * This function queues a transaction to the specified process. It will try
+ * to find a thread in the target process to handle the transaction and
+ * wake it up. If no thread is found, the work is queued to the proc
+ * waitqueue.
+ *
+ * If the @thread parameter is not NULL, the transaction is always queued
+ * to the waitlist of that specific thread.
+ *
+ * Return: true if the transactions was successfully queued
+ * false if the target process or thread is dead
+ */
+static bool binder_proc_transaction(struct binder_transaction *t,
+   struct binder_proc *proc,
+   

[PATCH 05/13] ANDROID: binder: improve priority inheritance.

[Martijn Coenen]

By raising the priority of a thread selected for
a transaction *before* we wake it up.

Delay restoring the priority when doing a reply
until after we wake-up the process receiving
the reply.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 74 ++--
 1 file changed, 53 insertions(+), 21 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 247e913a6e6a..76d7544120a2 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -610,6 +610,7 @@ enum {
  * @is_dead:  thread is dead and awaiting free
  *when outstanding transactions are cleaned up
  *(protected by @proc->inner_lock)
+ * @task: struct task_struct for this thread
  *
  * Bookkeeping structure for binder threads.
  */
@@ -628,6 +629,7 @@ struct binder_thread {
struct binder_stats stats;
atomic_t tmp_ref;
bool is_dead;
+   struct task_struct *task;
 };
 
 struct binder_transaction {
@@ -646,6 +648,7 @@ struct binder_transaction {
unsigned intflags;
struct binder_priority  priority;
struct binder_priority  saved_priority;
+   boolset_priority_called;
kuid_t  sender_euid;
/**
 * @lock:  protects @from, @to_proc, and @to_thread
@@ -1160,6 +1163,38 @@ static void binder_set_priority(struct task_struct *task,
set_user_nice(task, priority);
 }
 
+static void binder_transaction_priority(struct task_struct *task,
+   struct binder_transaction *t,
+   struct binder_priority node_prio)
+{
+   struct binder_priority desired_prio;
+
+   if (t->set_priority_called)
+   return;
+
+   t->set_priority_called = true;
+   t->saved_priority.sched_policy = task->policy;
+   t->saved_priority.prio = task->normal_prio;
+
+   desired_prio.prio = t->priority.prio;
+   desired_prio.sched_policy = t->priority.sched_policy;
+
+   if (node_prio.prio < t->priority.prio ||
+   (node_prio.prio == t->priority.prio &&
+node_prio.sched_policy == SCHED_FIFO)) {
+   /*
+* In case the minimum priority on the node is
+* higher (lower value), use that priority. If
+* the priority is the same, but the node uses
+* SCHED_FIFO, prefer SCHED_FIFO, since it can
+* run unbounded, unlike SCHED_RR.
+*/
+   desired_prio = node_prio;
+   }
+
+   binder_set_priority(task, desired_prio);
+}
+
 static struct binder_node *binder_get_node_ilocked(struct binder_proc *proc,
   binder_uintptr_t ptr)
 {
@@ -2633,11 +2668,15 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
 {
struct list_head *target_list = NULL;
struct binder_node *node = t->buffer->target_node;
+   struct binder_priority node_prio;
bool oneway = !!(t->flags & TF_ONE_WAY);
bool wakeup = true;
 
BUG_ON(!node);
binder_node_lock(node);
+   node_prio.prio = node->min_priority;
+   node_prio.sched_policy = node->sched_policy;
+
if (oneway) {
BUG_ON(thread);
if (node->has_async_transaction) {
@@ -2659,12 +2698,14 @@ static bool binder_proc_transaction(struct 
binder_transaction *t,
if (!thread && !target_list)
thread = binder_select_thread_ilocked(proc);
 
-   if (thread)
+   if (thread) {
target_list = >todo;
-   else if (!target_list)
+   binder_transaction_priority(thread->task, t, node_prio);
+   } else if (!target_list) {
target_list = >todo;
-   else
+   } else {
BUG_ON(target_list != >async_todo);
+   }
 
binder_enqueue_work_ilocked(>work, target_list);
 
@@ -2741,7 +2782,6 @@ static void binder_transaction(struct binder_proc *proc,
}
thread->transaction_stack = in_reply_to->to_parent;
binder_inner_proc_unlock(proc);
-   binder_set_priority(current, in_reply_to->saved_priority);
target_thread = binder_get_txn_from_and_acq_inner(in_reply_to);
if (target_thread == NULL) {
return_error = BR_DEAD_REPLY;
@@ -3151,6 +3191,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_enqueue_work_ilocked(>work, _thread->todo);
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
+   binder_set_priority(current, in_reply_to->saved_priority);
binder_free_transaction(i

[PATCH 04/13] ANDROID: binder: add min sched_policy to node.

[Martijn Coenen]

This change adds flags to flat_binder_object.flags
to allow indicating a minimum scheduling policy for
the node. It also clarifies the valid value range
for the priority bits in the flags.

Internally, we use the priority map that the kernel
uses, e.g. [0..99] for real-time policies and [100..139]
for the SCHED_NORMAL/SCHED_BATCH policies.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c| 26 ++-
 include/uapi/linux/android/binder.h | 41 -
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index bb61116d1c0c..247e913a6e6a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -352,6 +352,8 @@ struct binder_error {
  *and by @lock)
  * @has_async_transaction: async transaction to node in progress
  *(protected by @lock)
+ * @sched_policy: minimum scheduling policy for node
+ *(invariant after initialized)
  * @accept_fds:   file descriptor operations supported for node
  *(invariant after initialized)
  * @min_priority: minimum scheduling priority
@@ -391,6 +393,7 @@ struct binder_node {
/*
 * invariant after initialization
 */
+   u8 sched_policy:2;
u8 accept_fds:1;
u8 min_priority;
};
@@ -1207,6 +1210,7 @@ static struct binder_node *binder_init_node_ilocked(
binder_uintptr_t ptr = fp ? fp->binder : 0;
binder_uintptr_t cookie = fp ? fp->cookie : 0;
__u32 flags = fp ? fp->flags : 0;
+   s8 priority;
 
BUG_ON(!spin_is_locked(>inner_lock));
while (*p) {
@@ -1238,8 +1242,10 @@ static struct binder_node *binder_init_node_ilocked(
node->ptr = ptr;
node->cookie = cookie;
node->work.type = BINDER_WORK_NODE;
-   node->min_priority = NICE_TO_PRIO(
-   flags & FLAT_BINDER_FLAG_PRIORITY_MASK);
+   priority = flags & FLAT_BINDER_FLAG_PRIORITY_MASK;
+   node->sched_policy = (flags & FLAT_BINDER_FLAG_PRIORITY_MASK) >>
+   FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT;
+   node->min_priority = to_kernel_prio(node->sched_policy, priority);
node->accept_fds = !!(flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
spin_lock_init(>lock);
INIT_LIST_HEAD(>work.entry);
@@ -4050,8 +4056,17 @@ static int binder_thread_read(struct binder_proc *proc,
tr.cookie =  target_node->cookie;
t->saved_priority.sched_policy = current->policy;
t->saved_priority.prio = current->normal_prio;
-   if (target_node->min_priority < t->priority.prio) {
-   prio.sched_policy = SCHED_NORMAL;
+   if (target_node->min_priority < t->priority.prio ||
+   (target_node->min_priority == t->priority.prio &&
+target_node->sched_policy == SCHED_FIFO)) {
+   /*
+* In case the minimum priority on the node is
+* higher (lower value), use that priority. If
+* the priority is the same, but the node uses
+* SCHED_FIFO, prefer SCHED_FIFO, since it can
+* run unbounded, unlike SCHED_RR.
+*/
+   prio.sched_policy = target_node->sched_policy;
prio.prio = target_node->min_priority;
}
binder_set_priority(current, prio);
@@ -5090,8 +5105,9 @@ static void print_binder_node_nilocked(struct seq_file *m,
hlist_for_each_entry(ref, >refs, node_entry)
count++;
 
-   seq_printf(m, "  node %d: u%016llx c%016llx hs %d hw %d ls %d lw %d is 
%d iw %d tr %d",
+   seq_printf(m, "  node %d: u%016llx c%016llx pri %d:%d hs %d hw %d ls %d 
lw %d is %d iw %d tr %d",
   node->debug_id, (u64)node->ptr, (u64)node->cookie,
+  node->sched_policy, node->min_priority,
   node->has_strong_ref, node->has_weak_ref,
   node->local_strong_refs, node->local_weak_refs,
   node->internal_strong_refs, count, node->tmp_refs);
diff --git a/include/uapi/linux/android/binder.h 
b/include/uapi/linux/android/binder.h
index 7668b5791c91..026558ac254d 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -37,9 +37,48 @@ enum {
BINDER_TYPE_PTR = B_PAC

[PATCH 09/13] ANDROID: binder: Don't BUG_ON(!spin_is_locked()).

[Martijn Coenen]

Because is_spin_locked() always returns false on UP
systems.

Use assert_spin_locked() instead, and remove the
WARN_ON() instances, since those were easy to verify.

Signed-off-by: Martijn Coenen <m...@android.com>
---
 drivers/android/binder.c | 26 ++
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d6648b5f5988..eb82226be098 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1013,7 +1013,7 @@ binder_select_thread_ilocked(struct binder_proc *proc)
 {
struct binder_thread *thread;
 
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
thread = list_first_entry_or_null(>waiting_threads,
  struct binder_thread,
  waiting_thread_node);
@@ -1044,7 +1044,7 @@ static void binder_wakeup_thread_ilocked(struct 
binder_proc *proc,
 struct binder_thread *thread,
 bool sync)
 {
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
 
if (thread) {
if (sync)
@@ -1223,7 +1223,7 @@ static struct binder_node *binder_get_node_ilocked(struct 
binder_proc *proc,
struct rb_node *n = proc->nodes.rb_node;
struct binder_node *node;
 
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
 
while (n) {
node = rb_entry(n, struct binder_node, rb_node);
@@ -1269,7 +1269,8 @@ static struct binder_node *binder_init_node_ilocked(
__u32 flags = fp ? fp->flags : 0;
s8 priority;
 
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
+
while (*p) {
 
parent = *p;
@@ -1348,9 +1349,9 @@ static int binder_inc_node_nilocked(struct binder_node 
*node, int strong,
 {
struct binder_proc *proc = node->proc;
 
-   BUG_ON(!spin_is_locked(>lock));
+   assert_spin_locked(>lock);
if (proc)
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
if (strong) {
if (internal) {
if (target_list == NULL &&
@@ -1401,9 +1402,9 @@ static bool binder_dec_node_nilocked(struct binder_node 
*node,
 {
struct binder_proc *proc = node->proc;
 
-   BUG_ON(!spin_is_locked(>lock));
+   assert_spin_locked(>lock);
if (proc)
-   BUG_ON(!spin_is_locked(>inner_lock));
+   assert_spin_locked(>inner_lock);
if (strong) {
if (internal)
node->internal_strong_refs--;
@@ -1927,7 +1928,7 @@ static void binder_pop_transaction_ilocked(struct 
binder_thread *target_thread,
   struct binder_transaction *t)
 {
BUG_ON(!target_thread);
-   BUG_ON(!spin_is_locked(_thread->proc->inner_lock));
+   assert_spin_locked(_thread->proc->inner_lock);
BUG_ON(target_thread->transaction_stack != t);
BUG_ON(target_thread->transaction_stack->from != target_thread);
target_thread->transaction_stack =
@@ -5071,7 +5072,6 @@ static void print_binder_transaction_ilocked(struct 
seq_file *m,
struct binder_proc *to_proc;
struct binder_buffer *buffer = t->buffer;
 
-   WARN_ON(!spin_is_locked(>inner_lock));
spin_lock(>lock);
to_proc = t->to_proc;
seq_printf(m,
@@ -5160,7 +5160,6 @@ static void print_binder_thread_ilocked(struct seq_file 
*m,
size_t start_pos = m->count;
size_t header_pos;
 
-   WARN_ON(!spin_is_locked(>proc->inner_lock));
seq_printf(m, "  thread %d: l %02x need_return %d tr %d\n",
thread->pid, thread->looper,
thread->looper_need_return,
@@ -5197,10 +5196,6 @@ static void print_binder_node_nilocked(struct seq_file 
*m,
struct binder_work *w;
int count;
 
-   WARN_ON(!spin_is_locked(>lock));
-   if (node->proc)
-   WARN_ON(!spin_is_locked(>proc->inner_lock));
-
count = 0;
hlist_for_each_entry(ref, >refs, node_entry)
count++;
@@ -5227,7 +5222,6 @@ static void print_binder_node_nilocked(struct seq_file *m,
 static void print_binder_ref_olocked(struct seq_file *m,
 struct binder_ref *ref)
 {
-   WARN_ON(!spin_is_locked(>proc->outer_lock));
binder_node_lock(ref->node);
seq_printf(m, "  ref %d: desc %d %snode %d s %d w %d d %pK\n",
   ref->data.debug_id, ref->data.desc,
-- 
2.14.1.480.gb18f417b89-goog

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel