Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1678083 > unrolled thread

[PATCH 00/37] fine-grained locking in binder driver

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/37] fine-grained locking in binder driver Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 30/37] binder: protect proc->nodes with inner lock Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 13/37] binder: refactor queue management in binder_thread_read Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 26/37] binder: introduce locking helper functions Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 18/37] binder: add more debug info when allocation fails. Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 24/37] binder: refactor binder ref inc/dec for thread safety Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 15/37] binder: don't modify thread->looper from other threads Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 22/37] binder: make sure target_node has strong ref Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 27/37] binder: use inner lock to sync work dq and node counts Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 10/37] binder: change binder_stats to atomics Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 36/37] binder: fix death race conditions Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
      Re: [PATCH 36/37] binder: fix death race conditions Greg KH <gregkh@linuxfoundation.org> - 2017-06-30 08:10 +0200
    [PATCH 19/37] binder: use atomic for transaction_log index Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 03/37] binder: Use wake up hint for synchronous transactions. Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
      Re: [PATCH 03/37] binder: Use wake up hint for synchronous  transactions. Greg KH <gregkh@linuxfoundation.org> - 2017-07-03 11:20 +0200
    [PATCH 33/37] binder: use inner lock to protect thread accounting Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 31/37] binder: protect proc->threads with inner_lock Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 04/37] binder: separate binder allocator structure from binder proc Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 09/37] binder: add protection for non-perf cases Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 37/37] binder: remove global binder lock Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 02/37] binder: use group leader instead of open thread Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
      Re: [PATCH 02/37] binder: use group leader instead of open thread Greg KH <gregkh@linuxfoundation.org> - 2017-07-03 11:20 +0200
    [PATCH 12/37] binder: add log information for binder transaction failures Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
    [PATCH 08/37] binder: remove binder_debug_no_lock mechanism Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
    [PATCH 17/37] binder: protect against two threads freeing buffer Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
    [PATCH 05/37] binder: remove unneeded cleanup code Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
    [PATCH 21/37] binder: guarantee txn complete / errors delivered in-order Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
    [PATCH 11/37] binder: make binder_last_id an atomic Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
    [PATCH 16/37] binder: remove dead code in binder_get_ref_for_node Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
    [PATCH 14/37] binder: avoid race conditions when enqueuing txn Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
    Re: [PATCH 00/37] fine-grained locking in binder driver Greg KH <gregkh@linuxfoundation.org> - 2017-06-30 08:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1678105 — [PATCH 02/37] binder: use group leader instead of open thread

The binder allocator assumes that the thread that
called binder_open will never die for the lifetime of
that proc. That thread is normally the group_leader,
however it may not be. Use the group_leader instead
of current.

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 157bd3e49ff4..9393924ae8e8 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3460,8 +3460,8 @@ static int binder_open(struct inode *nodp, struct file *filp)
 	proc = kzalloc(sizeof(*proc), GFP_KERNEL);
 	if (proc == NULL)
 		return -ENOMEM;
-	get_task_struct(current);
-	proc->tsk = current;
+	get_task_struct(current->group_leader);
+	proc->tsk = current->group_leader;
 	INIT_LIST_HEAD(&proc->todo);
 	init_waitqueue_head(&proc->wait);
 	proc->default_priority = task_nice(current);
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1679811 — Re: [PATCH 02/37] binder: use group leader instead of open thread

On Thu, Jun 29, 2017 at 12:01:36PM -0700, Todd Kjos wrote:
> The binder allocator assumes that the thread that
> called binder_open will never die for the lifetime of
> that proc. That thread is normally the group_leader,
> however it may not be. Use the group_leader instead
> of current.
> 
> Signed-off-by: Todd Kjos <tkjos@google.com>
> ---
>  drivers/android/binder.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

This should be backported to older kernels, right?  How far back?  Same
for patch 01/37, right?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1678106 — [PATCH 12/37] binder: add log information for binder transaction failures

Add additional information to determine the cause of binder
failures. Adds the following to failed transaction log and
kernel messages:
	return_error : value returned for transaction
	return_error_param : errno returned by binder allocator
	return_error_line : line number where error detected

Also, return BR_DEAD_REPLY if an allocation error indicates
a dead proc (-ESRCH)

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c       | 87 +++++++++++++++++++++++++++++++++++++-----
 drivers/android/binder_alloc.c | 20 +++++-----
 2 files changed, 88 insertions(+), 19 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 25f30d81c7d0..62ac0c41b8a6 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -196,6 +196,9 @@ struct binder_transaction_log_entry {
 	int to_node;
 	int data_size;
 	int offsets_size;
+	int return_error_line;
+	uint32_t return_error;
+	uint32_t return_error_param;
 	const char *context_name;
 };
 struct binder_transaction_log {
@@ -1142,7 +1145,7 @@ static int binder_translate_binder(struct flat_binder_object *fp,
 
 	ref = binder_get_ref_for_node(target_proc, node);
 	if (!ref)
-		return -EINVAL;
+		return -ENOMEM;
 
 	if (fp->hdr.type == BINDER_TYPE_BINDER)
 		fp->hdr.type = BINDER_TYPE_HANDLE;
@@ -1199,7 +1202,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
 
 		new_ref = binder_get_ref_for_node(target_proc, ref->node);
 		if (!new_ref)
-			return -EINVAL;
+			return -ENOMEM;
 
 		fp->binder = 0;
 		fp->handle = new_ref->desc;
@@ -1397,7 +1400,9 @@ static void binder_transaction(struct binder_proc *proc,
 	wait_queue_head_t *target_wait;
 	struct binder_transaction *in_reply_to = NULL;
 	struct binder_transaction_log_entry *e;
-	uint32_t return_error;
+	uint32_t return_error = 0;
+	uint32_t return_error_param = 0;
+	uint32_t return_error_line = 0;
 	struct binder_buffer_object *last_fixup_obj = NULL;
 	binder_size_t last_fixup_min_off = 0;
 	struct binder_context *context = proc->context;
@@ -1417,6 +1422,8 @@ static void binder_transaction(struct binder_proc *proc,
 			binder_user_error("%d:%d got reply transaction with no transaction stack\n",
 					  proc->pid, thread->pid);
 			return_error = BR_FAILED_REPLY;
+			return_error_param = -EPROTO;
+			return_error_line = __LINE__;
 			goto err_empty_call_stack;
 		}
 		binder_set_nice(in_reply_to->saved_priority);
@@ -1428,6 +1435,8 @@ static void binder_transaction(struct binder_proc *proc,
 				in_reply_to->to_thread ?
 				in_reply_to->to_thread->pid : 0);
 			return_error = BR_FAILED_REPLY;
+			return_error_param = -EPROTO;
+			return_error_line = __LINE__;
 			in_reply_to = NULL;
 			goto err_bad_call_stack;
 		}
@@ -1435,6 +1444,7 @@ static void binder_transaction(struct binder_proc *proc,
 		target_thread = in_reply_to->from;
 		if (target_thread == NULL) {
 			return_error = BR_DEAD_REPLY;
+			return_error_line = __LINE__;
 			goto err_dead_binder;
 		}
 		if (target_thread->transaction_stack != in_reply_to) {
@@ -1444,6 +1454,8 @@ static void binder_transaction(struct binder_proc *proc,
 				target_thread->transaction_stack->debug_id : 0,
 				in_reply_to->debug_id);
 			return_error = BR_FAILED_REPLY;
+			return_error_param = -EPROTO;
+			return_error_line = __LINE__;
 			in_reply_to = NULL;
 			target_thread = NULL;
 			goto err_dead_binder;
@@ -1458,6 +1470,8 @@ static void binder_transaction(struct binder_proc *proc,
 				binder_user_error("%d:%d got transaction to invalid handle\n",
 					proc->pid, thread->pid);
 				return_error = BR_FAILED_REPLY;
+				return_error_param = -EINVAL;
+				return_error_line = __LINE__;
 				goto err_invalid_target_handle;
 			}
 			target_node = ref->node;
@@ -1467,6 +1481,7 @@ static void binder_transaction(struct binder_proc *proc,
 			if (target_node == NULL) {
 				return_error = BR_DEAD_REPLY;
 				mutex_unlock(&context->context_mgr_node_lock);
+				return_error_line = __LINE__;
 				goto err_no_context_mgr_node;
 			}
 			mutex_unlock(&context->context_mgr_node_lock);
@@ -1475,11 +1490,14 @@ static void binder_transaction(struct binder_proc *proc,
 		target_proc = target_node->proc;
 		if (target_proc == NULL) {
 			return_error = BR_DEAD_REPLY;
+			return_error_line = __LINE__;
 			goto err_dead_binder;
 		}
 		if (security_binder_transaction(proc->tsk,
 						target_proc->tsk) < 0) {
 			return_error = BR_FAILED_REPLY;
+			return_error_param = -EPERM;
+			return_error_line = __LINE__;
 			goto err_invalid_target_handle;
 		}
 		if (!(tr->flags & TF_ONE_WAY) && thread->transaction_stack) {
@@ -1493,6 +1511,8 @@ static void binder_transaction(struct binder_proc *proc,
 					tmp->to_thread ?
 					tmp->to_thread->pid : 0);
 				return_error = BR_FAILED_REPLY;
+				return_error_param = -EPROTO;
+				return_error_line = __LINE__;
 				goto err_bad_call_stack;
 			}
 			while (tmp) {
@@ -1516,6 +1536,8 @@ static void binder_transaction(struct binder_proc *proc,
 	t = kzalloc(sizeof(*t), GFP_KERNEL);
 	if (t == NULL) {
 		return_error = BR_FAILED_REPLY;
+		return_error_param = -ENOMEM;
+		return_error_line = __LINE__;
 		goto err_alloc_t_failed;
 	}
 	binder_stats_created(BINDER_STAT_TRANSACTION);
@@ -1523,6 +1545,8 @@ static void binder_transaction(struct binder_proc *proc,
 	tcomplete = kzalloc(sizeof(*tcomplete), GFP_KERNEL);
 	if (tcomplete == NULL) {
 		return_error = BR_FAILED_REPLY;
+		return_error_param = -ENOMEM;
+		return_error_line = __LINE__;
 		goto err_alloc_tcomplete_failed;
 	}
 	binder_stats_created(BINDER_STAT_TRANSACTION_COMPLETE);
@@ -1565,8 +1589,15 @@ static void binder_transaction(struct binder_proc *proc,
 	t->buffer = binder_alloc_new_buf(&target_proc->alloc, tr->data_size,
 		tr->offsets_size, extra_buffers_size,
 		!reply && (t->flags & TF_ONE_WAY));
-	if (t->buffer == NULL) {
-		return_error = BR_FAILED_REPLY;
+	if (IS_ERR(t->buffer)) {
+		/*
+		 * -ESRCH indicates VMA cleared. The target is dying.
+		 */
+		return_error_param = PTR_ERR(t->buffer);
+		return_error = return_error_param == -ESRCH ?
+			BR_DEAD_REPLY : BR_FAILED_REPLY;
+		return_error_line = __LINE__;
+		t->buffer = NULL;
 		goto err_binder_alloc_buf_failed;
 	}
 	t->buffer->allow_user_free = 0;
@@ -1586,6 +1617,8 @@ static void binder_transaction(struct binder_proc *proc,
 		binder_user_error("%d:%d got transaction with invalid data ptr\n",
 				proc->pid, thread->pid);
 		return_error = BR_FAILED_REPLY;
+		return_error_param = -EFAULT;
+		return_error_line = __LINE__;
 		goto err_copy_data_failed;
 	}
 	if (copy_from_user(offp, (const void __user *)(uintptr_t)
@@ -1593,12 +1626,16 @@ static void binder_transaction(struct binder_proc *proc,
 		binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
 				proc->pid, thread->pid);
 		return_error = BR_FAILED_REPLY;
+		return_error_param = -EFAULT;
+		return_error_line = __LINE__;
 		goto err_copy_data_failed;
 	}
 	if (!IS_ALIGNED(tr->offsets_size, sizeof(binder_size_t))) {
 		binder_user_error("%d:%d got transaction with invalid offsets size, %lld\n",
 				proc->pid, thread->pid, (u64)tr->offsets_size);
 		return_error = BR_FAILED_REPLY;
+		return_error_param = -EINVAL;
+		return_error_line = __LINE__;
 		goto err_bad_offset;
 	}
 	if (!IS_ALIGNED(extra_buffers_size, sizeof(u64))) {
@@ -1606,6 +1643,8 @@ static void binder_transaction(struct binder_proc *proc,
 				  proc->pid, thread->pid,
 				  (u64)extra_buffers_size);
 		return_error = BR_FAILED_REPLY;
+		return_error_param = -EINVAL;
+		return_error_line = __LINE__;
 		goto err_bad_offset;
 	}
 	off_end = (void *)off_start + tr->offsets_size;
@@ -1622,6 +1661,8 @@ static void binder_transaction(struct binder_proc *proc,
 					  (u64)off_min,
 					  (u64)t->buffer->data_size);
 			return_error = BR_FAILED_REPLY;
+			return_error_param = -EINVAL;
+			return_error_line = __LINE__;
 			goto err_bad_offset;
 		}
 
@@ -1636,6 +1677,8 @@ static void binder_transaction(struct binder_proc *proc,
 			ret = binder_translate_binder(fp, t, thread);
 			if (ret < 0) {
 				return_error = BR_FAILED_REPLY;
+				return_error_param = ret;
+				return_error_line = __LINE__;
 				goto err_translate_failed;
 			}
 		} break;
@@ -1647,6 +1690,8 @@ static void binder_transaction(struct binder_proc *proc,
 			ret = binder_translate_handle(fp, t, thread);
 			if (ret < 0) {
 				return_error = BR_FAILED_REPLY;
+				return_error_param = ret;
+				return_error_line = __LINE__;
 				goto err_translate_failed;
 			}
 		} break;
@@ -1658,6 +1703,8 @@ static void binder_transaction(struct binder_proc *proc,
 
 			if (target_fd < 0) {
 				return_error = BR_FAILED_REPLY;
+				return_error_param = target_fd;
+				return_error_line = __LINE__;
 				goto err_translate_failed;
 			}
 			fp->pad_binder = 0;
@@ -1674,6 +1721,8 @@ static void binder_transaction(struct binder_proc *proc,
 				binder_user_error("%d:%d got transaction with invalid parent offset or type\n",
 						  proc->pid, thread->pid);
 				return_error = BR_FAILED_REPLY;
+				return_error_param = -EINVAL;
+				return_error_line = __LINE__;
 				goto err_bad_parent;
 			}
 			if (!binder_validate_fixup(t->buffer, off_start,
@@ -1683,12 +1732,16 @@ static void binder_transaction(struct binder_proc *proc,
 				binder_user_error("%d:%d got transaction with out-of-order buffer fixup\n",
 						  proc->pid, thread->pid);
 				return_error = BR_FAILED_REPLY;
+				return_error_param = -EINVAL;
+				return_error_line = __LINE__;
 				goto err_bad_parent;
 			}
 			ret = binder_translate_fd_array(fda, parent, t, thread,
 							in_reply_to);
 			if (ret < 0) {
 				return_error = BR_FAILED_REPLY;
+				return_error_param = ret;
+				return_error_line = __LINE__;
 				goto err_translate_failed;
 			}
 			last_fixup_obj = parent;
@@ -1704,6 +1757,8 @@ static void binder_transaction(struct binder_proc *proc,
 				binder_user_error("%d:%d got transaction with too large buffer\n",
 						  proc->pid, thread->pid);
 				return_error = BR_FAILED_REPLY;
+				return_error_param = -EINVAL;
+				return_error_line = __LINE__;
 				goto err_bad_offset;
 			}
 			if (copy_from_user(sg_bufp,
@@ -1711,7 +1766,9 @@ static void binder_transaction(struct binder_proc *proc,
 					   bp->buffer, bp->length)) {
 				binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
 						  proc->pid, thread->pid);
+				return_error_param = -EFAULT;
 				return_error = BR_FAILED_REPLY;
+				return_error_line = __LINE__;
 				goto err_copy_data_failed;
 			}
 			/* Fixup buffer pointer to target proc address space */
@@ -1726,6 +1783,8 @@ static void binder_transaction(struct binder_proc *proc,
 						  last_fixup_min_off);
 			if (ret < 0) {
 				return_error = BR_FAILED_REPLY;
+				return_error_param = ret;
+				return_error_line = __LINE__;
 				goto err_translate_failed;
 			}
 			last_fixup_obj = bp;
@@ -1735,6 +1794,8 @@ static void binder_transaction(struct binder_proc *proc,
 			binder_user_error("%d:%d got transaction with invalid object type, %x\n",
 				proc->pid, thread->pid, hdr->type);
 			return_error = BR_FAILED_REPLY;
+			return_error_param = -EINVAL;
+			return_error_line = __LINE__;
 			goto err_bad_object_type;
 		}
 	}
@@ -1789,13 +1850,17 @@ static void binder_transaction(struct binder_proc *proc,
 err_invalid_target_handle:
 err_no_context_mgr_node:
 	binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
-		     "%d:%d transaction failed %d, size %lld-%lld\n",
-		     proc->pid, thread->pid, return_error,
-		     (u64)tr->data_size, (u64)tr->offsets_size);
+		     "%d:%d transaction failed %d/%d, size %lld-%lld line %d\n",
+		     proc->pid, thread->pid, return_error, return_error_param,
+		     (u64)tr->data_size, (u64)tr->offsets_size,
+		     return_error_line);
 
 	{
 		struct binder_transaction_log_entry *fe;
 
+		e->return_error = return_error;
+		e->return_error_param = return_error_param;
+		e->return_error_line = return_error_line;
 		fe = binder_transaction_log_add(&binder_transaction_log_failed);
 		*fe = *e;
 	}
@@ -3622,11 +3687,13 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
 					struct binder_transaction_log_entry *e)
 {
 	seq_printf(m,
-		   "%d: %s from %d:%d to %d:%d context %s node %d handle %d size %d:%d\n",
+		   "%d: %s from %d:%d to %d:%d context %s node %d handle %d size %d:%d ret %d/%d l=%d\n",
 		   e->debug_id, (e->call_type == 2) ? "reply" :
 		   ((e->call_type == 1) ? "async" : "call "), e->from_proc,
 		   e->from_thread, e->to_proc, e->to_thread, e->context_name,
-		   e->to_node, e->target_handle, e->data_size, e->offsets_size);
+		   e->to_node, e->target_handle, e->data_size, e->offsets_size,
+		   e->return_error, e->return_error_param,
+		   e->return_error_line);
 }
 
 static int binder_transaction_log_show(struct seq_file *m, void *unused)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 198d04c5d958..a0af1419cc79 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -262,7 +262,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		up_write(&mm->mmap_sem);
 		mmput(mm);
 	}
-	return -ENOMEM;
+	return vma ? -ENOMEM : -ESRCH;
 }
 
 struct binder_buffer *binder_alloc_new_buf_locked(struct binder_alloc *alloc,
@@ -278,11 +278,12 @@ struct binder_buffer *binder_alloc_new_buf_locked(struct binder_alloc *alloc,
 	void *has_page_addr;
 	void *end_page_addr;
 	size_t size, data_offsets_size;
+	int ret;
 
 	if (alloc->vma == NULL) {
 		pr_err("%d: binder_alloc_buf, no vma\n",
 		       alloc->pid);
-		return NULL;
+		return ERR_PTR(-ESRCH);
 	}
 
 	data_offsets_size = ALIGN(data_size, sizeof(void *)) +
@@ -292,21 +293,21 @@ struct binder_buffer *binder_alloc_new_buf_locked(struct binder_alloc *alloc,
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
 				"%d: got transaction with invalid size %zd-%zd\n",
 				alloc->pid, data_size, offsets_size);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 	size = data_offsets_size + ALIGN(extra_buffers_size, sizeof(void *));
 	if (size < data_offsets_size || size < extra_buffers_size) {
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
 				"%d: got transaction with invalid extra_buffers_size %zd\n",
 				alloc->pid, extra_buffers_size);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 	if (is_async &&
 	    alloc->free_async_space < size + sizeof(struct binder_buffer)) {
 		binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
 			     "%d: binder_alloc_buf size %zd failed, no async space left\n",
 			      alloc->pid, size);
-		return NULL;
+		return ERR_PTR(-ENOSPC);
 	}
 
 	while (n) {
@@ -327,7 +328,7 @@ struct binder_buffer *binder_alloc_new_buf_locked(struct binder_alloc *alloc,
 	if (best_fit == NULL) {
 		pr_err("%d: binder_alloc_buf size %zd failed, no address space\n",
 			alloc->pid, size);
-		return NULL;
+		return ERR_PTR(-ENOSPC);
 	}
 	if (n == NULL) {
 		buffer = rb_entry(best_fit, struct binder_buffer, rb_node);
@@ -350,9 +351,10 @@ struct binder_buffer *binder_alloc_new_buf_locked(struct binder_alloc *alloc,
 		(void *)PAGE_ALIGN((uintptr_t)buffer->data + buffer_size);
 	if (end_page_addr > has_page_addr)
 		end_page_addr = has_page_addr;
-	if (binder_update_page_range(alloc, 1,
-	    (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr, NULL))
-		return NULL;
+	ret = binder_update_page_range(alloc, 1,
+	    (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr, NULL);
+	if (ret)
+		return ERR_PTR(ret);
 
 	rb_erase(best_fit, &alloc->free_buffers);
 	buffer->free = 0;
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678107 — [PATCH 08/37] binder: remove binder_debug_no_lock mechanism

With the global lock, there was a mechanism to access
binder driver debugging information with the global
lock disabled to debug deadlocks or other issues.
This mechanism is rarely (if ever) used anymore
and wasn't needed during the development of
fine-grained locking in the binder driver.
Removing it.

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 0512971cfc53..10fda7ab9fa5 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -107,9 +107,6 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR |
 	BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
 module_param_named(debug_mask, binder_debug_mask, uint, S_IWUSR | S_IRUGO);
 
-static bool binder_debug_no_lock;
-module_param_named(proc_no_lock, binder_debug_no_lock, bool, S_IWUSR | S_IRUGO);
-
 static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
 module_param_named(devices, binder_devices_param, charp, 0444);
 
@@ -3507,10 +3504,8 @@ static int binder_state_show(struct seq_file *m, void *unused)
 {
 	struct binder_proc *proc;
 	struct binder_node *node;
-	int do_lock = !binder_debug_no_lock;
 
-	if (do_lock)
-		binder_lock(__func__);
+	binder_lock(__func__);
 
 	seq_puts(m, "binder state:\n");
 
@@ -3521,18 +3516,15 @@ static int binder_state_show(struct seq_file *m, void *unused)
 
 	hlist_for_each_entry(proc, &binder_procs, proc_node)
 		print_binder_proc(m, proc, 1);
-	if (do_lock)
-		binder_unlock(__func__);
+	binder_unlock(__func__);
 	return 0;
 }
 
 static int binder_stats_show(struct seq_file *m, void *unused)
 {
 	struct binder_proc *proc;
-	int do_lock = !binder_debug_no_lock;
 
-	if (do_lock)
-		binder_lock(__func__);
+	binder_lock(__func__);
 
 	seq_puts(m, "binder stats:\n");
 
@@ -3540,24 +3532,20 @@ static int binder_stats_show(struct seq_file *m, void *unused)
 
 	hlist_for_each_entry(proc, &binder_procs, proc_node)
 		print_binder_proc_stats(m, proc);
-	if (do_lock)
-		binder_unlock(__func__);
+	binder_unlock(__func__);
 	return 0;
 }
 
 static int binder_transactions_show(struct seq_file *m, void *unused)
 {
 	struct binder_proc *proc;
-	int do_lock = !binder_debug_no_lock;
 
-	if (do_lock)
-		binder_lock(__func__);
+	binder_lock(__func__);
 
 	seq_puts(m, "binder transactions:\n");
 	hlist_for_each_entry(proc, &binder_procs, proc_node)
 		print_binder_proc(m, proc, 0);
-	if (do_lock)
-		binder_unlock(__func__);
+	binder_unlock(__func__);
 	return 0;
 }
 
@@ -3565,10 +3553,8 @@ static int binder_proc_show(struct seq_file *m, void *unused)
 {
 	struct binder_proc *itr;
 	int pid = (unsigned long)m->private;
-	int do_lock = !binder_debug_no_lock;
 
-	if (do_lock)
-		binder_lock(__func__);
+	binder_lock(__func__);
 
 	hlist_for_each_entry(itr, &binder_procs, proc_node) {
 		if (itr->pid == pid) {
@@ -3576,8 +3562,7 @@ static int binder_proc_show(struct seq_file *m, void *unused)
 			print_binder_proc(m, itr, 1);
 		}
 	}
-	if (do_lock)
-		binder_unlock(__func__);
+	binder_unlock(__func__);
 	return 0;
 }
 
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678108 — [PATCH 17/37] binder: protect against two threads freeing buffer

Adds protection against malicious user code freeing
the same buffer at the same time which could cause
a crash. Cannot happen under normal use.

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c       |  4 ++--
 drivers/android/binder_alloc.c | 22 +++++++++++++++++-----
 drivers/android/binder_alloc.h |  7 ++++---
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 3bbfb2455b70..a1912a22c89c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2024,8 +2024,8 @@ static int binder_thread_write(struct binder_proc *proc,
 				return -EFAULT;
 			ptr += sizeof(binder_uintptr_t);
 
-			buffer = binder_alloc_buffer_lookup(&proc->alloc,
-							    data_ptr);
+			buffer = binder_alloc_prepare_to_free(&proc->alloc,
+							      data_ptr);
 			if (buffer == NULL) {
 				binder_user_error("%d:%d BC_FREE_BUFFER u%016llx no match\n",
 					proc->pid, thread->pid, (u64)data_ptr);
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index a0af1419cc79..2a2e41b13de5 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -116,7 +116,7 @@ static void binder_insert_allocated_buffer_locked(
 	rb_insert_color(&new_buffer->rb_node, &alloc->allocated_buffers);
 }
 
-static struct binder_buffer *binder_alloc_buffer_lookup_locked(
+static struct binder_buffer *binder_alloc_prepare_to_free_locked(
 		struct binder_alloc *alloc,
 		uintptr_t user_ptr)
 {
@@ -135,8 +135,19 @@ static struct binder_buffer *binder_alloc_buffer_lookup_locked(
 			n = n->rb_left;
 		else if (kern_ptr > buffer)
 			n = n->rb_right;
-		else
+		else {
+			/*
+			 * Guard against user threads attempting to
+			 * free the buffer twice
+			 */
+			if (buffer->free_in_progress) {
+				pr_err("%d:%d FREE_BUFFER u%016llx user freed buffer twice\n",
+				       alloc->pid, current->pid, (u64)user_ptr);
+				return NULL;
+			}
+			buffer->free_in_progress = 1;
 			return buffer;
+		}
 	}
 	return NULL;
 }
@@ -152,13 +163,13 @@ static struct binder_buffer *binder_alloc_buffer_lookup_locked(
  *
  * Return:	Pointer to buffer or NULL
  */
-struct binder_buffer *binder_alloc_buffer_lookup(struct binder_alloc *alloc,
-						 uintptr_t user_ptr)
+struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
+						   uintptr_t user_ptr)
 {
 	struct binder_buffer *buffer;
 
 	mutex_lock(&alloc->mutex);
-	buffer = binder_alloc_buffer_lookup_locked(alloc, user_ptr);
+	buffer = binder_alloc_prepare_to_free_locked(alloc, user_ptr);
 	mutex_unlock(&alloc->mutex);
 	return buffer;
 }
@@ -358,6 +369,7 @@ struct binder_buffer *binder_alloc_new_buf_locked(struct binder_alloc *alloc,
 
 	rb_erase(best_fit, &alloc->free_buffers);
 	buffer->free = 0;
+	buffer->free_in_progress = 0;
 	binder_insert_allocated_buffer_locked(alloc, buffer);
 	if (buffer_size != size) {
 		struct binder_buffer *new_buffer = (void *)buffer->data + size;
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 721c511431f9..088e4ffc6230 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -48,7 +48,8 @@ struct binder_buffer {
 	unsigned free:1;
 	unsigned allow_user_free:1;
 	unsigned async_transaction:1;
-	unsigned debug_id:29;
+	unsigned free_in_progress:1;
+	unsigned debug_id:28;
 
 	struct binder_transaction *transaction;
 
@@ -109,8 +110,8 @@ extern struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
 extern void binder_alloc_init(struct binder_alloc *alloc);
 extern void binder_alloc_vma_close(struct binder_alloc *alloc);
 extern struct binder_buffer *
-binder_alloc_buffer_lookup(struct binder_alloc *alloc,
-			   uintptr_t user_ptr);
+binder_alloc_prepare_to_free(struct binder_alloc *alloc,
+			     uintptr_t user_ptr);
 extern void binder_alloc_free_buf(struct binder_alloc *alloc,
 				  struct binder_buffer *buffer);
 extern int binder_alloc_mmap_handler(struct binder_alloc *alloc,
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678110 — [PATCH 05/37] binder: remove unneeded cleanup code

The buffer's transaction has already been freed before
binder_deferred_release. No need to do it again.

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 1097d056ea6b..2cbd7558fc3f 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3631,7 +3631,6 @@ static int binder_node_release(struct binder_node *node, int refs)
 
 static void binder_deferred_release(struct binder_proc *proc)
 {
-	struct binder_transaction *t;
 	struct binder_context *context = proc->context;
 	struct rb_node *n;
 	int threads, nodes, incoming_refs, outgoing_refs, buffers,
@@ -3689,14 +3688,8 @@ static void binder_deferred_release(struct binder_proc *proc)
 
 		buffer = rb_entry(n, struct binder_buffer, rb_node);
 
-		t = buffer->transaction;
-		if (t) {
-			t->buffer = NULL;
-			buffer->transaction = NULL;
-			pr_err("release proc %d, transaction %d, not freed\n",
-			       proc->pid, t->debug_id);
-			/*BUG();*/
-		}
+		/* Transaction should already have been freed */
+		BUG_ON(buffer->transaction);
 
 		binder_free_buf(proc, buffer);
 		buffers++;
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678113 — [PATCH 21/37] binder: guarantee txn complete / errors delivered in-order

Since errors are tracked in the return_error/return_error2
fields of the binder_thread object and BR_TRANSACTION_COMPLETEs
can be tracked either in those fields or via the thread todo
work list, it is possible for errors to be reported ahead
of the associated txn complete.

Use the thread todo work list for errors to guarantee
order. Also changed binder_send_failed_reply to pop
the transaction even if it failed to send a reply.

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 127 +++++++++++++++++++++++++++--------------------
 1 file changed, 73 insertions(+), 54 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d2fcf3cc29a6..84a57dd7b973 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -249,6 +249,7 @@ struct binder_work {
 	enum {
 		BINDER_WORK_TRANSACTION = 1,
 		BINDER_WORK_TRANSACTION_COMPLETE,
+		BINDER_WORK_RETURN_ERROR,
 		BINDER_WORK_NODE,
 		BINDER_WORK_DEAD_BINDER,
 		BINDER_WORK_DEAD_BINDER_AND_CLEAR,
@@ -256,6 +257,11 @@ struct binder_work {
 	} type;
 };
 
+struct binder_error {
+	struct binder_work work;
+	uint32_t cmd;
+};
+
 struct binder_node {
 	int debug_id;
 	struct binder_work work;
@@ -350,10 +356,8 @@ struct binder_thread {
 	bool looper_need_return; /* can be written by other thread */
 	struct binder_transaction *transaction_stack;
 	struct list_head todo;
-	uint32_t return_error; /* Write failed, return error code in read buf */
-	uint32_t return_error2; /* Write failed, return error code in read */
-		/* buffer. Used when sending a reply to a dead process that */
-		/* we are also waiting on */
+	struct binder_error return_error;
+	struct binder_error reply_error;
 	wait_queue_head_t wait;
 	struct binder_stats stats;
 };
@@ -794,29 +798,24 @@ static void binder_send_failed_reply(struct binder_transaction *t,
 	while (1) {
 		target_thread = t->from;
 		if (target_thread) {
-			if (target_thread->return_error != BR_OK &&
-			   target_thread->return_error2 == BR_OK) {
-				target_thread->return_error2 =
-					target_thread->return_error;
-				target_thread->return_error = BR_OK;
-			}
-			if (target_thread->return_error == BR_OK) {
-				binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
-					     "send failed reply for transaction %d to %d:%d\n",
-					      t->debug_id,
-					      target_thread->proc->pid,
-					      target_thread->pid);
-
-				binder_pop_transaction(target_thread, t);
-				target_thread->return_error = error_code;
+			binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+				     "send failed reply for transaction %d to %d:%d\n",
+				      t->debug_id,
+				      target_thread->proc->pid,
+				      target_thread->pid);
+
+			binder_pop_transaction(target_thread, t);
+			if (target_thread->reply_error.cmd == BR_OK) {
+				target_thread->reply_error.cmd = error_code;
+				list_add_tail(
+					&target_thread->reply_error.work.entry,
+					&target_thread->todo);
 				wake_up_interruptible(&target_thread->wait);
-				binder_free_transaction(t);
 			} else {
-				pr_err("reply failed, target thread, %d:%d, has error code %d already\n",
-					target_thread->proc->pid,
-					target_thread->pid,
-					target_thread->return_error);
+				WARN(1, "Unexpected reply error: %u\n",
+						target_thread->reply_error.cmd);
 			}
+			binder_free_transaction(t);
 			return;
 		}
 		next = t->from_parent;
@@ -1884,12 +1883,17 @@ static void binder_transaction(struct binder_proc *proc,
 		WRITE_ONCE(fe->debug_id_done, t_debug_id);
 	}
 
-	BUG_ON(thread->return_error != BR_OK);
+	BUG_ON(thread->return_error.cmd != BR_OK);
 	if (in_reply_to) {
-		thread->return_error = BR_TRANSACTION_COMPLETE;
+		thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
+		list_add_tail(&thread->return_error.work.entry,
+			      &thread->todo);
 		binder_send_failed_reply(in_reply_to, return_error);
-	} else
-		thread->return_error = return_error;
+	} else {
+		thread->return_error.cmd = return_error;
+		list_add_tail(&thread->return_error.work.entry,
+			      &thread->todo);
+	}
 }
 
 static int binder_thread_write(struct binder_proc *proc,
@@ -1903,7 +1907,7 @@ static int binder_thread_write(struct binder_proc *proc,
 	void __user *ptr = buffer + *consumed;
 	void __user *end = buffer + size;
 
-	while (ptr < end && thread->return_error == BR_OK) {
+	while (ptr < end && thread->return_error.cmd == BR_OK) {
 		if (get_user(cmd, (uint32_t __user *)ptr))
 			return -EFAULT;
 		ptr += sizeof(uint32_t);
@@ -2183,7 +2187,12 @@ static int binder_thread_write(struct binder_proc *proc,
 				}
 				death = kzalloc(sizeof(*death), GFP_KERNEL);
 				if (death == NULL) {
-					thread->return_error = BR_ERROR;
+					WARN_ON(thread->return_error.cmd !=
+						BR_OK);
+					thread->return_error.cmd = BR_ERROR;
+					list_add_tail(
+					    &thread->return_error.work.entry,
+					    &thread->todo);
 					binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
 						     "%d:%d BC_REQUEST_DEATH_NOTIFICATION failed\n",
 						     proc->pid, thread->pid);
@@ -2299,8 +2308,7 @@ static int binder_has_proc_work(struct binder_proc *proc,
 
 static int binder_has_thread_work(struct binder_thread *thread)
 {
-	return !list_empty(&thread->todo) || thread->return_error != BR_OK ||
-		thread->looper_need_return;
+	return !list_empty(&thread->todo) || thread->looper_need_return;
 }
 
 static int binder_put_node_cmd(struct binder_proc *proc,
@@ -2356,25 +2364,6 @@ static int binder_thread_read(struct binder_proc *proc,
 	wait_for_proc_work = thread->transaction_stack == NULL &&
 				list_empty(&thread->todo);
 
-	if (thread->return_error != BR_OK && ptr < end) {
-		if (thread->return_error2 != BR_OK) {
-			if (put_user(thread->return_error2, (uint32_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(uint32_t);
-			binder_stat_br(proc, thread, thread->return_error2);
-			if (ptr == end)
-				goto done;
-			thread->return_error2 = BR_OK;
-		}
-		if (put_user(thread->return_error, (uint32_t __user *)ptr))
-			return -EFAULT;
-		ptr += sizeof(uint32_t);
-		binder_stat_br(proc, thread, thread->return_error);
-		thread->return_error = BR_OK;
-		goto done;
-	}
-
-
 	thread->looper |= BINDER_LOOPER_STATE_WAITING;
 	if (wait_for_proc_work)
 		proc->ready_threads++;
@@ -2441,6 +2430,19 @@ static int binder_thread_read(struct binder_proc *proc,
 		case BINDER_WORK_TRANSACTION: {
 			t = container_of(w, struct binder_transaction, work);
 		} break;
+		case BINDER_WORK_RETURN_ERROR: {
+			struct binder_error *e = container_of(
+					w, struct binder_error, work);
+
+			WARN_ON(e->cmd == BR_OK);
+			if (put_user(e->cmd, (uint32_t __user *)ptr))
+				return -EFAULT;
+			e->cmd = BR_OK;
+			ptr += sizeof(uint32_t);
+
+			binder_stat_br(proc, thread, cmd);
+			list_del(&w->entry);
+		} break;
 		case BINDER_WORK_TRANSACTION_COMPLETE: {
 			cmd = BR_TRANSACTION_COMPLETE;
 			if (put_user(cmd, (uint32_t __user *)ptr))
@@ -2685,6 +2687,14 @@ static void binder_release_work(struct list_head *list)
 				binder_free_transaction(t);
 			}
 		} break;
+		case BINDER_WORK_RETURN_ERROR: {
+			struct binder_error *e = container_of(
+					w, struct binder_error, work);
+
+			binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+				"undelivered TRANSACTION_ERROR: %u\n",
+				e->cmd);
+		} break;
 		case BINDER_WORK_TRANSACTION_COMPLETE: {
 			binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
 				"undelivered TRANSACTION_COMPLETE\n");
@@ -2740,8 +2750,10 @@ static struct binder_thread *binder_get_thread(struct binder_proc *proc)
 		rb_link_node(&thread->rb_node, parent, p);
 		rb_insert_color(&thread->rb_node, &proc->threads);
 		thread->looper_need_return = true;
-		thread->return_error = BR_OK;
-		thread->return_error2 = BR_OK;
+		thread->return_error.work.type = BINDER_WORK_RETURN_ERROR;
+		thread->return_error.cmd = BR_OK;
+		thread->reply_error.work.type = BINDER_WORK_RETURN_ERROR;
+		thread->reply_error.cmd = BR_OK;
 	}
 	return thread;
 }
@@ -2799,7 +2811,7 @@ static unsigned int binder_poll(struct file *filp,
 	thread = binder_get_thread(proc);
 
 	wait_for_proc_work = thread->transaction_stack == NULL &&
-		list_empty(&thread->todo) && thread->return_error == BR_OK;
+		list_empty(&thread->todo);
 
 	binder_unlock(__func__);
 
@@ -3378,6 +3390,13 @@ static void print_binder_work(struct seq_file *m, const char *prefix,
 		t = container_of(w, struct binder_transaction, work);
 		print_binder_transaction(m, transaction_prefix, t);
 		break;
+	case BINDER_WORK_RETURN_ERROR: {
+		struct binder_error *e = container_of(
+				w, struct binder_error, work);
+
+		seq_printf(m, "%stransaction error: %u\n",
+			   prefix, e->cmd);
+	} break;
 	case BINDER_WORK_TRANSACTION_COMPLETE:
 		seq_printf(m, "%stransaction complete\n", prefix);
 		break;
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678114 — [PATCH 11/37] binder: make binder_last_id an atomic

Use an atomic for binder_last_id to avoid locking it

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cd4191d027e1..25f30d81c7d0 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -59,7 +59,7 @@ static DEFINE_SPINLOCK(binder_dead_nodes_lock);
 
 static struct dentry *binder_debugfs_dir_entry_root;
 static struct dentry *binder_debugfs_dir_entry_proc;
-static int binder_last_id;
+static atomic_t binder_last_id;
 
 #define BINDER_DEBUG_ENTRY(name) \
 static int binder_##name##_open(struct inode *inode, struct file *file) \
@@ -496,7 +496,7 @@ static struct binder_node *binder_new_node(struct binder_proc *proc,
 	binder_stats_created(BINDER_STAT_NODE);
 	rb_link_node(&node->rb_node, parent, p);
 	rb_insert_color(&node->rb_node, &proc->nodes);
-	node->debug_id = ++binder_last_id;
+	node->debug_id = atomic_inc_return(&binder_last_id);
 	node->proc = proc;
 	node->ptr = ptr;
 	node->cookie = cookie;
@@ -639,7 +639,7 @@ static struct binder_ref *binder_get_ref_for_node(struct binder_proc *proc,
 	if (new_ref == NULL)
 		return NULL;
 	binder_stats_created(BINDER_STAT_REF);
-	new_ref->debug_id = ++binder_last_id;
+	new_ref->debug_id = atomic_inc_return(&binder_last_id);
 	new_ref->proc = proc;
 	new_ref->node = node;
 	rb_link_node(&new_ref->rb_node_node, parent, p);
@@ -1527,7 +1527,7 @@ static void binder_transaction(struct binder_proc *proc,
 	}
 	binder_stats_created(BINDER_STAT_TRANSACTION_COMPLETE);
 
-	t->debug_id = ++binder_last_id;
+	t->debug_id = atomic_inc_return(&binder_last_id);
 	e->debug_id = t->debug_id;
 
 	if (reply)
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678115 — [PATCH 16/37] binder: remove dead code in binder_get_ref_for_node

node is always non-NULL in binder_get_ref_for_node so the
conditional and else clause are not needed

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 3c1129d91825..3bbfb2455b70 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -670,18 +670,12 @@ static struct binder_ref *binder_get_ref_for_node(struct binder_proc *proc,
 	}
 	rb_link_node(&new_ref->rb_node_desc, parent, p);
 	rb_insert_color(&new_ref->rb_node_desc, &proc->refs_by_desc);
-	if (node) {
-		hlist_add_head(&new_ref->node_entry, &node->refs);
+	hlist_add_head(&new_ref->node_entry, &node->refs);
 
-		binder_debug(BINDER_DEBUG_INTERNAL_REFS,
-			     "%d new ref %d desc %d for node %d\n",
-			      proc->pid, new_ref->debug_id, new_ref->desc,
-			      node->debug_id);
-	} else {
-		binder_debug(BINDER_DEBUG_INTERNAL_REFS,
-			     "%d new ref %d desc %d for dead node\n",
-			      proc->pid, new_ref->debug_id, new_ref->desc);
-	}
+	binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+		     "%d new ref %d desc %d for node %d\n",
+		      proc->pid, new_ref->debug_id, new_ref->desc,
+		      node->debug_id);
 	return new_ref;
 }
 
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678116 — [PATCH 14/37] binder: avoid race conditions when enqueuing txn

Currently, the transaction complete work item is queued
after the transaction. This means that it is possible
for the transaction to be handled and a reply to be
enqueued in the current thread before the transaction
complete is enqueued, which violates the protocol
with userspace who may not expect the transaction
complete. Fixed by always enqueing the transaction
complete first.

Also, once the transaction is enqueued, it is unsafe
to access since it might be freed. Currently,
t->flags is accessed to determine whether a sync
wake is needed. Changed to access tr->flags
instead.

Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/android/binder.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f17d1dfa5b02..71faf548482d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1799,6 +1799,9 @@ static void binder_transaction(struct binder_proc *proc,
 			goto err_bad_object_type;
 		}
 	}
+	tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE;
+	list_add_tail(&tcomplete->entry, &thread->todo);
+
 	if (reply) {
 		BUG_ON(t->buffer->async_transaction != 0);
 		binder_pop_transaction(target_thread, in_reply_to);
@@ -1818,10 +1821,8 @@ static void binder_transaction(struct binder_proc *proc,
 	}
 	t->work.type = BINDER_WORK_TRANSACTION;
 	list_add_tail(&t->work.entry, target_list);
-	tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE;
-	list_add_tail(&tcomplete->entry, &thread->todo);
 	if (target_wait) {
-		if (reply || !(t->flags & TF_ONE_WAY))
+		if (reply || !(tr->flags & TF_ONE_WAY))
 			wake_up_interruptible_sync(target_wait);
 		else
 			wake_up_interruptible(target_wait);
-- 
2.13.2.725.g09c95d1e9-goog

[toc] | [prev] | [next] | [standalone]

#1678503

On Thu, Jun 29, 2017 at 12:01:34PM -0700, Todd Kjos wrote:
> The binder driver uses a global mutex to serialize access to state in a
> multi-threaded environment. This global lock has been increasingly
> problematic as Android devices have scaled to more cores. The problem is
> not so much contention for the global lock which still remains relatively
> low, but the priority inversion which occurs regularly when a lower
> priority thread is preempted while holding the lock and a higher priority
> thread becomes blocked on it. These cases can be especially painful if the
> lower priority thread runs in the background on a slow core at a low
> frequency. This often manifests as missed frames or other glitches.
> 
> For several years, a hacky solution has been used in many Android devices
> which disables preemption for most of the time the global mutex is held.
> This dramatically decreased the cases of glitches induced by priority
> inversion and increased the average throughput for binder transactions.
> 
> Moving to fine-grained locking in this patchset results is a cleaner
> and more scalable solution than the preempt disable hack. Priority
> inversion is decreased significantly.

Yeah, it is great to see this work done!  I'll review these on Monday,
sorry, am traveling at the moment...

thanks,

greg k-h

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web