[PATCH 0/13] Binder driver refactor and minor bug fixes

[PATCH 0/13] Binder driver refactor and minor bug fixes

[Riley Andrews]

This patch series is a cleanup of the binder driver which should make it more
maintainable. That being said, the first two patches in this patch set are
simple bug fixes, one fixing an issue with 64 bit, the other removing an instance
where an error code could be returned twice. Both are fairly straightforward.

The bulk of the refactoring patches involve splitting up the larger functions in
the binder driver: binder_thread_write, binder_thread_read, and binder_transaction.
These functions all had switch statements which are now split up into a function
per case statement. Other smaller changes split off related bits of functionality
from binder_transaction() and binder_thread_read(), hopefully pruning these functions
down to a point where their purpose is more clear.

(1) Fix two bugs:
 - 64 bit user pointer issue
 - error value can get returned to userspace twice

(2) Split binder_thread_write() into smaller functions.
 - move all cases into dedicated functions

(3) Split apart binder_transaction().
 - move flat binder object translation loop into a separate function, and break every case into a dedicated function.
 - add helper functions to move complexity out of binder_transaction() for transaction stack, logging, etc.

(4) Split binder_thread_read() into smaller functions.
 - move all cases into dedicated functions
 - add helper functions for waiting for work, processing work nodes, and handling errors.

[PATCH 01/13] drivers: android: correct the size of struct binder_uintptr_t for BC_DEAD_BINDER_DONE

[Riley Andrews]

From: Lisa Du <cldu@marvell.com>

There's one point was missed in the patch commit da49889deb34 ("staging:
binder: Support concurrent 32 bit and 64 bit processes."). When configure
BINDER_IPC_32BIT, the size of binder_uintptr_t was 32bits, but size of
void * is 64bit on 64bit system. Correct it here.

Signed-off-by: Lisa Du <cldu@marvell.com>
---
 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 6607f3c..f1a26d9 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2074,7 +2074,7 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (get_user(cookie, (binder_uintptr_t __user *)ptr))
 				return -EFAULT;
 
-			ptr += sizeof(void *);
+			ptr += sizeof(cookie);
 			list_for_each_entry(w, &proc->delivered_death, entry) {
 				struct binder_ref_death *tmp_death = container_of(w, struct binder_ref_death, work);
 
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 01/13] drivers: android: correct the size of struct binder_uintptr_t for BC_DEAD_BINDER_DONE

[Dan Carpenter]

On Thu, May 28, 2015 at 04:08:19PM -0700, Riley Andrews wrote:
> From: Lisa Du <cldu@marvell.com>
> 
> There's one point was missed in the patch commit da49889deb34 ("staging:
> binder: Support concurrent 32 bit and 64 bit processes."). When configure
> BINDER_IPC_32BIT, the size of binder_uintptr_t was 32bits, but size of
> void * is 64bit on 64bit system. Correct it here.
> 
> Signed-off-by: Lisa Du <cldu@marvell.com>

You should have your own name signed here as well.

regards,
dan carpenter

[PATCH 02/13] android: binder: fix duplicate error return.

[Riley Andrews]

Duplicate errors can be returned to userspace when the thread
error code is left set when the read buffer runs out of space.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f1a26d9..2069759 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2167,9 +2167,9 @@ retry:
 				return -EFAULT;
 			ptr += sizeof(uint32_t);
 			binder_stat_br(proc, thread, thread->return_error2);
+			thread->return_error2 = BR_OK;
 			if (ptr == end)
 				goto done;
-			thread->return_error2 = BR_OK;
 		}
 		if (put_user(thread->return_error, (uint32_t __user *)ptr))
 			return -EFAULT;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 03/13] android: binder: refactor binder_thread_write

[Riley Andrews]

Give every case in the switch statement its own dedicated function.
Remove the process argument to binder_transact, as it can be derived
from the thread argument as with all of the other newly created
functions.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 613 +++++++++++++++++++++++++++--------------------
 1 file changed, 350 insertions(+), 263 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 2069759..e47c2d4 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1314,8 +1314,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 	}
 }
 
-static void binder_transaction(struct binder_proc *proc,
-			       struct binder_thread *thread,
+static void binder_transaction(struct binder_thread *thread,
 			       struct binder_transaction_data *tr, int reply)
 {
 	struct binder_transaction *t;
@@ -1328,6 +1327,7 @@ 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;
+	struct binder_proc *proc = thread->proc;
 	uint32_t return_error;
 
 	e = binder_transaction_log_add(&binder_transaction_log);
@@ -1752,10 +1752,329 @@ err_no_context_mgr_node:
 		thread->return_error = return_error;
 }
 
+static void binder_call_inc_dec_ref(struct binder_thread *thread,
+				    uint32_t target, uint32_t cmd)
+{
+	struct binder_proc *proc = thread->proc;
+	struct binder_ref *ref;
+	const char *debug_string;
+
+	if (target == 0 && binder_context_mgr_node &&
+	    (cmd == BC_INCREFS || cmd == BC_ACQUIRE)) {
+		ref = binder_get_ref_for_node(proc, binder_context_mgr_node);
+		if (ref->desc != target) {
+			binder_user_error("%d:%d tried to acquire reference to desc 0, got %d instead\n",
+					  proc->pid, thread->pid, ref->desc);
+			return;
+		}
+	} else {
+		ref = binder_get_ref(proc, target);
+	}
+	if (!ref) {
+		binder_user_error("%d:%d refcount change on invalid ref %d\n",
+				  proc->pid, thread->pid, target);
+		return;
+	}
+	switch (cmd) {
+	case BC_INCREFS:
+		debug_string = "IncRefs";
+		binder_inc_ref(ref, 0, NULL);
+		break;
+	case BC_ACQUIRE:
+		debug_string = "Acquire";
+		binder_inc_ref(ref, 1, NULL);
+		break;
+	case BC_RELEASE:
+		debug_string = "Release";
+		binder_dec_ref(ref, 1);
+		break;
+	case BC_DECREFS:
+	default:
+		debug_string = "DecRefs";
+		binder_dec_ref(ref, 0);
+		break;
+	}
+	binder_debug(BINDER_DEBUG_USER_REFS,
+		     "%d:%d %s ref %d desc %d s %d w %d for node %d\n",
+		     proc->pid, thread->pid, debug_string, ref->debug_id,
+		     ref->desc, ref->strong, ref->weak, ref->node->debug_id);
+}
+
+static void binder_call_inc_ref_done(struct binder_thread *thread,
+				     binder_uintptr_t node_ptr,
+				     binder_uintptr_t cookie, uint32_t cmd)
+{
+	struct binder_node *node;
+	struct binder_proc *proc = thread->proc;
+
+	node = binder_get_node(proc, node_ptr);
+	if (!node) {
+		binder_user_error("%d:%d %s u%016llx no match\n",
+				  proc->pid, thread->pid,
+				  cmd == BC_INCREFS_DONE ?
+				  "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
+				  (u64)node_ptr);
+		return;
+	}
+	if (cookie != node->cookie) {
+		binder_user_error("%d:%d %s u%016llx node %d cookie mismatch %016llx != %016llx\n",
+				  proc->pid, thread->pid,
+				  cmd == BC_INCREFS_DONE ?
+				  "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
+				  (u64)node_ptr, node->debug_id,
+				  (u64)cookie, (u64)node->cookie);
+		return;
+	}
+	if (cmd == BC_ACQUIRE_DONE) {
+		if (node->pending_strong_ref == 0) {
+			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
+					  proc->pid, thread->pid,
+					  node->debug_id);
+			return;
+		}
+		node->pending_strong_ref = 0;
+	} else {
+		if (node->pending_weak_ref == 0) {
+			binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
+					  proc->pid, thread->pid,
+					  node->debug_id);
+			return;
+		}
+		node->pending_weak_ref = 0;
+	}
+	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
+	binder_debug(BINDER_DEBUG_USER_REFS, "%d:%d %s node %d ls %d lw %d\n",
+		     proc->pid, thread->pid, cmd == BC_INCREFS_DONE ?
+		     "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
+		     node->debug_id, node->local_strong_refs,
+		     node->local_weak_refs);
+}
+
+static void binder_call_free_buffer(struct binder_thread *thread,
+				    binder_uintptr_t data_ptr)
+{
+	struct binder_buffer *buffer;
+	struct binder_proc *proc = thread->proc;
+
+	buffer = binder_buffer_lookup(proc, data_ptr);
+	if (!buffer) {
+		binder_user_error("%d:%d BC_FREE_BUFFER u%016llx no match\n",
+				  proc->pid, thread->pid, (u64)data_ptr);
+		return;
+	}
+	if (!buffer->allow_user_free) {
+		binder_user_error("%d:%d BC_FREE_BUFFER u%016llx matched unreturned buffer\n",
+				  proc->pid, thread->pid, (u64)data_ptr);
+		return;
+	}
+	binder_debug(BINDER_DEBUG_FREE_BUFFER,
+		     "%d:%d BC_FREE_BUFFER u%016llx found buffer %d for %s transaction\n",
+		     proc->pid, thread->pid, (u64)data_ptr, buffer->debug_id,
+		     buffer->transaction ? "active" : "finished");
+
+	if (buffer->transaction) {
+		buffer->transaction->buffer = NULL;
+		buffer->transaction = NULL;
+	}
+	if (buffer->async_transaction && buffer->target_node) {
+		BUG_ON(!buffer->target_node->has_async_transaction);
+		if (list_empty(&buffer->target_node->async_todo))
+			buffer->target_node->has_async_transaction = 0;
+		else
+			list_move_tail(buffer->target_node->async_todo.next,
+				       &thread->todo);
+	}
+	trace_binder_transaction_buffer_release(buffer);
+	binder_transaction_buffer_release(proc, buffer, NULL);
+	binder_free_buf(proc, buffer);
+}
+
+static void binder_call_register_looper(struct binder_thread *thread)
+{
+	struct binder_proc *proc = thread->proc;
+
+	binder_debug(BINDER_DEBUG_THREADS, "%d:%d BC_REGISTER_LOOPER\n",
+		     proc->pid, thread->pid);
+	if (thread->looper & BINDER_LOOPER_STATE_ENTERED) {
+		thread->looper |= BINDER_LOOPER_STATE_INVALID;
+		binder_user_error("%d:%d ERROR: BC_REGISTER_LOOPER called after BC_ENTER_LOOPER\n",
+				  proc->pid, thread->pid);
+	} else if (proc->requested_threads == 0) {
+		thread->looper |= BINDER_LOOPER_STATE_INVALID;
+		binder_user_error("%d:%d ERROR: BC_REGISTER_LOOPER called without request\n",
+				  proc->pid, thread->pid);
+	} else {
+		proc->requested_threads--;
+		proc->requested_threads_started++;
+	}
+	thread->looper |= BINDER_LOOPER_STATE_REGISTERED;
+}
+
+static void binder_call_enter_looper(struct binder_thread *thread)
+{
+	struct binder_proc *proc = thread->proc;
+
+	binder_debug(BINDER_DEBUG_THREADS, "%d:%d BC_ENTER_LOOPER\n",
+		     proc->pid, thread->pid);
+	if (thread->looper & BINDER_LOOPER_STATE_REGISTERED) {
+		thread->looper |= BINDER_LOOPER_STATE_INVALID;
+		binder_user_error("%d:%d ERROR: BC_ENTER_LOOPER called after BC_REGISTER_LOOPER\n",
+				  proc->pid, thread->pid);
+	}
+	thread->looper |= BINDER_LOOPER_STATE_ENTERED;
+}
+
+static void binder_call_req_death_notification(struct binder_thread *thread,
+					       uint32_t target,
+					       binder_uintptr_t cookie)
+{
+	struct binder_ref *ref;
+	struct binder_ref_death *death;
+	struct binder_proc *proc = thread->proc;
+
+	ref = binder_get_ref(proc, target);
+	if (!ref) {
+		binder_user_error("%d:%d %s invalid ref %d\n",
+				  proc->pid, thread->pid,
+				  "BC_REQUEST_DEATH_NOTIFICATION",
+				  target);
+		return;
+	}
+
+	binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
+		     "%d:%d %s %016llx ref %d desc %d s %d w %d for node %d\n",
+		     proc->pid, thread->pid,
+		     "BC_REQUEST_DEATH_NOTIFICATION",
+		     (u64)cookie, ref->debug_id, ref->desc,
+		     ref->strong, ref->weak, ref->node->debug_id);
+
+	if (ref->death) {
+		binder_user_error("%d:%d BC_REQUEST_DEATH_NOTIFICATION death notification already set\n",
+				  proc->pid, thread->pid);
+		return;
+	}
+	death = kzalloc(sizeof(*death), GFP_KERNEL);
+	if (!death) {
+		thread->return_error = BR_ERROR;
+		binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+			     "%d:%d BC_REQUEST_DEATH_NOTIFICATION failed\n",
+			     proc->pid, thread->pid);
+		return;
+	}
+	binder_stats_created(BINDER_STAT_DEATH);
+	INIT_LIST_HEAD(&death->work.entry);
+	death->cookie = cookie;
+	ref->death = death;
+	if (!ref->node->proc) {
+		ref->death->work.type = BINDER_WORK_DEAD_BINDER;
+		if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED |
+				      BINDER_LOOPER_STATE_ENTERED)) {
+			list_add_tail(&ref->death->work.entry,
+				      &thread->todo);
+		} else {
+			list_add_tail(&ref->death->work.entry,
+				      &proc->todo);
+			wake_up_interruptible(&proc->wait);
+		}
+	}
+}
+
+static void binder_call_clear_death_notification(struct binder_thread *thread,
+						 uint32_t target,
+						 binder_uintptr_t cookie)
+{
+	struct binder_ref *ref;
+	struct binder_ref_death *death;
+	struct binder_proc *proc = thread->proc;
+
+	ref = binder_get_ref(proc, target);
+	if (!ref) {
+		binder_user_error("%d:%d %s invalid ref %d\n",
+				  proc->pid, thread->pid,
+				  "BC_CLEAR_DEATH_NOTIFICATION",
+				  target);
+		return;
+	}
+
+	binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
+		     "%d:%d %s %016llx ref %d desc %d s %d w %d for node %d\n",
+		     proc->pid, thread->pid,
+		     "BC_CLEAR_DEATH_NOTIFICATION",
+		     (u64)cookie, ref->debug_id, ref->desc,
+		     ref->strong, ref->weak, ref->node->debug_id);
+
+	if (!ref->death) {
+		binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification not active\n",
+				  proc->pid, thread->pid);
+		return;
+	}
+	death = ref->death;
+	if (death->cookie != cookie) {
+		binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification cookie mismatch %016llx != %016llx\n",
+				  proc->pid, thread->pid,
+				  (u64)death->cookie, (u64)cookie);
+		return;
+	}
+	ref->death = NULL;
+	if (list_empty(&death->work.entry)) {
+		death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
+		if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED |
+				      BINDER_LOOPER_STATE_ENTERED)) {
+			list_add_tail(&death->work.entry,
+				      &thread->todo);
+		} else {
+			list_add_tail(&death->work.entry, &proc->todo);
+			wake_up_interruptible(&proc->wait);
+		}
+	} else {
+		BUG_ON(death->work.type != BINDER_WORK_DEAD_BINDER);
+		death->work.type = BINDER_WORK_DEAD_BINDER_AND_CLEAR;
+	}
+}
+
+static void binder_call_dead_binder_done(struct binder_thread *thread,
+					 binder_uintptr_t cookie)
+{
+	struct binder_work *work;
+	struct binder_ref_death *death = NULL;
+	struct binder_proc *proc = thread->proc;
+
+	list_for_each_entry(work, &proc->delivered_death, entry) {
+		struct binder_ref_death *tmp_death;
+
+		tmp_death = container_of(work, struct binder_ref_death, work);
+		if (tmp_death->cookie == cookie) {
+			death = tmp_death;
+			break;
+		}
+	}
+	binder_debug(BINDER_DEBUG_DEAD_BINDER,
+		     "%d:%d BC_DEAD_BINDER_DONE %016llx found %p\n",
+		     proc->pid, thread->pid, (u64)cookie,
+		     death);
+	if (!death) {
+		binder_user_error("%d:%d BC_DEAD_BINDER_DONE %016llx not found\n",
+				  proc->pid, thread->pid, (u64)cookie);
+		return;
+	}
+
+	list_del_init(&death->work.entry);
+	if (death->work.type == BINDER_WORK_DEAD_BINDER_AND_CLEAR) {
+		death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
+		if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED |
+				      BINDER_LOOPER_STATE_ENTERED)) {
+			list_add_tail(&death->work.entry, &thread->todo);
+		} else {
+			list_add_tail(&death->work.entry, &proc->todo);
+			wake_up_interruptible(&proc->wait);
+		}
+	}
+}
+
 static int binder_thread_write(struct binder_proc *proc,
-			struct binder_thread *thread,
-			binder_uintptr_t binder_buffer, size_t size,
-			binder_size_t *consumed)
+			       struct binder_thread *thread,
+			       binder_uintptr_t binder_buffer, size_t size,
+			       binder_size_t *consumed)
 {
 	uint32_t cmd;
 	void __user *buffer = (void __user *)(uintptr_t)binder_buffer;
@@ -1778,58 +2097,17 @@ static int binder_thread_write(struct binder_proc *proc,
 		case BC_RELEASE:
 		case BC_DECREFS: {
 			uint32_t target;
-			struct binder_ref *ref;
-			const char *debug_string;
 
 			if (get_user(target, (uint32_t __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(uint32_t);
-			if (target == 0 && binder_context_mgr_node &&
-			    (cmd == BC_INCREFS || cmd == BC_ACQUIRE)) {
-				ref = binder_get_ref_for_node(proc,
-					       binder_context_mgr_node);
-				if (ref->desc != target) {
-					binder_user_error("%d:%d tried to acquire reference to desc 0, got %d instead\n",
-						proc->pid, thread->pid,
-						ref->desc);
-				}
-			} else
-				ref = binder_get_ref(proc, target);
-			if (ref == NULL) {
-				binder_user_error("%d:%d refcount change on invalid ref %d\n",
-					proc->pid, thread->pid, target);
-				break;
-			}
-			switch (cmd) {
-			case BC_INCREFS:
-				debug_string = "IncRefs";
-				binder_inc_ref(ref, 0, NULL);
-				break;
-			case BC_ACQUIRE:
-				debug_string = "Acquire";
-				binder_inc_ref(ref, 1, NULL);
-				break;
-			case BC_RELEASE:
-				debug_string = "Release";
-				binder_dec_ref(ref, 1);
-				break;
-			case BC_DECREFS:
-			default:
-				debug_string = "DecRefs";
-				binder_dec_ref(ref, 0);
-				break;
-			}
-			binder_debug(BINDER_DEBUG_USER_REFS,
-				     "%d:%d %s ref %d desc %d s %d w %d for node %d\n",
-				     proc->pid, thread->pid, debug_string, ref->debug_id,
-				     ref->desc, ref->strong, ref->weak, ref->node->debug_id);
+			binder_call_inc_dec_ref(thread, target, cmd);
 			break;
 		}
 		case BC_INCREFS_DONE:
 		case BC_ACQUIRE_DONE: {
 			binder_uintptr_t node_ptr;
 			binder_uintptr_t cookie;
-			struct binder_node *node;
 
 			if (get_user(node_ptr, (binder_uintptr_t __user *)ptr))
 				return -EFAULT;
@@ -1837,48 +2115,7 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (get_user(cookie, (binder_uintptr_t __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(binder_uintptr_t);
-			node = binder_get_node(proc, node_ptr);
-			if (node == NULL) {
-				binder_user_error("%d:%d %s u%016llx no match\n",
-					proc->pid, thread->pid,
-					cmd == BC_INCREFS_DONE ?
-					"BC_INCREFS_DONE" :
-					"BC_ACQUIRE_DONE",
-					(u64)node_ptr);
-				break;
-			}
-			if (cookie != node->cookie) {
-				binder_user_error("%d:%d %s u%016llx node %d cookie mismatch %016llx != %016llx\n",
-					proc->pid, thread->pid,
-					cmd == BC_INCREFS_DONE ?
-					"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
-					(u64)node_ptr, node->debug_id,
-					(u64)cookie, (u64)node->cookie);
-				break;
-			}
-			if (cmd == BC_ACQUIRE_DONE) {
-				if (node->pending_strong_ref == 0) {
-					binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
-						proc->pid, thread->pid,
-						node->debug_id);
-					break;
-				}
-				node->pending_strong_ref = 0;
-			} else {
-				if (node->pending_weak_ref == 0) {
-					binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
-						proc->pid, thread->pid,
-						node->debug_id);
-					break;
-				}
-				node->pending_weak_ref = 0;
-			}
-			binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
-			binder_debug(BINDER_DEBUG_USER_REFS,
-				     "%d:%d %s node %d ls %d lw %d\n",
-				     proc->pid, thread->pid,
-				     cmd == BC_INCREFS_DONE ? "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
-				     node->debug_id, node->local_strong_refs, node->local_weak_refs);
+			binder_call_inc_ref_done(thread, node_ptr, cookie, cmd);
 			break;
 		}
 		case BC_ATTEMPT_ACQUIRE:
@@ -1887,49 +2124,15 @@ static int binder_thread_write(struct binder_proc *proc,
 		case BC_ACQUIRE_RESULT:
 			pr_err("BC_ACQUIRE_RESULT not supported\n");
 			return -EINVAL;
-
 		case BC_FREE_BUFFER: {
 			binder_uintptr_t data_ptr;
-			struct binder_buffer *buffer;
 
 			if (get_user(data_ptr, (binder_uintptr_t __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(binder_uintptr_t);
-
-			buffer = binder_buffer_lookup(proc, 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);
-				break;
-			}
-			if (!buffer->allow_user_free) {
-				binder_user_error("%d:%d BC_FREE_BUFFER u%016llx matched unreturned buffer\n",
-					proc->pid, thread->pid, (u64)data_ptr);
-				break;
-			}
-			binder_debug(BINDER_DEBUG_FREE_BUFFER,
-				     "%d:%d BC_FREE_BUFFER u%016llx found buffer %d for %s transaction\n",
-				     proc->pid, thread->pid, (u64)data_ptr,
-				     buffer->debug_id,
-				     buffer->transaction ? "active" : "finished");
-
-			if (buffer->transaction) {
-				buffer->transaction->buffer = NULL;
-				buffer->transaction = NULL;
-			}
-			if (buffer->async_transaction && buffer->target_node) {
-				BUG_ON(!buffer->target_node->has_async_transaction);
-				if (list_empty(&buffer->target_node->async_todo))
-					buffer->target_node->has_async_transaction = 0;
-				else
-					list_move_tail(buffer->target_node->async_todo.next, &thread->todo);
-			}
-			trace_binder_transaction_buffer_release(buffer);
-			binder_transaction_buffer_release(proc, buffer, NULL);
-			binder_free_buf(proc, buffer);
+			binder_call_free_buffer(thread, data_ptr);
 			break;
 		}
-
 		case BC_TRANSACTION:
 		case BC_REPLY: {
 			struct binder_transaction_data tr;
@@ -1937,38 +2140,14 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (copy_from_user(&tr, ptr, sizeof(tr)))
 				return -EFAULT;
 			ptr += sizeof(tr);
-			binder_transaction(proc, thread, &tr, cmd == BC_REPLY);
+			binder_transaction(thread, &tr, cmd == BC_REPLY);
 			break;
 		}
-
 		case BC_REGISTER_LOOPER:
-			binder_debug(BINDER_DEBUG_THREADS,
-				     "%d:%d BC_REGISTER_LOOPER\n",
-				     proc->pid, thread->pid);
-			if (thread->looper & BINDER_LOOPER_STATE_ENTERED) {
-				thread->looper |= BINDER_LOOPER_STATE_INVALID;
-				binder_user_error("%d:%d ERROR: BC_REGISTER_LOOPER called after BC_ENTER_LOOPER\n",
-					proc->pid, thread->pid);
-			} else if (proc->requested_threads == 0) {
-				thread->looper |= BINDER_LOOPER_STATE_INVALID;
-				binder_user_error("%d:%d ERROR: BC_REGISTER_LOOPER called without request\n",
-					proc->pid, thread->pid);
-			} else {
-				proc->requested_threads--;
-				proc->requested_threads_started++;
-			}
-			thread->looper |= BINDER_LOOPER_STATE_REGISTERED;
+			binder_call_register_looper(thread);
 			break;
 		case BC_ENTER_LOOPER:
-			binder_debug(BINDER_DEBUG_THREADS,
-				     "%d:%d BC_ENTER_LOOPER\n",
-				     proc->pid, thread->pid);
-			if (thread->looper & BINDER_LOOPER_STATE_REGISTERED) {
-				thread->looper |= BINDER_LOOPER_STATE_INVALID;
-				binder_user_error("%d:%d ERROR: BC_ENTER_LOOPER called after BC_REGISTER_LOOPER\n",
-					proc->pid, thread->pid);
-			}
-			thread->looper |= BINDER_LOOPER_STATE_ENTERED;
+			binder_call_enter_looper(thread);
 			break;
 		case BC_EXIT_LOOPER:
 			binder_debug(BINDER_DEBUG_THREADS,
@@ -1976,13 +2155,23 @@ static int binder_thread_write(struct binder_proc *proc,
 				     proc->pid, thread->pid);
 			thread->looper |= BINDER_LOOPER_STATE_EXITED;
 			break;
+		case BC_REQUEST_DEATH_NOTIFICATION: {
+			uint32_t target;
+			binder_uintptr_t cookie;
 
-		case BC_REQUEST_DEATH_NOTIFICATION:
+			if (get_user(target, (uint32_t __user *)ptr))
+				return -EFAULT;
+			ptr += sizeof(uint32_t);
+			if (get_user(cookie, (binder_uintptr_t __user *)ptr))
+				return -EFAULT;
+			ptr += sizeof(binder_uintptr_t);
+			binder_call_req_death_notification(thread, target,
+							   cookie);
+			break;
+		}
 		case BC_CLEAR_DEATH_NOTIFICATION: {
 			uint32_t target;
 			binder_uintptr_t cookie;
-			struct binder_ref *ref;
-			struct binder_ref_death *death;
 
 			if (get_user(target, (uint32_t __user *)ptr))
 				return -EFAULT;
@@ -1990,121 +2179,19 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (get_user(cookie, (binder_uintptr_t __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(binder_uintptr_t);
-			ref = binder_get_ref(proc, target);
-			if (ref == NULL) {
-				binder_user_error("%d:%d %s invalid ref %d\n",
-					proc->pid, thread->pid,
-					cmd == BC_REQUEST_DEATH_NOTIFICATION ?
-					"BC_REQUEST_DEATH_NOTIFICATION" :
-					"BC_CLEAR_DEATH_NOTIFICATION",
-					target);
-				break;
-			}
-
-			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
-				     "%d:%d %s %016llx ref %d desc %d s %d w %d for node %d\n",
-				     proc->pid, thread->pid,
-				     cmd == BC_REQUEST_DEATH_NOTIFICATION ?
-				     "BC_REQUEST_DEATH_NOTIFICATION" :
-				     "BC_CLEAR_DEATH_NOTIFICATION",
-				     (u64)cookie, ref->debug_id, ref->desc,
-				     ref->strong, ref->weak, ref->node->debug_id);
-
-			if (cmd == BC_REQUEST_DEATH_NOTIFICATION) {
-				if (ref->death) {
-					binder_user_error("%d:%d BC_REQUEST_DEATH_NOTIFICATION death notification already set\n",
-						proc->pid, thread->pid);
-					break;
-				}
-				death = kzalloc(sizeof(*death), GFP_KERNEL);
-				if (death == NULL) {
-					thread->return_error = BR_ERROR;
-					binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
-						     "%d:%d BC_REQUEST_DEATH_NOTIFICATION failed\n",
-						     proc->pid, thread->pid);
-					break;
-				}
-				binder_stats_created(BINDER_STAT_DEATH);
-				INIT_LIST_HEAD(&death->work.entry);
-				death->cookie = cookie;
-				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)) {
-						list_add_tail(&ref->death->work.entry, &thread->todo);
-					} else {
-						list_add_tail(&ref->death->work.entry, &proc->todo);
-						wake_up_interruptible(&proc->wait);
-					}
-				}
-			} else {
-				if (ref->death == NULL) {
-					binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification not active\n",
-						proc->pid, thread->pid);
-					break;
-				}
-				death = ref->death;
-				if (death->cookie != cookie) {
-					binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification cookie mismatch %016llx != %016llx\n",
-						proc->pid, thread->pid,
-						(u64)death->cookie,
-						(u64)cookie);
-					break;
-				}
-				ref->death = NULL;
-				if (list_empty(&death->work.entry)) {
-					death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
-					if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
-						list_add_tail(&death->work.entry, &thread->todo);
-					} else {
-						list_add_tail(&death->work.entry, &proc->todo);
-						wake_up_interruptible(&proc->wait);
-					}
-				} else {
-					BUG_ON(death->work.type != BINDER_WORK_DEAD_BINDER);
-					death->work.type = BINDER_WORK_DEAD_BINDER_AND_CLEAR;
-				}
-			}
-		} break;
+			binder_call_clear_death_notification(thread, target,
+							     cookie);
+			break;
+		}
 		case BC_DEAD_BINDER_DONE: {
-			struct binder_work *w;
 			binder_uintptr_t cookie;
-			struct binder_ref_death *death = NULL;
 
 			if (get_user(cookie, (binder_uintptr_t __user *)ptr))
 				return -EFAULT;
-
 			ptr += sizeof(cookie);
-			list_for_each_entry(w, &proc->delivered_death, entry) {
-				struct binder_ref_death *tmp_death = container_of(w, struct binder_ref_death, work);
-
-				if (tmp_death->cookie == cookie) {
-					death = tmp_death;
-					break;
-				}
-			}
-			binder_debug(BINDER_DEBUG_DEAD_BINDER,
-				     "%d:%d BC_DEAD_BINDER_DONE %016llx found %p\n",
-				     proc->pid, thread->pid, (u64)cookie,
-				     death);
-			if (death == NULL) {
-				binder_user_error("%d:%d BC_DEAD_BINDER_DONE %016llx not found\n",
-					proc->pid, thread->pid, (u64)cookie);
-				break;
-			}
-
-			list_del_init(&death->work.entry);
-			if (death->work.type == BINDER_WORK_DEAD_BINDER_AND_CLEAR) {
-				death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
-				if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
-					list_add_tail(&death->work.entry, &thread->todo);
-				} else {
-					list_add_tail(&death->work.entry, &proc->todo);
-					wake_up_interruptible(&proc->wait);
-				}
-			}
-		} break;
-
+			binder_call_dead_binder_done(thread, cookie);
+			break;
+		}
 		default:
 			pr_err("%d:%d unknown command %d\n",
 			       proc->pid, thread->pid, cmd);
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 03/13] android: binder: refactor binder_thread_write

[Dan Carpenter]

This patch is ok.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

On Thu, May 28, 2015 at 04:08:21PM -0700, Riley Andrews wrote:
> +static void binder_call_inc_dec_ref(struct binder_thread *thread,
> +				    uint32_t target, uint32_t cmd)
> +{
> +	struct binder_proc *proc = thread->proc;
> +	struct binder_ref *ref;
> +	const char *debug_string;
> +
> +	if (target == 0 && binder_context_mgr_node &&
> +	    (cmd == BC_INCREFS || cmd == BC_ACQUIRE)) {
> +		ref = binder_get_ref_for_node(proc, binder_context_mgr_node);
> +		if (ref->desc != target) {
> +			binder_user_error("%d:%d tried to acquire reference to desc 0, got %d instead\n",
> +					  proc->pid, thread->pid, ref->desc);
> +			return;

Presumably we never hit this error condition.  In the original code we
printed an error and continued but now we bail on error.  Presumably
this is a theoretical bug fix which doesn't affect real life so I will
allow it.  But in the future don't mix behaviour changes with clean up
patches.

> +		}
> +	} else {
> +		ref = binder_get_ref(proc, target);
> +	}
> +	if (!ref) {

In the original code this was "if (ref == NULL)".  Please don't mix
these kinds of cleanups with moving code around because it makes it
harder for me to review.  Do that in a separate patch.

regards,
dan carpenter

[PATCH 04/13] android: binder: refactor binder_transact handle translation

[Riley Andrews]

Add dedicated functions for handling the movement of fds, binder objects,
and references across processes.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 311 +++++++++++++++++++++++++----------------------
 1 file changed, 167 insertions(+), 144 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e47c2d4..a9a160a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1314,6 +1314,156 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 	}
 }
 
+static int binder_translate_object(struct flat_binder_object *fp,
+				   struct binder_transaction *t,
+				   struct binder_thread *thread)
+{
+	struct binder_ref *ref;
+	struct binder_proc *proc = thread->proc;
+	struct binder_node *node;
+	struct binder_proc *target_proc = t->to_proc;
+
+	node = binder_get_node(proc, fp->binder);
+	if (!node) {
+		node = binder_new_node(proc, fp->binder, fp->cookie);
+		if (!node)
+			return BR_FAILED_REPLY;
+
+		node->min_priority = fp->flags & FLAT_BINDER_FLAG_PRIORITY_MASK;
+		node->accept_fds = !!(fp->flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
+	}
+	if (fp->cookie != node->cookie) {
+		binder_user_error("%d:%d sending u%016llx node %d, cookie mismatch %016llx != %016llx\n",
+				  proc->pid, thread->pid,
+				  (u64)fp->binder, node->debug_id,
+				  (u64)fp->cookie, (u64)node->cookie);
+		return BR_FAILED_REPLY;
+	}
+
+	if (security_binder_transfer_binder(proc->tsk, target_proc->tsk))
+		return BR_FAILED_REPLY;
+
+	ref = binder_get_ref_for_node(target_proc, node);
+	if (!ref)
+		return BR_FAILED_REPLY;
+
+	if (fp->type == BINDER_TYPE_BINDER)
+		fp->type = BINDER_TYPE_HANDLE;
+	else
+		fp->type = BINDER_TYPE_WEAK_HANDLE;
+	fp->handle = ref->desc;
+	binder_inc_ref(ref, fp->type == BINDER_TYPE_HANDLE, &thread->todo);
+
+	trace_binder_transaction_node_to_ref(t, node, ref);
+	binder_debug(BINDER_DEBUG_TRANSACTION,
+		     "        node %d u%016llx -> ref %d desc %d\n",
+		     node->debug_id, (u64)node->ptr,
+		     ref->debug_id, ref->desc);
+	return BR_OK;
+}
+
+static int binder_translate_handle(struct flat_binder_object *fp,
+				   struct binder_transaction *t,
+				   struct binder_thread *thread)
+{
+	struct binder_proc *proc = thread->proc;
+	struct binder_proc *target_proc = t->to_proc;
+	struct binder_ref *ref;
+
+	ref = binder_get_ref(proc, fp->handle);
+	if (!ref) {
+		binder_user_error("%d:%d got transaction with invalid handle, %d\n",
+				  proc->pid, thread->pid, fp->handle);
+		return BR_FAILED_REPLY;
+	}
+	if (security_binder_transfer_binder(proc->tsk, target_proc->tsk))
+		return BR_FAILED_REPLY;
+	if (ref->node->proc == target_proc) {
+		if (fp->type == BINDER_TYPE_HANDLE)
+			fp->type = BINDER_TYPE_BINDER;
+		else
+			fp->type = BINDER_TYPE_WEAK_BINDER;
+		fp->binder = ref->node->ptr;
+		fp->cookie = ref->node->cookie;
+		binder_inc_node(ref->node, fp->type == BINDER_TYPE_BINDER, 0,
+				NULL);
+		trace_binder_transaction_ref_to_node(t, ref);
+		binder_debug(BINDER_DEBUG_TRANSACTION,
+			     "        ref %d desc %d -> node %d u%016llx\n",
+			     ref->debug_id, ref->desc, ref->node->debug_id,
+			     (u64)ref->node->ptr);
+	} else {
+		struct binder_ref *new_ref;
+
+		new_ref = binder_get_ref_for_node(target_proc, ref->node);
+		if (!new_ref)
+			return BR_FAILED_REPLY;
+		fp->handle = new_ref->desc;
+		binder_inc_ref(new_ref, fp->type == BINDER_TYPE_HANDLE, NULL);
+		trace_binder_transaction_ref_to_ref(t, ref, new_ref);
+		binder_debug(BINDER_DEBUG_TRANSACTION,
+			     "        ref %d desc %d -> ref %d desc %d (node %d)\n",
+			     ref->debug_id, ref->desc, new_ref->debug_id,
+			     new_ref->desc, ref->node->debug_id);
+	}
+	return BR_OK;
+}
+
+static int binder_translate_fd(struct flat_binder_object *fp,
+			       struct binder_transaction *t,
+			       struct binder_thread *thread,
+			       struct binder_transaction *in_reply_to)
+
+{
+	struct binder_proc *proc = thread->proc;
+	struct binder_proc *target_proc = t->to_proc;
+	int target_fd;
+	struct file *file;
+	int ret;
+	bool target_allows_fd;
+
+	if (in_reply_to)
+		target_allows_fd = !!(in_reply_to->flags & TF_ACCEPT_FDS);
+	else
+		target_allows_fd = t->buffer->target_node->accept_fds;
+	if (!target_allows_fd) {
+		binder_user_error("%d:%d got %s with fd, %d, but target does not allow fds\n",
+				  proc->pid, thread->pid,
+				  (in_reply_to ? "reply" : "transaction"),
+				  fp->handle);
+		goto err_fd_not_accepted;
+	}
+
+	file = fget(fp->handle);
+	if (!file) {
+		binder_user_error("%d:%d got transaction with invalid fd, %d\n",
+				  proc->pid, thread->pid, fp->handle);
+		goto err_fget;
+	}
+	ret = security_binder_transfer_file(proc->tsk, target_proc->tsk, file);
+	if (ret < 0)
+		goto err_security;
+
+	target_fd = task_get_unused_fd_flags(target_proc, O_CLOEXEC);
+	if (target_fd < 0)
+		goto err_get_unused_fd;
+
+	task_fd_install(target_proc, target_fd, file);
+	trace_binder_transaction_fd(t, fp->handle, target_fd);
+	binder_debug(BINDER_DEBUG_TRANSACTION, "        fd %d -> %d\n",
+		     fp->handle, target_fd);
+	/* TODO: fput? */
+	fp->handle = target_fd;
+	return BR_OK;
+
+err_get_unused_fd:
+err_security:
+	fput(file);
+err_fget:
+err_fd_not_accepted:
+	return BR_FAILED_REPLY;
+}
+
 static void binder_transaction(struct binder_thread *thread,
 			       struct binder_transaction_data *tr, int reply)
 {
@@ -1536,145 +1686,23 @@ static void binder_transaction(struct binder_thread *thread,
 		fp = (struct flat_binder_object *)(t->buffer->data + *offp);
 		switch (fp->type) {
 		case BINDER_TYPE_BINDER:
-		case BINDER_TYPE_WEAK_BINDER: {
-			struct binder_ref *ref;
-			struct binder_node *node = binder_get_node(proc, fp->binder);
-
-			if (node == NULL) {
-				node = binder_new_node(proc, fp->binder, fp->cookie);
-				if (node == NULL) {
-					return_error = BR_FAILED_REPLY;
-					goto err_binder_new_node_failed;
-				}
-				node->min_priority = fp->flags & FLAT_BINDER_FLAG_PRIORITY_MASK;
-				node->accept_fds = !!(fp->flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
-			}
-			if (fp->cookie != node->cookie) {
-				binder_user_error("%d:%d sending u%016llx node %d, cookie mismatch %016llx != %016llx\n",
-					proc->pid, thread->pid,
-					(u64)fp->binder, node->debug_id,
-					(u64)fp->cookie, (u64)node->cookie);
-				return_error = BR_FAILED_REPLY;
-				goto err_binder_get_ref_for_node_failed;
-			}
-			if (security_binder_transfer_binder(proc->tsk,
-							    target_proc->tsk)) {
-				return_error = BR_FAILED_REPLY;
-				goto err_binder_get_ref_for_node_failed;
-			}
-			ref = binder_get_ref_for_node(target_proc, node);
-			if (ref == NULL) {
-				return_error = BR_FAILED_REPLY;
-				goto err_binder_get_ref_for_node_failed;
-			}
-			if (fp->type == BINDER_TYPE_BINDER)
-				fp->type = BINDER_TYPE_HANDLE;
-			else
-				fp->type = BINDER_TYPE_WEAK_HANDLE;
-			fp->handle = ref->desc;
-			binder_inc_ref(ref, fp->type == BINDER_TYPE_HANDLE,
-				       &thread->todo);
-
-			trace_binder_transaction_node_to_ref(t, node, ref);
-			binder_debug(BINDER_DEBUG_TRANSACTION,
-				     "        node %d u%016llx -> ref %d desc %d\n",
-				     node->debug_id, (u64)node->ptr,
-				     ref->debug_id, ref->desc);
-		} break;
+		case BINDER_TYPE_WEAK_BINDER:
+			return_error = binder_translate_object(fp, t, thread);
+			if (return_error != BR_OK)
+				goto err_translate_failed;
+			break;
 		case BINDER_TYPE_HANDLE:
-		case BINDER_TYPE_WEAK_HANDLE: {
-			struct binder_ref *ref = binder_get_ref(proc, fp->handle);
-
-			if (ref == NULL) {
-				binder_user_error("%d:%d got transaction with invalid handle, %d\n",
-						proc->pid,
-						thread->pid, fp->handle);
-				return_error = BR_FAILED_REPLY;
-				goto err_binder_get_ref_failed;
-			}
-			if (security_binder_transfer_binder(proc->tsk,
-							    target_proc->tsk)) {
-				return_error = BR_FAILED_REPLY;
-				goto err_binder_get_ref_failed;
-			}
-			if (ref->node->proc == target_proc) {
-				if (fp->type == BINDER_TYPE_HANDLE)
-					fp->type = BINDER_TYPE_BINDER;
-				else
-					fp->type = BINDER_TYPE_WEAK_BINDER;
-				fp->binder = ref->node->ptr;
-				fp->cookie = ref->node->cookie;
-				binder_inc_node(ref->node, fp->type == BINDER_TYPE_BINDER, 0, NULL);
-				trace_binder_transaction_ref_to_node(t, ref);
-				binder_debug(BINDER_DEBUG_TRANSACTION,
-					     "        ref %d desc %d -> node %d u%016llx\n",
-					     ref->debug_id, ref->desc, ref->node->debug_id,
-					     (u64)ref->node->ptr);
-			} else {
-				struct binder_ref *new_ref;
-
-				new_ref = binder_get_ref_for_node(target_proc, ref->node);
-				if (new_ref == NULL) {
-					return_error = BR_FAILED_REPLY;
-					goto err_binder_get_ref_for_node_failed;
-				}
-				fp->handle = new_ref->desc;
-				binder_inc_ref(new_ref, fp->type == BINDER_TYPE_HANDLE, NULL);
-				trace_binder_transaction_ref_to_ref(t, ref,
-								    new_ref);
-				binder_debug(BINDER_DEBUG_TRANSACTION,
-					     "        ref %d desc %d -> ref %d desc %d (node %d)\n",
-					     ref->debug_id, ref->desc, new_ref->debug_id,
-					     new_ref->desc, ref->node->debug_id);
-			}
-		} break;
-
-		case BINDER_TYPE_FD: {
-			int target_fd;
-			struct file *file;
-
-			if (reply) {
-				if (!(in_reply_to->flags & TF_ACCEPT_FDS)) {
-					binder_user_error("%d:%d got reply with fd, %d, but target does not allow fds\n",
-						proc->pid, thread->pid, fp->handle);
-					return_error = BR_FAILED_REPLY;
-					goto err_fd_not_allowed;
-				}
-			} else if (!target_node->accept_fds) {
-				binder_user_error("%d:%d got transaction with fd, %d, but target does not allow fds\n",
-					proc->pid, thread->pid, fp->handle);
-				return_error = BR_FAILED_REPLY;
-				goto err_fd_not_allowed;
-			}
-
-			file = fget(fp->handle);
-			if (file == NULL) {
-				binder_user_error("%d:%d got transaction with invalid fd, %d\n",
-					proc->pid, thread->pid, fp->handle);
-				return_error = BR_FAILED_REPLY;
-				goto err_fget_failed;
-			}
-			if (security_binder_transfer_file(proc->tsk,
-							  target_proc->tsk,
-							  file) < 0) {
-				fput(file);
-				return_error = BR_FAILED_REPLY;
-				goto err_get_unused_fd_failed;
-			}
-			target_fd = task_get_unused_fd_flags(target_proc, O_CLOEXEC);
-			if (target_fd < 0) {
-				fput(file);
-				return_error = BR_FAILED_REPLY;
-				goto err_get_unused_fd_failed;
-			}
-			task_fd_install(target_proc, target_fd, file);
-			trace_binder_transaction_fd(t, fp->handle, target_fd);
-			binder_debug(BINDER_DEBUG_TRANSACTION,
-				     "        fd %d -> %d\n", fp->handle, target_fd);
-			/* TODO: fput? */
-			fp->handle = target_fd;
-		} break;
-
+		case BINDER_TYPE_WEAK_HANDLE:
+			return_error = binder_translate_handle(fp, t, thread);
+			if (return_error != BR_OK)
+				goto err_translate_failed;
+			break;
+		case BINDER_TYPE_FD:
+			return_error = binder_translate_fd(fp, t, thread,
+							   in_reply_to);
+			if (return_error != BR_OK)
+				goto err_translate_failed;
+			break;
 		default:
 			binder_user_error("%d:%d got transaction with invalid object type, %x\n",
 				proc->pid, thread->pid, fp->type);
@@ -1707,12 +1735,7 @@ static void binder_transaction(struct binder_thread *thread,
 		wake_up_interruptible(target_wait);
 	return;
 
-err_get_unused_fd_failed:
-err_fget_failed:
-err_fd_not_allowed:
-err_binder_get_ref_for_node_failed:
-err_binder_get_ref_failed:
-err_binder_new_node_failed:
+err_translate_failed:
 err_bad_object_type:
 err_bad_offset:
 err_copy_data_failed:
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 05/13] android: binder: refactor binder_transact transaction buffer loop

[Riley Andrews]

Pull the loop that translates the flat_binder_objects into a separate
function, binder_transaction_buffer_acquire.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 128 ++++++++++++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 51 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index a9a160a..407c1ee 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1464,12 +1464,84 @@ err_fd_not_accepted:
 	return BR_FAILED_REPLY;
 }
 
+static int binder_transaction_buffer_acquire(
+	struct binder_transaction *t, struct binder_transaction_data *tr,
+	struct binder_thread *thread, struct binder_transaction *in_reply_to)
+{
+	struct binder_proc *proc = thread->proc;
+	binder_size_t *offp, *off_end, off_min;
+	struct flat_binder_object *fp;
+	uint32_t result;
+
+	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 BR_FAILED_REPLY;
+	}
+
+	if (t->buffer->target_node)
+		binder_inc_node(t->buffer->target_node, 1, 0, NULL);
+
+	off_min = 0;
+	offp = (binder_size_t *)(t->buffer->data +
+				 ALIGN(tr->data_size, sizeof(void *)));
+	off_end = (void *)offp + tr->offsets_size;
+	for (; offp < off_end; offp++) {
+		if (*offp > t->buffer->data_size - sizeof(*fp) ||
+		    *offp < off_min ||
+		    t->buffer->data_size < sizeof(*fp) ||
+		    !IS_ALIGNED(*offp, sizeof(u32))) {
+			binder_user_error("%d:%d got transaction with invalid offset, %lld (min %lld, max %lld)\n",
+					  proc->pid, thread->pid, (u64)*offp,
+					  (u64)off_min,
+					  (u64)(t->buffer->data_size -
+					  sizeof(*fp)));
+			result = BR_FAILED_REPLY;
+			goto error;
+		}
+		fp = (struct flat_binder_object *)(t->buffer->data + *offp);
+		off_min = *offp + sizeof(struct flat_binder_object);
+		switch (fp->type) {
+		case BINDER_TYPE_BINDER:
+		case BINDER_TYPE_WEAK_BINDER:
+			result = binder_translate_object(fp, t, thread);
+			if (result != BR_OK)
+				goto error;
+			break;
+		case BINDER_TYPE_HANDLE:
+		case BINDER_TYPE_WEAK_HANDLE:
+			result = binder_translate_handle(fp, t, thread);
+			if (result != BR_OK)
+				goto error;
+			break;
+		case BINDER_TYPE_FD:
+			result = binder_translate_fd(fp, t, thread,
+						     in_reply_to);
+			if (result != BR_OK)
+				goto error;
+			break;
+		default:
+			binder_user_error("got transaction with invalid object type, %x\n",
+					  fp->type);
+			result = BR_FAILED_REPLY;
+			goto error;
+		}
+	}
+	return BR_OK;
+
+error:
+	trace_binder_transaction_failed_buffer_release(t->buffer);
+	binder_transaction_buffer_release(t->to_proc, t->buffer, offp);
+	return result;
+}
+
 static void binder_transaction(struct binder_thread *thread,
 			       struct binder_transaction_data *tr, int reply)
 {
 	struct binder_transaction *t;
 	struct binder_work *tcomplete;
-	binder_size_t *offp, *off_end;
+	binder_size_t *offp;
 	struct binder_proc *target_proc;
 	struct binder_thread *target_thread = NULL;
 	struct binder_node *target_node = NULL;
@@ -1645,8 +1717,6 @@ static void binder_transaction(struct binder_thread *thread,
 	t->buffer->transaction = t;
 	t->buffer->target_node = target_node;
 	trace_binder_transaction_alloc_buf(t->buffer);
-	if (target_node)
-		binder_inc_node(target_node, 1, 0, NULL);
 
 	offp = (binder_size_t *)(t->buffer->data +
 				 ALIGN(tr->data_size, sizeof(void *)));
@@ -1665,51 +1735,11 @@ static void binder_transaction(struct binder_thread *thread,
 		return_error = BR_FAILED_REPLY;
 		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;
-		goto err_bad_offset;
-	}
-	off_end = (void *)offp + tr->offsets_size;
-	for (; offp < off_end; offp++) {
-		struct flat_binder_object *fp;
+	return_error = binder_transaction_buffer_acquire(t, tr, thread,
+							 in_reply_to);
+	if (return_error != BR_OK)
+		goto err_translate_failed;
 
-		if (*offp > t->buffer->data_size - sizeof(*fp) ||
-		    t->buffer->data_size < sizeof(*fp) ||
-		    !IS_ALIGNED(*offp, sizeof(u32))) {
-			binder_user_error("%d:%d got transaction with invalid offset, %lld\n",
-					  proc->pid, thread->pid, (u64)*offp);
-			return_error = BR_FAILED_REPLY;
-			goto err_bad_offset;
-		}
-		fp = (struct flat_binder_object *)(t->buffer->data + *offp);
-		switch (fp->type) {
-		case BINDER_TYPE_BINDER:
-		case BINDER_TYPE_WEAK_BINDER:
-			return_error = binder_translate_object(fp, t, thread);
-			if (return_error != BR_OK)
-				goto err_translate_failed;
-			break;
-		case BINDER_TYPE_HANDLE:
-		case BINDER_TYPE_WEAK_HANDLE:
-			return_error = binder_translate_handle(fp, t, thread);
-			if (return_error != BR_OK)
-				goto err_translate_failed;
-			break;
-		case BINDER_TYPE_FD:
-			return_error = binder_translate_fd(fp, t, thread,
-							   in_reply_to);
-			if (return_error != BR_OK)
-				goto err_translate_failed;
-			break;
-		default:
-			binder_user_error("%d:%d got transaction with invalid object type, %x\n",
-				proc->pid, thread->pid, fp->type);
-			return_error = BR_FAILED_REPLY;
-			goto err_bad_object_type;
-		}
-	}
 	if (reply) {
 		BUG_ON(t->buffer->async_transaction != 0);
 		binder_pop_transaction(target_thread, in_reply_to);
@@ -1736,11 +1766,7 @@ static void binder_transaction(struct binder_thread *thread,
 	return;
 
 err_translate_failed:
-err_bad_object_type:
-err_bad_offset:
 err_copy_data_failed:
-	trace_binder_transaction_failed_buffer_release(t->buffer);
-	binder_transaction_buffer_release(target_proc, t->buffer, offp);
 	t->buffer->transaction = NULL;
 	binder_free_buf(target_proc, t->buffer);
 err_binder_alloc_buf_failed:
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 05/13] android: binder: refactor binder_transact transaction buffer loop

[Dan Carpenter]

On Thu, May 28, 2015 at 04:08:23PM -0700, Riley Andrews wrote:
> +static int binder_transaction_buffer_acquire(
> +	struct binder_transaction *t, struct binder_transaction_data *tr,
> +	struct binder_thread *thread, struct binder_transaction *in_reply_to)
> +{
> +	struct binder_proc *proc = thread->proc;
> +	binder_size_t *offp, *off_end, off_min;
> +	struct flat_binder_object *fp;
> +	uint32_t result;
> +
> +	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 BR_FAILED_REPLY;


This smells like a behavior change.  In the original code we called
trace_binder_transaction_failed_buffer_release().  Are you sure it's
correct?

Naming the labels after the goto location is an anti-pattern.

	aaa = kmalloc();
	if (!aaa)
		goto kmalloc_failed;

The label name doesn't provide useful information compared to the line
before.  If binder_transaction_buffer_acquire() fails we say "goto
err_translate_failed;" but actually translate didn't fail because we
haven't yet attempted to translate so the little information the label
does provide is misleading.  Grumble grumble etc.  Also "error:" is a
meaningless label name.  Name labels after what the label does
"goto release_buffer;".

regards,
dan carpenter

[PATCH 06/13] android: binder: add function to find target binder node

[Riley Andrews]

Pull the logic that determines the target_node of a transaction into a
dedicated function.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 407c1ee..99a3270 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1536,6 +1536,29 @@ error:
 	return result;
 }
 
+static int binder_get_tr_target_node(struct binder_thread *thread,
+				     struct binder_transaction_data *tr,
+				     struct binder_node **target_node)
+{
+	struct binder_proc *proc = thread->proc;
+	struct binder_ref *ref;
+
+	if (tr->target.handle) {
+		ref = binder_get_ref(proc, tr->target.handle);
+		if (!ref) {
+			binder_user_error("%d:%d got transaction to invalid handle\n",
+					  proc->pid, thread->pid);
+			return BR_FAILED_REPLY;
+		}
+		*target_node = ref->node;
+	} else {
+		*target_node = binder_context_mgr_node;
+		if (!(*target_node))
+			return BR_DEAD_REPLY;
+	}
+	return BR_OK;
+}
+
 static void binder_transaction(struct binder_thread *thread,
 			       struct binder_transaction_data *tr, int reply)
 {
@@ -1599,24 +1622,10 @@ static void binder_transaction(struct binder_thread *thread,
 		}
 		target_proc = target_thread->proc;
 	} else {
-		if (tr->target.handle) {
-			struct binder_ref *ref;
-
-			ref = binder_get_ref(proc, tr->target.handle);
-			if (ref == NULL) {
-				binder_user_error("%d:%d got transaction to invalid handle\n",
-					proc->pid, thread->pid);
-				return_error = BR_FAILED_REPLY;
-				goto err_invalid_target_handle;
-			}
-			target_node = ref->node;
-		} else {
-			target_node = binder_context_mgr_node;
-			if (target_node == NULL) {
-				return_error = BR_DEAD_REPLY;
-				goto err_no_context_mgr_node;
-			}
-		}
+		return_error = binder_get_tr_target_node(thread, tr,
+							 &target_node);
+		if (return_error != BR_OK)
+			goto err_invalid_target_handle;
 		e->to_node = target_node->debug_id;
 		target_proc = target_node->proc;
 		if (target_proc == NULL) {
@@ -1780,7 +1789,6 @@ err_bad_call_stack:
 err_empty_call_stack:
 err_dead_binder:
 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,
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 07/13] android: binder: add functions for manipulating transaction stack

[Riley Andrews]

Add helper functions for manipulating the transaction stack, and
for validating the transaction stack during binder transactions
and replies.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 126 +++++++++++++++++++++++++++++------------------
 1 file changed, 79 insertions(+), 47 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 99a3270..ed94121 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1167,14 +1167,20 @@ static int binder_dec_ref(struct binder_ref *ref, int strong)
 	return 0;
 }
 
+static void binder_push_transaction(struct binder_thread *thread,
+				    struct binder_transaction *t)
+{
+	t->from_parent = thread->transaction_stack;
+	thread->transaction_stack = t;
+}
+
 static void binder_pop_transaction(struct binder_thread *target_thread,
 				   struct binder_transaction *t)
 {
 	if (target_thread) {
 		BUG_ON(target_thread->transaction_stack != t);
 		BUG_ON(target_thread->transaction_stack->from != target_thread);
-		target_thread->transaction_stack =
-			target_thread->transaction_stack->from_parent;
+		target_thread->transaction_stack = t->from_parent;
 		t->from = NULL;
 	}
 	t->need_reply = 0;
@@ -1184,6 +1190,24 @@ static void binder_pop_transaction(struct binder_thread *target_thread,
 	binder_stats_deleted(BINDER_STAT_TRANSACTION);
 }
 
+static void binder_dst_save_transaction(struct binder_thread *thread,
+					struct binder_transaction *t)
+{
+	t->to_parent = thread->transaction_stack;
+	thread->transaction_stack = t;
+}
+
+static struct binder_transaction *
+binder_dst_restore_transaction(struct binder_thread *thread)
+{
+	struct binder_transaction *t = thread->transaction_stack;
+
+	if (!t)
+		return NULL;
+	thread->transaction_stack = t->to_parent;
+	return t;
+}
+
 static void binder_send_failed_reply(struct binder_transaction *t,
 				     uint32_t error_code)
 {
@@ -1559,6 +1583,47 @@ static int binder_get_tr_target_node(struct binder_thread *thread,
 	return BR_OK;
 }
 
+static int binder_reply_validate_stack(struct binder_thread *thread)
+{
+	struct binder_proc *proc = thread->proc;
+	struct binder_transaction *in_reply_to = NULL;
+
+	in_reply_to = thread->transaction_stack;
+	if (!in_reply_to) {
+		binder_user_error("%d:%d got reply transaction with no transaction stack\n",
+				  proc->pid, thread->pid);
+		return BR_FAILED_REPLY;
+	}
+	if (in_reply_to->to_thread != thread) {
+		binder_user_error("%d:%d got reply transaction with bad transaction stack, transaction %d has target %d:%d\n",
+				  proc->pid, thread->pid, in_reply_to->debug_id,
+				  in_reply_to->to_proc ?
+				  in_reply_to->to_proc->pid : 0,
+				  in_reply_to->to_thread ?
+				  in_reply_to->to_thread->pid : 0);
+		return BR_FAILED_REPLY;
+	}
+	return BR_OK;
+}
+
+static int binder_tr_validate_stack(struct binder_thread *thread)
+{
+	struct binder_transaction *prior = thread->transaction_stack;
+	struct binder_proc *proc = thread->proc;
+
+	if (prior->to_thread != thread) {
+		binder_user_error("%d:%d got new transaction with bad transaction stack, transaction %d has target %d:%d\n",
+				  proc->pid, thread->pid,
+				  prior->debug_id,
+				  prior->to_proc ?
+				  prior->to_proc->pid : 0,
+				  prior->to_thread ?
+				  prior->to_thread->pid : 0);
+		return BR_FAILED_REPLY;
+	}
+	return BR_OK;
+}
+
 static void binder_transaction(struct binder_thread *thread,
 			       struct binder_transaction_data *tr, int reply)
 {
@@ -1584,42 +1649,17 @@ static void binder_transaction(struct binder_thread *thread,
 	e->offsets_size = tr->offsets_size;
 
 	if (reply) {
-		in_reply_to = thread->transaction_stack;
-		if (in_reply_to == NULL) {
-			binder_user_error("%d:%d got reply transaction with no transaction stack\n",
-					  proc->pid, thread->pid);
-			return_error = BR_FAILED_REPLY;
-			goto err_empty_call_stack;
-		}
-		binder_set_nice(in_reply_to->saved_priority);
-		if (in_reply_to->to_thread != thread) {
-			binder_user_error("%d:%d got reply transaction with bad transaction stack, transaction %d has target %d:%d\n",
-				proc->pid, thread->pid, in_reply_to->debug_id,
-				in_reply_to->to_proc ?
-				in_reply_to->to_proc->pid : 0,
-				in_reply_to->to_thread ?
-				in_reply_to->to_thread->pid : 0);
-			return_error = BR_FAILED_REPLY;
-			in_reply_to = NULL;
+		return_error = binder_reply_validate_stack(thread);
+		if (return_error != BR_OK)
 			goto err_bad_call_stack;
-		}
-		thread->transaction_stack = in_reply_to->to_parent;
+		in_reply_to = binder_dst_restore_transaction(thread);
+		binder_set_nice(in_reply_to->saved_priority);
 		target_thread = in_reply_to->from;
-		if (target_thread == NULL) {
+		if (!target_thread) {
 			return_error = BR_DEAD_REPLY;
 			goto err_dead_binder;
 		}
-		if (target_thread->transaction_stack != in_reply_to) {
-			binder_user_error("%d:%d got reply transaction with bad target transaction stack %d, expected %d\n",
-				proc->pid, thread->pid,
-				target_thread->transaction_stack ?
-				target_thread->transaction_stack->debug_id : 0,
-				in_reply_to->debug_id);
-			return_error = BR_FAILED_REPLY;
-			in_reply_to = NULL;
-			target_thread = NULL;
-			goto err_dead_binder;
-		}
+		BUG_ON(target_thread->transaction_stack != in_reply_to);
 		target_proc = target_thread->proc;
 	} else {
 		return_error = binder_get_tr_target_node(thread, tr,
@@ -1640,16 +1680,11 @@ static void binder_transaction(struct binder_thread *thread,
 		if (!(tr->flags & TF_ONE_WAY) && thread->transaction_stack) {
 			struct binder_transaction *tmp;
 
-			tmp = thread->transaction_stack;
-			if (tmp->to_thread != thread) {
-				binder_user_error("%d:%d got new transaction with bad transaction stack, transaction %d has target %d:%d\n",
-					proc->pid, thread->pid, tmp->debug_id,
-					tmp->to_proc ? tmp->to_proc->pid : 0,
-					tmp->to_thread ?
-					tmp->to_thread->pid : 0);
-				return_error = BR_FAILED_REPLY;
+			return_error = binder_tr_validate_stack(thread);
+			if (return_error != BR_OK)
 				goto err_bad_call_stack;
-			}
+
+			tmp = thread->transaction_stack;
 			while (tmp) {
 				if (tmp->from && tmp->from->proc == target_proc)
 					target_thread = tmp->from;
@@ -1755,8 +1790,7 @@ static void binder_transaction(struct binder_thread *thread,
 	} else if (!(t->flags & TF_ONE_WAY)) {
 		BUG_ON(t->buffer->async_transaction != 0);
 		t->need_reply = 1;
-		t->from_parent = thread->transaction_stack;
-		thread->transaction_stack = t;
+		binder_push_transaction(thread, t);
 	} else {
 		BUG_ON(target_node == NULL);
 		BUG_ON(t->buffer->async_transaction != 1);
@@ -1786,7 +1820,6 @@ err_alloc_tcomplete_failed:
 	binder_stats_deleted(BINDER_STAT_TRANSACTION);
 err_alloc_t_failed:
 err_bad_call_stack:
-err_empty_call_stack:
 err_dead_binder:
 err_invalid_target_handle:
 	binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
@@ -2579,9 +2612,8 @@ retry:
 		list_del(&t->work.entry);
 		t->buffer->allow_user_free = 1;
 		if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) {
-			t->to_parent = thread->transaction_stack;
 			t->to_thread = thread;
-			thread->transaction_stack = t;
+			binder_dst_save_transaction(thread, t);
 		} else {
 			t->buffer->transaction = NULL;
 			kfree(t);
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 08/13] android: binder: add function for logging failed transactions

[Riley Andrews]

Add another helper function that adds log entries to failed log.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ed94121..f7f2217 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -211,6 +211,16 @@ static struct binder_transaction_log_entry *binder_transaction_log_add(
 	return e;
 }
 
+static void binder_transaction_log_add_copy(
+	struct binder_transaction_log *log,
+	struct binder_transaction_log_entry *entry)
+{
+	struct binder_transaction_log_entry *failed;
+
+	failed = binder_transaction_log_add(log);
+	*failed = *entry;
+}
+
 struct binder_work {
 	struct list_head entry;
 	enum {
@@ -1827,13 +1837,7 @@ err_invalid_target_handle:
 		     proc->pid, thread->pid, return_error,
 		     (u64)tr->data_size, (u64)tr->offsets_size);
 
-	{
-		struct binder_transaction_log_entry *fe;
-
-		fe = binder_transaction_log_add(&binder_transaction_log_failed);
-		*fe = *e;
-	}
-
+	binder_transaction_log_add_copy(&binder_transaction_log_failed, e);
 	BUG_ON(thread->return_error != BR_OK);
 	if (in_reply_to) {
 		thread->return_error = BR_TRANSACTION_COMPLETE;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 09/13] android: binder: add function for finding prior thread in transaction stack

[Riley Andrews]

Add a helper function to find a thread within a transaction stack.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f7f2217..abd5556 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1634,6 +1634,19 @@ static int binder_tr_validate_stack(struct binder_thread *thread)
 	return BR_OK;
 }
 
+struct binder_thread *binder_find_used_thread(struct binder_proc *proc,
+					      struct binder_transaction *stack)
+{
+	struct binder_transaction *tr = stack;
+
+	while (tr) {
+		if (tr->from && tr->from->proc == proc)
+			return tr->from;
+		tr = tr->from_parent;
+	}
+	return NULL;
+}
+
 static void binder_transaction(struct binder_thread *thread,
 			       struct binder_transaction_data *tr, int reply)
 {
@@ -1688,18 +1701,12 @@ static void binder_transaction(struct binder_thread *thread,
 			goto err_invalid_target_handle;
 		}
 		if (!(tr->flags & TF_ONE_WAY) && thread->transaction_stack) {
-			struct binder_transaction *tmp;
-
 			return_error = binder_tr_validate_stack(thread);
 			if (return_error != BR_OK)
 				goto err_bad_call_stack;
 
-			tmp = thread->transaction_stack;
-			while (tmp) {
-				if (tmp->from && tmp->from->proc == target_proc)
-					target_thread = tmp->from;
-				tmp = tmp->from_parent;
-			}
+			target_thread = binder_find_used_thread(target_proc,
+						thread->transaction_stack);
 		}
 	}
 	if (target_thread) {
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 10/13] android: binder: refactor binder_thread_read loop

[Riley Andrews]

Add dedicated functions for every work type in the switch statement.
Refactor the loop logic to remove the while 1, and make it explicit
which work items cause the loop to exit.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 433 +++++++++++++++++++++++++----------------------
 1 file changed, 231 insertions(+), 202 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index abd5556..b69ca0a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2327,6 +2327,207 @@ static int binder_has_thread_work(struct binder_thread *thread)
 		(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
 }
 
+static int binder_work_transaction(struct binder_thread *thread,
+				   struct binder_work *w, void * __user *ptr)
+{
+	struct binder_proc *proc = thread->proc;
+	struct binder_transaction_data tr;
+	struct binder_transaction *t;
+	uint32_t cmd;
+
+	t = container_of(w, struct binder_transaction, work);
+	BUG_ON(!t->buffer);
+	if (t->buffer->target_node) {
+		struct binder_node *target_node = t->buffer->target_node;
+
+		tr.target.ptr = target_node->ptr;
+		tr.cookie =  target_node->cookie;
+		t->saved_priority = task_nice(current);
+		if (t->priority < target_node->min_priority &&
+		    !(t->flags & TF_ONE_WAY))
+			binder_set_nice(t->priority);
+		else if (!(t->flags & TF_ONE_WAY) ||
+			 t->saved_priority > target_node->min_priority)
+			binder_set_nice(target_node->min_priority);
+		cmd = BR_TRANSACTION;
+	} else {
+		tr.target.ptr = 0;
+		tr.cookie = 0;
+		cmd = BR_REPLY;
+	}
+	tr.code = t->code;
+	tr.flags = t->flags;
+	tr.sender_euid = from_kuid(current_user_ns(), t->sender_euid);
+
+	if (t->from) {
+		struct task_struct *sender = t->from->proc->tsk;
+
+		tr.sender_pid = task_tgid_nr_ns(sender,
+						task_active_pid_ns(current));
+	} else {
+		tr.sender_pid = 0;
+	}
+
+	tr.data_size = t->buffer->data_size;
+	tr.offsets_size = t->buffer->offsets_size;
+	tr.data.ptr.buffer = (binder_uintptr_t)((uintptr_t)t->buffer->data +
+				proc->user_buffer_offset);
+	tr.data.ptr.offsets = tr.data.ptr.buffer + ALIGN(t->buffer->data_size,
+				    sizeof(void *));
+
+	if (put_user(cmd, (uint32_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(uint32_t);
+	if (copy_to_user(*ptr, &tr, sizeof(tr)))
+		return -EFAULT;
+	*ptr += sizeof(tr);
+
+	trace_binder_transaction_received(t);
+	binder_stat_br(proc, thread, cmd);
+	binder_debug(BINDER_DEBUG_TRANSACTION,
+		     "%d:%d %s %d %d:%d, cmd %d size %zd-%zd ptr %016llx-%016llx\n",
+		     proc->pid, thread->pid,
+		     (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" : "BR_REPLY",
+		     t->debug_id, t->from ? t->from->proc->pid : 0,
+		     t->from ? t->from->pid : 0, cmd,
+		     t->buffer->data_size, t->buffer->offsets_size,
+		     (u64)tr.data.ptr.buffer, (u64)tr.data.ptr.offsets);
+
+	list_del(&t->work.entry);
+	t->buffer->allow_user_free = 1;
+	if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) {
+		t->to_thread = thread;
+		binder_dst_save_transaction(thread, t);
+	} else {
+		t->buffer->transaction = NULL;
+		kfree(t);
+		binder_stats_deleted(BINDER_STAT_TRANSACTION);
+	}
+	return 0;
+}
+
+static int binder_work_node(struct binder_thread *thread,
+			    struct binder_work *w, void * __user *ptr)
+{
+	struct binder_node *node = container_of(w, struct binder_node, work);
+	struct binder_proc *proc = thread->proc;
+	uint32_t cmd = BR_NOOP;
+	const char *cmd_name;
+	int strong = node->internal_strong_refs || node->local_strong_refs;
+	int weak = !hlist_empty(&node->refs) || node->local_weak_refs || strong;
+
+	if (weak && !node->has_weak_ref) {
+		cmd = BR_INCREFS;
+		cmd_name = "BR_INCREFS";
+		node->has_weak_ref = 1;
+		node->pending_weak_ref = 1;
+		node->local_weak_refs++;
+	} else if (strong && !node->has_strong_ref) {
+		cmd = BR_ACQUIRE;
+		cmd_name = "BR_ACQUIRE";
+		node->has_strong_ref = 1;
+		node->pending_strong_ref = 1;
+		node->local_strong_refs++;
+	} else if (!strong && node->has_strong_ref) {
+		cmd = BR_RELEASE;
+		cmd_name = "BR_RELEASE";
+		node->has_strong_ref = 0;
+	} else if (!weak && node->has_weak_ref) {
+		cmd = BR_DECREFS;
+		cmd_name = "BR_DECREFS";
+		node->has_weak_ref = 0;
+	}
+	if (cmd != BR_NOOP) {
+		if (put_user(cmd, (uint32_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(uint32_t);
+		if (put_user(node->ptr, (binder_uintptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(binder_uintptr_t);
+		if (put_user(node->cookie, (binder_uintptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(binder_uintptr_t);
+
+		binder_stat_br(proc, thread, cmd);
+		binder_debug(BINDER_DEBUG_USER_REFS,
+			     "%d:%d %s %d u%016llx c%016llx\n",
+			     proc->pid, thread->pid, cmd_name, node->debug_id,
+			     (u64)node->ptr, (u64)node->cookie);
+	} else {
+		list_del_init(&w->entry);
+		if (!weak && !strong) {
+			binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+				     "%d:%d node %d u%016llx c%016llx deleted\n",
+				     proc->pid, thread->pid, node->debug_id,
+				     (u64)node->ptr, (u64)node->cookie);
+			rb_erase(&node->rb_node, &proc->nodes);
+			kfree(node);
+			binder_stats_deleted(BINDER_STAT_NODE);
+		} else {
+			binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+				     "%d:%d node %d u%016llx c%016llx state unchanged\n",
+				     proc->pid, thread->pid, node->debug_id,
+				     (u64)node->ptr, (u64)node->cookie);
+		}
+	}
+	return 0;
+}
+
+static int binder_work_dead_binder(struct binder_thread *thread,
+				   struct binder_work *w, void * __user *ptr)
+{
+	struct binder_ref_death *death;
+	struct binder_proc *proc = thread->proc;
+	uint32_t cmd;
+
+	death = container_of(w, struct binder_ref_death, work);
+	if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION)
+		cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
+	else
+		cmd = BR_DEAD_BINDER;
+	if (put_user(cmd, (uint32_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(uint32_t);
+	if (put_user(death->cookie, (binder_uintptr_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(binder_uintptr_t);
+	binder_stat_br(proc, thread, cmd);
+	binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION, "%d:%d %s %016llx\n",
+		     proc->pid, thread->pid, cmd == BR_DEAD_BINDER ?
+		     "BR_DEAD_BINDER" : "BR_CLEAR_DEATH_NOTIFICATION_DONE",
+		     (u64)death->cookie);
+
+	if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION) {
+		list_del(&w->entry);
+		kfree(death);
+		binder_stats_deleted(BINDER_STAT_DEATH);
+	} else {
+		list_move(&w->entry, &proc->delivered_death);
+	}
+	return 0;
+}
+
+static int binder_work_tr_complete(struct binder_thread *thread,
+				   struct binder_work *w, void * __user *ptr)
+{
+	uint32_t cmd = BR_TRANSACTION_COMPLETE;
+	struct binder_proc *proc = thread->proc;
+
+	if (put_user(cmd, (uint32_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(uint32_t);
+
+	binder_stat_br(proc, thread, cmd);
+	binder_debug(BINDER_DEBUG_TRANSACTION_COMPLETE,
+		     "%d:%d BR_TRANSACTION_COMPLETE\n",
+		     proc->pid, thread->pid);
+
+	list_del(&w->entry);
+	kfree(w);
+	binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
+	return 0;
+}
+
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
 			      binder_uintptr_t binder_buffer, size_t size,
@@ -2408,11 +2609,9 @@ retry:
 	if (ret)
 		return ret;
 
-	while (1) {
-		uint32_t cmd;
-		struct binder_transaction_data tr;
+	while (((end - ptr) >= sizeof(struct binder_transaction_data) + 4)) {
 		struct binder_work *w;
-		struct binder_transaction *t = NULL;
+		bool done_processing_work = false;
 
 		if (!list_empty(&thread->todo)) {
 			w = list_first_entry(&thread->todo, struct binder_work,
@@ -2428,209 +2627,39 @@ retry:
 			break;
 		}
 
-		if (end - ptr < sizeof(tr) + 4)
-			break;
-
 		switch (w->type) {
-		case BINDER_WORK_TRANSACTION: {
-			t = container_of(w, struct binder_transaction, work);
-		} break;
-		case BINDER_WORK_TRANSACTION_COMPLETE: {
-			cmd = BR_TRANSACTION_COMPLETE;
-			if (put_user(cmd, (uint32_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(uint32_t);
-
-			binder_stat_br(proc, thread, cmd);
-			binder_debug(BINDER_DEBUG_TRANSACTION_COMPLETE,
-				     "%d:%d BR_TRANSACTION_COMPLETE\n",
-				     proc->pid, thread->pid);
-
-			list_del(&w->entry);
-			kfree(w);
-			binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
-		} break;
-		case BINDER_WORK_NODE: {
-			struct binder_node *node = container_of(w, struct binder_node, work);
-			uint32_t cmd = BR_NOOP;
-			const char *cmd_name;
-			int strong = node->internal_strong_refs || node->local_strong_refs;
-			int weak = !hlist_empty(&node->refs) || node->local_weak_refs || strong;
-
-			if (weak && !node->has_weak_ref) {
-				cmd = BR_INCREFS;
-				cmd_name = "BR_INCREFS";
-				node->has_weak_ref = 1;
-				node->pending_weak_ref = 1;
-				node->local_weak_refs++;
-			} else if (strong && !node->has_strong_ref) {
-				cmd = BR_ACQUIRE;
-				cmd_name = "BR_ACQUIRE";
-				node->has_strong_ref = 1;
-				node->pending_strong_ref = 1;
-				node->local_strong_refs++;
-			} else if (!strong && node->has_strong_ref) {
-				cmd = BR_RELEASE;
-				cmd_name = "BR_RELEASE";
-				node->has_strong_ref = 0;
-			} else if (!weak && node->has_weak_ref) {
-				cmd = BR_DECREFS;
-				cmd_name = "BR_DECREFS";
-				node->has_weak_ref = 0;
-			}
-			if (cmd != BR_NOOP) {
-				if (put_user(cmd, (uint32_t __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(uint32_t);
-				if (put_user(node->ptr,
-					     (binder_uintptr_t __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(binder_uintptr_t);
-				if (put_user(node->cookie,
-					     (binder_uintptr_t __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(binder_uintptr_t);
-
-				binder_stat_br(proc, thread, cmd);
-				binder_debug(BINDER_DEBUG_USER_REFS,
-					     "%d:%d %s %d u%016llx c%016llx\n",
-					     proc->pid, thread->pid, cmd_name,
-					     node->debug_id,
-					     (u64)node->ptr, (u64)node->cookie);
-			} else {
-				list_del_init(&w->entry);
-				if (!weak && !strong) {
-					binder_debug(BINDER_DEBUG_INTERNAL_REFS,
-						     "%d:%d node %d u%016llx c%016llx deleted\n",
-						     proc->pid, thread->pid,
-						     node->debug_id,
-						     (u64)node->ptr,
-						     (u64)node->cookie);
-					rb_erase(&node->rb_node, &proc->nodes);
-					kfree(node);
-					binder_stats_deleted(BINDER_STAT_NODE);
-				} else {
-					binder_debug(BINDER_DEBUG_INTERNAL_REFS,
-						     "%d:%d node %d u%016llx c%016llx state unchanged\n",
-						     proc->pid, thread->pid,
-						     node->debug_id,
-						     (u64)node->ptr,
-						     (u64)node->cookie);
-				}
-			}
-		} break;
+		case BINDER_WORK_TRANSACTION:
+			ret = binder_work_transaction(thread, w, &ptr);
+			/*
+			 * Threads can only handle one incoming transaction,
+			 * at a time, so return before processing any more
+			 * transaction work nodes.
+			 */
+			done_processing_work = true;
+			break;
+		case BINDER_WORK_TRANSACTION_COMPLETE:
+			ret = binder_work_tr_complete(thread, w, &ptr);
+			break;
+		case BINDER_WORK_NODE:
+			ret = binder_work_node(thread, w, &ptr);
+			break;
 		case BINDER_WORK_DEAD_BINDER:
 		case BINDER_WORK_DEAD_BINDER_AND_CLEAR:
-		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION: {
-			struct binder_ref_death *death;
-			uint32_t cmd;
-
-			death = container_of(w, struct binder_ref_death, work);
-			if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION)
-				cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
-			else
-				cmd = BR_DEAD_BINDER;
-			if (put_user(cmd, (uint32_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(uint32_t);
-			if (put_user(death->cookie,
-				     (binder_uintptr_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(binder_uintptr_t);
-			binder_stat_br(proc, thread, cmd);
-			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
-				     "%d:%d %s %016llx\n",
-				      proc->pid, thread->pid,
-				      cmd == BR_DEAD_BINDER ?
-				      "BR_DEAD_BINDER" :
-				      "BR_CLEAR_DEATH_NOTIFICATION_DONE",
-				      (u64)death->cookie);
-
-			if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION) {
-				list_del(&w->entry);
-				kfree(death);
-				binder_stats_deleted(BINDER_STAT_DEATH);
-			} else
-				list_move(&w->entry, &proc->delivered_death);
-			if (cmd == BR_DEAD_BINDER)
-				goto done; /* DEAD_BINDER notifications can cause transactions */
-		} break;
-		}
-
-		if (!t)
-			continue;
-
-		BUG_ON(t->buffer == NULL);
-		if (t->buffer->target_node) {
-			struct binder_node *target_node = t->buffer->target_node;
-
-			tr.target.ptr = target_node->ptr;
-			tr.cookie =  target_node->cookie;
-			t->saved_priority = task_nice(current);
-			if (t->priority < target_node->min_priority &&
-			    !(t->flags & TF_ONE_WAY))
-				binder_set_nice(t->priority);
-			else if (!(t->flags & TF_ONE_WAY) ||
-				 t->saved_priority > target_node->min_priority)
-				binder_set_nice(target_node->min_priority);
-			cmd = BR_TRANSACTION;
-		} else {
-			tr.target.ptr = 0;
-			tr.cookie = 0;
-			cmd = BR_REPLY;
-		}
-		tr.code = t->code;
-		tr.flags = t->flags;
-		tr.sender_euid = from_kuid(current_user_ns(), t->sender_euid);
-
-		if (t->from) {
-			struct task_struct *sender = t->from->proc->tsk;
-
-			tr.sender_pid = task_tgid_nr_ns(sender,
-							task_active_pid_ns(current));
-		} else {
-			tr.sender_pid = 0;
+			/*
+			 * Dead binder notifications can cause userspace to
+			 * issue transactions, so break out here so that only
+			 * one notification is given to userspace at a time.
+			 */
+			done_processing_work = true;
+		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
+			ret = binder_work_dead_binder(thread, w, &ptr);
+			break;
 		}
+		if (ret < 0)
+			return ret;
 
-		tr.data_size = t->buffer->data_size;
-		tr.offsets_size = t->buffer->offsets_size;
-		tr.data.ptr.buffer = (binder_uintptr_t)(
-					(uintptr_t)t->buffer->data +
-					proc->user_buffer_offset);
-		tr.data.ptr.offsets = tr.data.ptr.buffer +
-					ALIGN(t->buffer->data_size,
-					    sizeof(void *));
-
-		if (put_user(cmd, (uint32_t __user *)ptr))
-			return -EFAULT;
-		ptr += sizeof(uint32_t);
-		if (copy_to_user(ptr, &tr, sizeof(tr)))
-			return -EFAULT;
-		ptr += sizeof(tr);
-
-		trace_binder_transaction_received(t);
-		binder_stat_br(proc, thread, cmd);
-		binder_debug(BINDER_DEBUG_TRANSACTION,
-			     "%d:%d %s %d %d:%d, cmd %d size %zd-%zd ptr %016llx-%016llx\n",
-			     proc->pid, thread->pid,
-			     (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" :
-			     "BR_REPLY",
-			     t->debug_id, t->from ? t->from->proc->pid : 0,
-			     t->from ? t->from->pid : 0, cmd,
-			     t->buffer->data_size, t->buffer->offsets_size,
-			     (u64)tr.data.ptr.buffer, (u64)tr.data.ptr.offsets);
-
-		list_del(&t->work.entry);
-		t->buffer->allow_user_free = 1;
-		if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) {
-			t->to_thread = thread;
-			binder_dst_save_transaction(thread, t);
-		} else {
-			t->buffer->transaction = NULL;
-			kfree(t);
-			binder_stats_deleted(BINDER_STAT_TRANSACTION);
-		}
-		break;
+		if (done_processing_work)
+			break;
 	}
 
 done:
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 11/13] android: binder: add function to handle waiting for binder_thread_read

[Riley Andrews]

Add another helper function for binder_thread_read. All of the logic for
waiting for work to do has been pulled into this function.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 104 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 38 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b69ca0a..c98436c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2528,6 +2528,71 @@ static int binder_work_tr_complete(struct binder_thread *thread,
 	return 0;
 }
 
+static int binder_wait_for_thread_work(struct binder_thread *thread,
+				       bool non_block)
+{
+	if (binder_has_thread_work(thread))
+		return 0;
+
+	if (non_block)
+		return -EAGAIN;
+
+	return wait_event_freezable(thread->wait,
+				    binder_has_thread_work(thread));
+}
+
+static int binder_wait_for_proc_work(struct binder_thread *thread,
+				     bool non_block)
+{
+	struct binder_proc *proc = thread->proc;
+
+	if (!(thread->looper & (BINDER_LOOPER_STATE_REGISTERED |
+				BINDER_LOOPER_STATE_ENTERED))) {
+		binder_user_error("%d:%d ERROR: Thread waiting for process work before calling BC_REGISTER_LOOPER or BC_ENTER_LOOPER (state %x)\n",
+				  proc->pid, thread->pid,
+				  thread->looper);
+		wait_event_interruptible(binder_user_error_wait,
+					 binder_stop_on_user_error < 2);
+	}
+	binder_set_nice(proc->default_priority);
+
+	if (binder_has_proc_work(proc, thread))
+		return 0;
+
+	if (non_block)
+		return -EAGAIN;
+
+	return wait_event_freezable_exclusive(proc->wait,
+			binder_has_proc_work(proc, thread));
+}
+
+static int binder_wait_for_work(struct binder_thread *thread, int non_block,
+				int wait_for_proc_work)
+{
+	int ret;
+	struct binder_proc *proc = thread->proc;
+
+	trace_binder_wait_for_work(wait_for_proc_work,
+				   !!thread->transaction_stack,
+				   !list_empty(&thread->todo));
+	thread->looper |= BINDER_LOOPER_STATE_WAITING;
+	if (wait_for_proc_work)
+		proc->ready_threads++;
+
+	binder_unlock(__func__);
+	if (wait_for_proc_work)
+		ret = binder_wait_for_proc_work(thread, non_block);
+	else
+		ret = binder_wait_for_thread_work(thread, non_block);
+	binder_lock(__func__);
+
+	if (wait_for_proc_work)
+		proc->ready_threads--;
+	thread->looper &= ~BINDER_LOOPER_STATE_WAITING;
+
+	return ret;
+}
+
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
 			      binder_uintptr_t binder_buffer, size_t size,
@@ -2568,44 +2633,7 @@ retry:
 		goto done;
 	}
 
-
-	thread->looper |= BINDER_LOOPER_STATE_WAITING;
-	if (wait_for_proc_work)
-		proc->ready_threads++;
-
-	binder_unlock(__func__);
-
-	trace_binder_wait_for_work(wait_for_proc_work,
-				   !!thread->transaction_stack,
-				   !list_empty(&thread->todo));
-	if (wait_for_proc_work) {
-		if (!(thread->looper & (BINDER_LOOPER_STATE_REGISTERED |
-					BINDER_LOOPER_STATE_ENTERED))) {
-			binder_user_error("%d:%d ERROR: Thread waiting for process work before calling BC_REGISTER_LOOPER or BC_ENTER_LOOPER (state %x)\n",
-				proc->pid, thread->pid, thread->looper);
-			wait_event_interruptible(binder_user_error_wait,
-						 binder_stop_on_user_error < 2);
-		}
-		binder_set_nice(proc->default_priority);
-		if (non_block) {
-			if (!binder_has_proc_work(proc, thread))
-				ret = -EAGAIN;
-		} else
-			ret = wait_event_freezable_exclusive(proc->wait, binder_has_proc_work(proc, thread));
-	} else {
-		if (non_block) {
-			if (!binder_has_thread_work(thread))
-				ret = -EAGAIN;
-		} else
-			ret = wait_event_freezable(thread->wait, binder_has_thread_work(thread));
-	}
-
-	binder_lock(__func__);
-
-	if (wait_for_proc_work)
-		proc->ready_threads--;
-	thread->looper &= ~BINDER_LOOPER_STATE_WAITING;
-
+	ret = binder_wait_for_work(thread, non_block, wait_for_proc_work);
 	if (ret)
 		return ret;
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 12/13] android: binder: add function to pass thread errors to userspace

[Riley Andrews]

Add a dedicated function for handling errors.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 51 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c98436c..a3129d4 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2593,6 +2593,35 @@ static int binder_wait_for_work(struct binder_thread *thread, int non_block,
 	return ret;
 }
 
+static int binder_handle_thread_error(struct binder_thread *thread,
+				      void * __user *ptr, void __user *end)
+{
+	struct binder_proc *proc = thread->proc;
+
+	if (*ptr == end)
+		return 0;
+
+	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);
+		thread->return_error2 = BR_OK;
+
+		if (*ptr == end)
+			return 0;
+	}
+
+	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;
+	return 0;
+}
+
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
 			      binder_uintptr_t binder_buffer, size_t size,
@@ -2613,23 +2642,11 @@ static int binder_thread_read(struct binder_proc *proc,
 
 retry:
 	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);
-			thread->return_error2 = BR_OK;
-			if (ptr == end)
-				goto done;
-		}
-		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;
+			     list_empty(&thread->todo);
+	if (thread->return_error != BR_OK) {
+		ret =  binder_handle_thread_error(thread, &ptr, end);
+		if (ret < 0)
+			return ret;
 		goto done;
 	}
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 13/13] android: binder: add function for processing work nodes in binder_thread_read

[Riley Andrews]

Split up binder_thread_read into a function for waiting for work
and a function for processing the work nodes.

Signed-off-by: Riley Andrews <riandrews@android.com>
---
 drivers/android/binder.c | 96 ++++++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 40 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index a3129d4..1ae8db7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2622,39 +2622,16 @@ static int binder_handle_thread_error(struct binder_thread *thread,
 	return 0;
 }
 
-static int binder_thread_read(struct binder_proc *proc,
-			      struct binder_thread *thread,
-			      binder_uintptr_t binder_buffer, size_t size,
-			      binder_size_t *consumed, int non_block)
+static int binder_thread_read_do_work(struct binder_thread *thread,
+				      bool wait_for_proc_work,
+				      void __user *buffer,
+				      void __user *end,
+				      void * __user *ptr)
 {
-	void __user *buffer = (void __user *)(uintptr_t)binder_buffer;
-	void __user *ptr = buffer + *consumed;
-	void __user *end = buffer + size;
-
-	int ret = 0;
-	int wait_for_proc_work;
-
-	if (*consumed == 0) {
-		if (put_user(BR_NOOP, (uint32_t __user *)ptr))
-			return -EFAULT;
-		ptr += sizeof(uint32_t);
-	}
-
-retry:
-	wait_for_proc_work = thread->transaction_stack == NULL &&
-			     list_empty(&thread->todo);
-	if (thread->return_error != BR_OK) {
-		ret =  binder_handle_thread_error(thread, &ptr, end);
-		if (ret < 0)
-			return ret;
-		goto done;
-	}
-
-	ret = binder_wait_for_work(thread, non_block, wait_for_proc_work);
-	if (ret)
-		return ret;
+	int ret;
+	struct binder_proc *proc = thread->proc;
 
-	while (((end - ptr) >= sizeof(struct binder_transaction_data) + 4)) {
+	while (((end - *ptr) >= sizeof(struct binder_transaction_data) + 4)) {
 		struct binder_work *w;
 		bool done_processing_work = false;
 
@@ -2665,16 +2642,12 @@ retry:
 			w = list_first_entry(&proc->todo, struct binder_work,
 					     entry);
 		} else {
-			/* no data added */
-			if (ptr - buffer == 4 &&
-			    !(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN))
-				goto retry;
 			break;
 		}
 
 		switch (w->type) {
 		case BINDER_WORK_TRANSACTION:
-			ret = binder_work_transaction(thread, w, &ptr);
+			ret = binder_work_transaction(thread, w, ptr);
 			/*
 			 * Threads can only handle one incoming transaction,
 			 * at a time, so return before processing any more
@@ -2683,10 +2656,10 @@ retry:
 			done_processing_work = true;
 			break;
 		case BINDER_WORK_TRANSACTION_COMPLETE:
-			ret = binder_work_tr_complete(thread, w, &ptr);
+			ret = binder_work_tr_complete(thread, w, ptr);
 			break;
 		case BINDER_WORK_NODE:
-			ret = binder_work_node(thread, w, &ptr);
+			ret = binder_work_node(thread, w, ptr);
 			break;
 		case BINDER_WORK_DEAD_BINDER:
 		case BINDER_WORK_DEAD_BINDER_AND_CLEAR:
@@ -2697,7 +2670,7 @@ retry:
 			 */
 			done_processing_work = true;
 		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
-			ret = binder_work_dead_binder(thread, w, &ptr);
+			ret = binder_work_dead_binder(thread, w, ptr);
 			break;
 		}
 		if (ret < 0)
@@ -2706,8 +2679,51 @@ retry:
 		if (done_processing_work)
 			break;
 	}
+	return 0;
+}
 
-done:
+static int binder_thread_read(struct binder_proc *proc,
+			      struct binder_thread *thread,
+			      binder_uintptr_t binder_buffer, size_t size,
+			      binder_size_t *consumed, int non_block)
+{
+	void __user *buffer = (void __user *)(uintptr_t)binder_buffer;
+	void __user *ptr = buffer + *consumed;
+	void __user *end = buffer + size;
+	bool wait_for_proc_work;
+
+	int ret = 0;
+
+	if (*consumed == 0) {
+		if (put_user(BR_NOOP, (uint32_t __user *)ptr))
+			return -EFAULT;
+		ptr += sizeof(uint32_t);
+	}
+
+	do {
+		if (thread->return_error != BR_OK) {
+			ret =  binder_handle_thread_error(thread, &ptr, end);
+			if (ret < 0)
+				return ret;
+			break;
+		}
+		if (!thread->transaction_stack && list_empty(&thread->todo))
+			wait_for_proc_work = true;
+		else
+			wait_for_proc_work = false;
+
+		ret = binder_wait_for_work(thread, non_block,
+					   wait_for_proc_work);
+		if (ret)
+			return ret;
+
+		ret = binder_thread_read_do_work(thread, wait_for_proc_work,
+						 buffer, end, &ptr);
+		if (ret)
+			return ret;
+	} while ((ptr - buffer == 4) &&
+		 !(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN) &&
+		 ((end - ptr) >= sizeof(struct binder_transaction_data) + 4));
 
 	*consumed = ptr - buffer;
 	if (proc->requested_threads + proc->ready_threads == 0 &&
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 13/13] android: binder: add function for processing work nodes in binder_thread_read

[Dan Carpenter]

On Thu, May 28, 2015 at 04:08:31PM -0700, Riley Andrews wrote:
> -done:
> +static int binder_thread_read(struct binder_proc *proc,
> +			      struct binder_thread *thread,
> +			      binder_uintptr_t binder_buffer, size_t size,
> +			      binder_size_t *consumed, int non_block)
> +{
> +	void __user *buffer = (void __user *)(uintptr_t)binder_buffer;
> +	void __user *ptr = buffer + *consumed;
> +	void __user *end = buffer + size;
> +	bool wait_for_proc_work;
> +
> +	int ret = 0;
> +
> +	if (*consumed == 0) {
> +		if (put_user(BR_NOOP, (uint32_t __user *)ptr))
> +			return -EFAULT;
> +		ptr += sizeof(uint32_t);
> +	}
> +
> +	do {
> +		if (thread->return_error != BR_OK) {
> +			ret =  binder_handle_thread_error(thread, &ptr, end);
> +			if (ret < 0)
> +				return ret;
> +			break;
> +		}
> +		if (!thread->transaction_stack && list_empty(&thread->todo))
> +			wait_for_proc_work = true;
> +		else
> +			wait_for_proc_work = false;
> +
> +		ret = binder_wait_for_work(thread, non_block,
> +					   wait_for_proc_work);
> +		if (ret)
> +			return ret;
> +
> +		ret = binder_thread_read_do_work(thread, wait_for_proc_work,
> +						 buffer, end, &ptr);
> +		if (ret)
> +			return ret;
> +	} while ((ptr - buffer == 4) &&
> +		 !(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN) &&
> +		 ((end - ptr) >= sizeof(struct binder_transaction_data) + 4));

"end" and "buffer" don't change so we could move check:

		((end - ptr) >= sizeof(struct binder_transaction_data) + 4)

to the start of the function.  I may have missed something because I'm
not terribly familiar with this code.

I don't really like the way this condition is written because if "ptr"
were greater than "end" it would be true.  This seems like something
that might happen.  Pass in bwr.read_size = 1. When we do the first
ptr += sizeof(uint32_t); then "end" is less than "ptr".

This condition was there in the original code as well so it's not
something the patch introduced but it worries me every time I look at
it, even if it turns out that it's not a problem.

Please write it like:

	(ptr + sizeof(struct binder_transaction_data) + 4 <= end)

or whatever so that we don't have to think about negative numbers.

regards,
dan carpenter