From 995a36e0402b844483e82498ca651c4a022ca608 Mon Sep 17 00:00:00 2001 From: Martijn Coenen Date: Fri, 2 Jun 2017 13:36:52 -0700 Subject: [PATCH] FROMLIST: binder: protect transaction_stack with inner lock. (from https://patchwork.kernel.org/patch/9817779/) This makes future changes to priority inheritance easier, since we want to be able to look at a thread's transaction stack when selecting a thread to inherit priority for. It also allows us to take just a single lock in a few paths, where we used to take two in succession. Change-Id: Ie30eaefe9f746577967bab76e64c49069b8a5cfa Signed-off-by: Martijn Coenen --- drivers/android/binder.c | 96 +++++++++++++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 17 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 9de2594e20bb..964a80997908 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -30,7 +30,8 @@ * 3) proc->inner_lock : protects the thread and node lists * (proc->threads, proc->nodes) and all todo lists associated * with the binder_proc (proc->todo, thread->todo, - * proc->delivered_death and node->async_todo). + * proc->delivered_death and node->async_todo), as well as + * thread->transaction_stack * binder_inner_proc_lock() and binder_inner_proc_unlock() * are used to acq/rel * @@ -566,11 +567,13 @@ enum { * @looper_needs_return: looping thread needs to exit driver * (no lock needed) * @transaction_stack: stack of in-progress transactions for this thread + * (protected by @proc->inner_lock) * @todo: list of work to do for this thread * (protected by @proc->inner_lock) * @return_error: transaction errors reported by this thread * (only accessed by this thread) * @reply_error: transaction errors reported by target thread + * (protected by @proc->inner_lock) * @wait: wait queue for thread work * @stats: per-thread statistics * (atomics, no lock needed) @@ -1644,10 +1647,11 @@ static int binder_inc_ref_for_node(struct binder_proc *proc, return ret; } -static void binder_pop_transaction(struct binder_thread *target_thread, - struct binder_transaction *t) +static void binder_pop_transaction_ilocked(struct binder_thread *target_thread, + struct binder_transaction *t) { BUG_ON(!target_thread); + BUG_ON(!spin_is_locked(&target_thread->proc->inner_lock)); BUG_ON(target_thread->transaction_stack != t); BUG_ON(target_thread->transaction_stack->from != target_thread); target_thread->transaction_stack = @@ -1731,6 +1735,35 @@ static struct binder_thread *binder_get_txn_from( return from; } +/** + * binder_get_txn_from_and_acq_inner() - get t->from and acquire inner lock + * @t: binder transaction for t->from + * + * Same as binder_get_txn_from() except it also acquires the proc->inner_lock + * to guarantee that the thread cannot be released while operating on it. + * The caller must call binder_inner_proc_unlock() to release the inner lock + * as well as call binder_dec_thread_txn() to release the reference. + * + * Return: the value of t->from + */ +static struct binder_thread *binder_get_txn_from_and_acq_inner( + struct binder_transaction *t) +{ + struct binder_thread *from; + + from = binder_get_txn_from(t); + if (!from) + return NULL; + binder_inner_proc_lock(from->proc); + if (t->from) { + BUG_ON(from != t->from); + return from; + } + binder_inner_proc_unlock(from->proc); + binder_thread_dec_tmpref(from); + return NULL; +} + static void binder_free_transaction(struct binder_transaction *t) { if (t->buffer) @@ -1747,7 +1780,7 @@ static void binder_send_failed_reply(struct binder_transaction *t, BUG_ON(t->flags & TF_ONE_WAY); while (1) { - target_thread = binder_get_txn_from(t); + target_thread = binder_get_txn_from_and_acq_inner(t); if (target_thread) { binder_debug(BINDER_DEBUG_FAILED_TRANSACTION, "send failed reply for transaction %d to %d:%d\n", @@ -1755,11 +1788,10 @@ static void binder_send_failed_reply(struct binder_transaction *t, target_thread->proc->pid, target_thread->pid); - binder_pop_transaction(target_thread, t); + binder_pop_transaction_ilocked(target_thread, t); if (target_thread->reply_error.cmd == BR_OK) { target_thread->reply_error.cmd = error_code; - binder_enqueue_work( - target_thread->proc, + binder_enqueue_work_ilocked( &target_thread->reply_error.work, &target_thread->todo); wake_up_interruptible(&target_thread->wait); @@ -1767,6 +1799,7 @@ static void binder_send_failed_reply(struct binder_transaction *t, WARN(1, "Unexpected reply error: %u\n", target_thread->reply_error.cmd); } + binder_inner_proc_unlock(target_thread->proc); binder_thread_dec_tmpref(target_thread); binder_free_transaction(t); return; @@ -2396,8 +2429,10 @@ static void binder_transaction(struct binder_proc *proc, e->context_name = proc->context->name; if (reply) { + binder_inner_proc_lock(proc); in_reply_to = thread->transaction_stack; if (in_reply_to == NULL) { + binder_inner_proc_unlock(proc); binder_user_error("%d:%d got reply transaction with no transaction stack\n", proc->pid, thread->pid); return_error = BR_FAILED_REPLY; @@ -2405,7 +2440,6 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_empty_call_stack; } - binder_set_nice(in_reply_to->saved_priority); if (in_reply_to->to_thread != thread) { spin_lock(&in_reply_to->lock); binder_user_error("%d:%d got reply transaction with bad transaction stack, transaction %d has target %d:%d\n", @@ -2415,6 +2449,7 @@ static void binder_transaction(struct binder_proc *proc, in_reply_to->to_thread ? in_reply_to->to_thread->pid : 0); spin_unlock(&in_reply_to->lock); + binder_inner_proc_unlock(proc); return_error = BR_FAILED_REPLY; return_error_param = -EPROTO; return_error_line = __LINE__; @@ -2422,7 +2457,9 @@ static void binder_transaction(struct binder_proc *proc, goto err_bad_call_stack; } thread->transaction_stack = in_reply_to->to_parent; - target_thread = binder_get_txn_from(in_reply_to); + binder_inner_proc_unlock(proc); + binder_set_nice(in_reply_to->saved_priority); + target_thread = binder_get_txn_from_and_acq_inner(in_reply_to); if (target_thread == NULL) { return_error = BR_DEAD_REPLY; return_error_line = __LINE__; @@ -2434,6 +2471,7 @@ static void binder_transaction(struct binder_proc *proc, target_thread->transaction_stack ? target_thread->transaction_stack->debug_id : 0, in_reply_to->debug_id); + binder_inner_proc_unlock(target_thread->proc); return_error = BR_FAILED_REPLY; return_error_param = -EPROTO; return_error_line = __LINE__; @@ -2443,6 +2481,7 @@ static void binder_transaction(struct binder_proc *proc, } target_proc = target_thread->proc; target_proc->tmp_ref++; + binder_inner_proc_unlock(target_thread->proc); } else { if (tr->target.handle) { struct binder_ref *ref; @@ -2499,6 +2538,7 @@ static void binder_transaction(struct binder_proc *proc, return_error_line = __LINE__; goto err_invalid_target_handle; } + binder_inner_proc_lock(proc); if (!(tr->flags & TF_ONE_WAY) && thread->transaction_stack) { struct binder_transaction *tmp; @@ -2511,6 +2551,7 @@ static void binder_transaction(struct binder_proc *proc, tmp->to_thread ? tmp->to_thread->pid : 0); spin_unlock(&tmp->lock); + binder_inner_proc_unlock(proc); return_error = BR_FAILED_REPLY; return_error_param = -EPROTO; return_error_line = __LINE__; @@ -2531,6 +2572,7 @@ static void binder_transaction(struct binder_proc *proc, tmp = tmp->from_parent; } } + binder_inner_proc_unlock(proc); } if (target_thread) { e->to_thread = target_thread->pid; @@ -2811,23 +2853,34 @@ static void binder_transaction(struct binder_proc *proc, t->work.type = BINDER_WORK_TRANSACTION; if (reply) { - if (target_thread->is_dead) + binder_inner_proc_lock(target_proc); + if (target_thread->is_dead) { + binder_inner_proc_unlock(target_proc); goto err_dead_proc_or_thread; + } BUG_ON(t->buffer->async_transaction != 0); - binder_pop_transaction(target_thread, in_reply_to); + binder_pop_transaction_ilocked(target_thread, in_reply_to); + binder_enqueue_work_ilocked(&t->work, target_list); + binder_inner_proc_unlock(target_proc); binder_free_transaction(in_reply_to); - binder_enqueue_work(target_proc, &t->work, target_list); } else if (!(t->flags & TF_ONE_WAY)) { BUG_ON(t->buffer->async_transaction != 0); + binder_inner_proc_lock(proc); t->need_reply = 1; t->from_parent = thread->transaction_stack; thread->transaction_stack = t; + binder_inner_proc_unlock(proc); + binder_inner_proc_lock(target_proc); if (target_proc->is_dead || (target_thread && target_thread->is_dead)) { - binder_pop_transaction(thread, t); + binder_inner_proc_unlock(target_proc); + binder_inner_proc_lock(proc); + binder_pop_transaction_ilocked(thread, t); + binder_inner_proc_unlock(proc); goto err_dead_proc_or_thread; } - binder_enqueue_work(target_proc, &t->work, target_list); + binder_enqueue_work_ilocked(&t->work, target_list); + binder_inner_proc_unlock(target_proc); } else { BUG_ON(target_node == NULL); BUG_ON(t->buffer->async_transaction != 1); @@ -2842,12 +2895,15 @@ static void binder_transaction(struct binder_proc *proc, * must be atomic with enqueue on * async_todo */ + binder_inner_proc_lock(target_proc); if (target_proc->is_dead || (target_thread && target_thread->is_dead)) { + binder_inner_proc_unlock(target_proc); binder_node_unlock(target_node); goto err_dead_proc_or_thread; } - binder_enqueue_work(target_proc, &t->work, target_list); + binder_enqueue_work_ilocked(&t->work, target_list); + binder_inner_proc_unlock(target_proc); binder_node_unlock(target_node); } if (target_wait) { @@ -3464,8 +3520,10 @@ static int binder_thread_read(struct binder_proc *proc, } retry: + binder_inner_proc_lock(proc); wait_for_proc_work = thread->transaction_stack == NULL && - binder_worklist_empty(proc, &thread->todo); + binder_worklist_empty_ilocked(&thread->todo); + binder_inner_proc_unlock(proc); thread->looper |= BINDER_LOOPER_STATE_WAITING; if (wait_for_proc_work) @@ -3777,9 +3835,11 @@ retry: binder_thread_dec_tmpref(t_from); t->buffer->allow_user_free = 1; if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) { + binder_inner_proc_lock(thread->proc); t->to_parent = thread->transaction_stack; t->to_thread = thread; thread->transaction_stack = t; + binder_inner_proc_unlock(thread->proc); } else { binder_free_transaction(t); } @@ -4017,8 +4077,10 @@ static unsigned int binder_poll(struct file *filp, thread = binder_get_thread(proc); + binder_inner_proc_lock(thread->proc); wait_for_proc_work = thread->transaction_stack == NULL && - binder_worklist_empty(proc, &thread->todo); + binder_worklist_empty_ilocked(&thread->todo); + binder_inner_proc_unlock(thread->proc); binder_unlock(__func__); -- 2.20.1