Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1678096
| From | Todd Kjos <tkjos@android.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | [PATCH 36/37] binder: fix death race conditions |
| Date | 2017-06-29 21:10 +0200 |
| Message-ID | <tXMZd-354-45@gated-at.bofh.it> (permalink) |
| References | <tXMZb-354-3@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
From: Martijn Coenen <maco@google.com>
A race existed where one thread could register
a death notification for a node, while another
thread was cleaning up that node and sending
out death notifications for its references,
causing simultaneous access to ref->death
because different locks were held.
Signed-off-by: Martijn Coenen <maco@google.com>
---
drivers/android/binder.c | 64 ++++++++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 36ef88d10631..1e50b034d49a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -442,6 +442,7 @@ struct binder_ref_data {
* ref for deletion in binder_cleanup_ref, a non-NULL
* @node indicates the node must be freed
* @death: pointer to death notification (ref_death) if requested
+ * (protected by @node->lock)
*
* Structure to track references from procA to target node (on procB). This
* structure is unsafe to access without holding @proc->outer_lock.
@@ -3337,10 +3338,12 @@ static int binder_thread_write(struct binder_proc *proc,
ref->data.desc, ref->data.strong,
ref->data.weak, ref->node->debug_id);
+ binder_node_lock(ref->node);
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);
+ binder_node_unlock(ref->node);
binder_proc_unlock(proc);
kfree(death);
break;
@@ -3349,7 +3352,6 @@ static int binder_thread_write(struct binder_proc *proc,
INIT_LIST_HEAD(&death->work.entry);
death->cookie = cookie;
ref->death = death;
- binder_node_lock(ref->node);
if (ref->node->proc == NULL) {
ref->death->work.type = BINDER_WORK_DEAD_BINDER;
if (thread->looper &
@@ -3368,9 +3370,7 @@ static int binder_thread_write(struct binder_proc *proc,
&proc->wait);
}
}
- binder_node_unlock(ref->node);
} else {
- binder_node_lock(ref->node);
if (ref->death == NULL) {
binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification not active\n",
proc->pid, thread->pid);
@@ -3410,8 +3410,8 @@ static int binder_thread_write(struct binder_proc *proc,
death->work.type = BINDER_WORK_DEAD_BINDER_AND_CLEAR;
}
binder_inner_proc_unlock(proc);
- binder_node_unlock(ref->node);
}
+ binder_node_unlock(ref->node);
binder_proc_unlock(proc);
} break;
case BC_DEAD_BINDER_DONE: {
@@ -3748,44 +3748,39 @@ static int binder_thread_read(struct binder_proc *proc,
case BINDER_WORK_CLEAR_DEATH_NOTIFICATION: {
struct binder_ref_death *death;
uint32_t cmd;
+ binder_uintptr_t cookie;
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;
- /*
- * TODO: there is a race condition between
- * death notification requests and delivery
- * of the notifications. This will be handled
- * in a later patch.
- */
- binder_inner_proc_unlock(proc);
- 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);
+ cookie = death->cookie;
+
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);
-
+ (u64)cookie);
if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION) {
+ binder_inner_proc_unlock(proc);
kfree(death);
binder_stats_deleted(BINDER_STAT_DEATH);
} else {
- binder_inner_proc_lock(proc);
binder_enqueue_work_ilocked(
w, &proc->delivered_death);
binder_inner_proc_unlock(proc);
}
+ if (put_user(cmd, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (put_user(cookie,
+ (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(binder_uintptr_t);
+ binder_stat_br(proc, thread, cmd);
if (cmd == BR_DEAD_BINDER)
goto done; /* DEAD_BINDER notifications can cause transactions */
} break;
@@ -4535,20 +4530,25 @@ static int binder_node_release(struct binder_node *node, int refs)
hlist_for_each_entry(ref, &node->refs, node_entry) {
refs++;
-
- if (!ref->death)
+ /*
+ * Need the node lock to synchronize
+ * with new notification requests and the
+ * inner lock to synchronize with queued
+ * death notifications.
+ */
+ binder_inner_proc_lock(ref->proc);
+ if (!ref->death) {
+ binder_inner_proc_unlock(ref->proc);
continue;
+ }
death++;
- binder_inner_proc_lock(ref->proc);
- if (list_empty(&ref->death->work.entry)) {
- ref->death->work.type = BINDER_WORK_DEAD_BINDER;
- binder_enqueue_work_ilocked(&ref->death->work,
- &ref->proc->todo);
- wake_up_interruptible(&ref->proc->wait);
- } else
- BUG();
+ BUG_ON(!list_empty(&ref->death->work.entry));
+ ref->death->work.type = BINDER_WORK_DEAD_BINDER;
+ binder_enqueue_work_ilocked(&ref->death->work,
+ &ref->proc->todo);
+ wake_up_interruptible(&ref->proc->wait);
binder_inner_proc_unlock(ref->proc);
}
--
2.13.2.725.g09c95d1e9-goog
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 00/37] fine-grained locking in binder driver Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 30/37] binder: protect proc->nodes with inner lock Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 13/37] binder: refactor queue management in binder_thread_read Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 26/37] binder: introduce locking helper functions Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 18/37] binder: add more debug info when allocation fails. Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 24/37] binder: refactor binder ref inc/dec for thread safety Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 15/37] binder: don't modify thread->looper from other threads Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 22/37] binder: make sure target_node has strong ref Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 27/37] binder: use inner lock to sync work dq and node counts Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 10/37] binder: change binder_stats to atomics Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 36/37] binder: fix death race conditions Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
Re: [PATCH 36/37] binder: fix death race conditions Greg KH <gregkh@linuxfoundation.org> - 2017-06-30 08:10 +0200
[PATCH 19/37] binder: use atomic for transaction_log index Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 03/37] binder: Use wake up hint for synchronous transactions. Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
Re: [PATCH 03/37] binder: Use wake up hint for synchronous transactions. Greg KH <gregkh@linuxfoundation.org> - 2017-07-03 11:20 +0200
[PATCH 33/37] binder: use inner lock to protect thread accounting Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 31/37] binder: protect proc->threads with inner_lock Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 04/37] binder: separate binder allocator structure from binder proc Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 09/37] binder: add protection for non-perf cases Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 37/37] binder: remove global binder lock Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 02/37] binder: use group leader instead of open thread Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
Re: [PATCH 02/37] binder: use group leader instead of open thread Greg KH <gregkh@linuxfoundation.org> - 2017-07-03 11:20 +0200
[PATCH 12/37] binder: add log information for binder transaction failures Todd Kjos <tkjos@android.com> - 2017-06-29 21:10 +0200
[PATCH 08/37] binder: remove binder_debug_no_lock mechanism Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
[PATCH 17/37] binder: protect against two threads freeing buffer Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
[PATCH 05/37] binder: remove unneeded cleanup code Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
[PATCH 21/37] binder: guarantee txn complete / errors delivered in-order Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
[PATCH 11/37] binder: make binder_last_id an atomic Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
[PATCH 16/37] binder: remove dead code in binder_get_ref_for_node Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
[PATCH 14/37] binder: avoid race conditions when enqueuing txn Todd Kjos <tkjos@android.com> - 2017-06-29 21:20 +0200
Re: [PATCH 00/37] fine-grained locking in binder driver Greg KH <gregkh@linuxfoundation.org> - 2017-06-30 08:10 +0200
csiph-web