From: Todd Kjos <tkjos@android.com>
To: gregkh@linuxfoundation.org, arve@android.com,
devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
maco@google.com, tkjos@google.com
Subject: [PATCH 32/37] binder: protect transaction_stack with inner lock.
Date: Thu, 29 Jun 2017 12:02:06 -0700 [thread overview]
Message-ID: <20170629190211.16927-33-tkjos@google.com> (raw)
In-Reply-To: <20170629190211.16927-1-tkjos@google.com>
From: Martijn Coenen <maco@google.com>
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.
Signed-off-by: Martijn Coenen <maco@google.com>
---
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 5deb9453dee4..9d18ca1f7dcc 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
*
@@ -567,11 +568,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 @@ static int binder_thread_read(struct binder_proc *proc,
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.13.2.725.g09c95d1e9-goog
next prev parent reply other threads:[~2017-06-29 19:06 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-29 19:01 [PATCH 00/37] fine-grained locking in binder driver Todd Kjos
2017-06-29 19:01 ` [PATCH 01/37] Revert "android: binder: Sanity check at binder ioctl" Todd Kjos
2017-07-03 9:17 ` Greg KH
[not found] ` <CAHRSSExh9JX5xiSRig55DSei31C_BPSasOKB+BTC6jjjuZ+ZpA@mail.gmail.com>
2017-07-05 18:47 ` Greg KH
2017-06-29 19:01 ` [PATCH 02/37] binder: use group leader instead of open thread Todd Kjos
2017-07-03 9:17 ` Greg KH
[not found] ` <CAHRSSEyH3t0igLJqcC4e-HR68RH0bg4T310jnRHZzrMChoOeOg@mail.gmail.com>
2017-07-05 18:47 ` Greg KH
2017-07-07 18:23 ` Todd Kjos
2017-07-07 18:29 ` Greg KH
2017-07-24 21:00 ` John Stultz
2017-07-24 21:07 ` John Stultz
2017-07-25 9:13 ` Martijn Coenen
2017-07-27 9:08 ` Amit Pundir
2017-07-27 13:23 ` Greg Kroah-Hartman
2017-07-27 13:40 ` Martijn Coenen
2017-07-27 13:42 ` Amit Pundir
2017-07-28 11:58 ` Martijn Coenen
2017-07-24 21:23 ` Greg Kroah-Hartman
2017-07-24 21:53 ` John Stultz
2017-06-29 19:01 ` [PATCH 03/37] binder: Use wake up hint for synchronous transactions Todd Kjos
2017-07-03 9:18 ` Greg KH
2017-06-29 19:01 ` [PATCH 04/37] binder: separate binder allocator structure from binder proc Todd Kjos
2017-06-29 19:01 ` [PATCH 05/37] binder: remove unneeded cleanup code Todd Kjos
2017-06-29 19:01 ` [PATCH 06/37] binder: separate out binder_alloc functions Todd Kjos
2017-06-29 19:01 ` [PATCH 07/37] binder: move binder_alloc to separate file Todd Kjos
2017-06-29 19:01 ` [PATCH 08/37] binder: remove binder_debug_no_lock mechanism Todd Kjos
2017-06-29 19:01 ` [PATCH 09/37] binder: add protection for non-perf cases Todd Kjos
2017-06-29 19:01 ` [PATCH 10/37] binder: change binder_stats to atomics Todd Kjos
2017-06-29 19:01 ` [PATCH 11/37] binder: make binder_last_id an atomic Todd Kjos
2017-06-29 19:01 ` [PATCH 12/37] binder: add log information for binder transaction failures Todd Kjos
2017-06-29 19:01 ` [PATCH 13/37] binder: refactor queue management in binder_thread_read Todd Kjos
2017-06-29 19:01 ` [PATCH 14/37] binder: avoid race conditions when enqueuing txn Todd Kjos
2017-06-29 19:01 ` [PATCH 15/37] binder: don't modify thread->looper from other threads Todd Kjos
2017-06-29 19:01 ` [PATCH 16/37] binder: remove dead code in binder_get_ref_for_node Todd Kjos
2017-06-29 19:01 ` [PATCH 17/37] binder: protect against two threads freeing buffer Todd Kjos
2017-06-29 19:01 ` [PATCH 18/37] binder: add more debug info when allocation fails Todd Kjos
2017-06-29 19:01 ` [PATCH 19/37] binder: use atomic for transaction_log index Todd Kjos
2017-06-29 19:01 ` [PATCH 20/37] binder: refactor binder_pop_transaction Todd Kjos
2017-06-29 19:01 ` [PATCH 21/37] binder: guarantee txn complete / errors delivered in-order Todd Kjos
2017-06-29 19:01 ` [PATCH 22/37] binder: make sure target_node has strong ref Todd Kjos
2017-06-29 19:01 ` [PATCH 23/37] binder: make sure accesses to proc/thread are safe Todd Kjos
2017-06-29 19:01 ` [PATCH 24/37] binder: refactor binder ref inc/dec for thread safety Todd Kjos
2017-06-29 19:01 ` [PATCH 25/37] binder: use node->tmp_refs to ensure node safety Todd Kjos
2017-06-29 19:02 ` [PATCH 26/37] binder: introduce locking helper functions Todd Kjos
2017-06-29 19:02 ` [PATCH 27/37] binder: use inner lock to sync work dq and node counts Todd Kjos
2017-06-29 19:02 ` [PATCH 28/37] binder: add spinlocks to protect todo lists Todd Kjos
2017-06-29 19:02 ` [PATCH 29/37] binder: add spinlock to protect binder_node Todd Kjos
2017-06-29 19:02 ` [PATCH 30/37] binder: protect proc->nodes with inner lock Todd Kjos
2017-06-29 19:02 ` [PATCH 31/37] binder: protect proc->threads with inner_lock Todd Kjos
2017-06-29 19:02 ` Todd Kjos [this message]
2017-06-29 19:02 ` [PATCH 33/37] binder: use inner lock to protect thread accounting Todd Kjos
2017-06-29 19:02 ` [PATCH 34/37] binder: protect binder_ref with outer lock Todd Kjos
2017-06-29 19:02 ` [PATCH 35/37] binder: protect against stale pointers in print_binder_transaction Todd Kjos
2017-06-29 19:02 ` [PATCH 36/37] binder: fix death race conditions Todd Kjos
2017-06-30 6:05 ` Greg KH
2017-06-29 19:02 ` [PATCH 37/37] binder: remove global binder lock Todd Kjos
2017-06-30 6:04 ` [PATCH 00/37] fine-grained locking in binder driver Greg KH
2017-07-17 12:49 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170629190211.16927-33-tkjos@google.com \
--to=tkjos@android.com \
--cc=arve@android.com \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maco@google.com \
--cc=tkjos@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).