From 425d23f84b9b10bb461d2495f9cbe6990727300b Mon Sep 17 00:00:00 2001 From: Todd Kjos Date: Mon, 12 Jun 2017 12:07:26 -0700 Subject: [PATCH] FROMLIST: binder: protect proc->nodes with inner lock (from https://patchwork.kernel.org/patch/9817783/) When locks for binder_ref handling are added, proc->nodes will need to be modified while holding the outer lock Change-Id: I7daf5a51d83cdf6ac31a3728b3ea3e6ab94bf2e7 Signed-off-by: Todd Kjos --- drivers/android/binder.c | 112 +++++++++++++++++++++++++++++++-------- 1 file changed, 89 insertions(+), 23 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 3b066d4d4c80..a68ccc5129b8 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -312,6 +312,7 @@ struct binder_error { * @work: worklist element for node work * (protected by @proc->inner_lock) * @rb_node: element for proc->nodes tree + * (protected by @proc->inner_lock) * @dead_node: element for binder_dead_nodes list * (protected by binder_dead_nodes_lock) * @proc: binder_proc that owns this node @@ -469,6 +470,7 @@ enum binder_deferred_state { * @threads: rbtree of binder_threads in this proc * @nodes: rbtree of binder nodes associated with * this proc ordered by node->ptr + * (protected by @inner_lock) * @refs_by_desc: rbtree of refs ordered by ref->desc * @refs_by_node: rbtree of refs ordered by ref->node * @pid PID of group_leader of process @@ -855,7 +857,7 @@ static void binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer); static void binder_free_thread(struct binder_thread *thread); static void binder_free_proc(struct binder_proc *proc); -static void binder_inc_node_tmpref(struct binder_node *node); +static void binder_inc_node_tmpref_ilocked(struct binder_node *node); static int task_get_unused_fd_flags(struct binder_proc *proc, int flags) { @@ -937,12 +939,14 @@ static void binder_set_nice(long nice) binder_user_error("%d RLIMIT_NICE not set\n", current->pid); } -static struct binder_node *binder_get_node(struct binder_proc *proc, - binder_uintptr_t ptr) +static struct binder_node *binder_get_node_ilocked(struct binder_proc *proc, + binder_uintptr_t ptr) { struct rb_node *n = proc->nodes.rb_node; struct binder_node *node; + BUG_ON(!spin_is_locked(&proc->inner_lock)); + while (n) { node = rb_entry(n, struct binder_node, rb_node); @@ -956,15 +960,28 @@ static struct binder_node *binder_get_node(struct binder_proc *proc, * to ensure node stays alive until * call to binder_put_node() */ - binder_inc_node_tmpref(node); + binder_inc_node_tmpref_ilocked(node); return node; } } return NULL; } -static struct binder_node *binder_new_node(struct binder_proc *proc, - struct flat_binder_object *fp) +static struct binder_node *binder_get_node(struct binder_proc *proc, + binder_uintptr_t ptr) +{ + struct binder_node *node; + + binder_inner_proc_lock(proc); + node = binder_get_node_ilocked(proc, ptr); + binder_inner_proc_unlock(proc); + return node; +} + +static struct binder_node *binder_init_node_ilocked( + struct binder_proc *proc, + struct binder_node *new_node, + struct flat_binder_object *fp) { struct rb_node **p = &proc->nodes.rb_node; struct rb_node *parent = NULL; @@ -973,7 +990,9 @@ static struct binder_node *binder_new_node(struct binder_proc *proc, binder_uintptr_t cookie = fp ? fp->cookie : 0; __u32 flags = fp ? fp->flags : 0; + BUG_ON(!spin_is_locked(&proc->inner_lock)); while (*p) { + parent = *p; node = rb_entry(parent, struct binder_node, rb_node); @@ -981,13 +1000,17 @@ static struct binder_node *binder_new_node(struct binder_proc *proc, p = &(*p)->rb_left; else if (ptr > node->ptr) p = &(*p)->rb_right; - else - return NULL; + else { + /* + * A matching node is already in + * the rb tree. Abandon the init + * and return it. + */ + binder_inc_node_tmpref_ilocked(node); + return node; + } } - - node = kzalloc(sizeof(*node), GFP_KERNEL); - if (node == NULL) - return NULL; + node = new_node; binder_stats_created(BINDER_STAT_NODE); node->tmp_refs++; rb_link_node(&node->rb_node, parent, p); @@ -1006,6 +1029,27 @@ static struct binder_node *binder_new_node(struct binder_proc *proc, "%d:%d node %d u%016llx c%016llx created\n", proc->pid, current->pid, node->debug_id, (u64)node->ptr, (u64)node->cookie); + + return node; +} + +static struct binder_node *binder_new_node(struct binder_proc *proc, + struct flat_binder_object *fp) +{ + struct binder_node *node; + struct binder_node *new_node = kzalloc(sizeof(*node), GFP_KERNEL); + + if (!new_node) + return NULL; + binder_inner_proc_lock(proc); + node = binder_init_node_ilocked(proc, new_node, fp); + binder_inner_proc_unlock(proc); + if (node != new_node) + /* + * The node was already added by another thread + */ + kfree(new_node); + return node; } @@ -4420,6 +4464,7 @@ static void binder_deferred_release(struct binder_proc *proc) nodes = 0; incoming_refs = 0; + binder_inner_proc_lock(proc); while ((n = rb_first(&proc->nodes))) { struct binder_node *node; @@ -4430,10 +4475,13 @@ static void binder_deferred_release(struct binder_proc *proc) * calling binder_node_release() which will either * kfree() the node or call binder_put_node() */ - binder_inc_node_tmpref(node); + binder_inc_node_tmpref_ilocked(node); rb_erase(&node->rb_node, &proc->nodes); + binder_inner_proc_unlock(proc); incoming_refs = binder_node_release(node, incoming_refs); + binder_inner_proc_lock(proc); } + binder_inner_proc_unlock(proc); outgoing_refs = 0; while ((n = rb_first(&proc->refs_by_desc))) { @@ -4618,14 +4666,16 @@ static void print_binder_thread_ilocked(struct seq_file *m, m->count = start_pos; } -static void print_binder_node_nlocked(struct seq_file *m, - struct binder_node *node) +static void print_binder_node_nilocked(struct seq_file *m, + struct binder_node *node) { struct binder_ref *ref; struct binder_work *w; int count; WARN_ON(!spin_is_locked(&node->lock)); + if (node->proc) + WARN_ON(!spin_is_locked(&node->proc->inner_lock)); count = 0; hlist_for_each_entry(ref, &node->refs, node_entry) @@ -4643,11 +4693,9 @@ static void print_binder_node_nlocked(struct seq_file *m, } seq_puts(m, "\n"); if (node->proc) { - binder_inner_proc_lock(node->proc); list_for_each_entry(w, &node->async_todo, entry) print_binder_work_ilocked(m, " ", " pending async transaction", w); - binder_inner_proc_unlock(node->proc); } } @@ -4669,6 +4717,7 @@ static void print_binder_proc(struct seq_file *m, struct rb_node *n; size_t start_pos = m->count; size_t header_pos; + struct binder_node *last_node = NULL; seq_printf(m, "proc %d\n", proc->pid); seq_printf(m, "context %s\n", proc->context->name); @@ -4678,15 +4727,30 @@ static void print_binder_proc(struct seq_file *m, for (n = rb_first(&proc->threads); n != NULL; n = rb_next(n)) print_binder_thread_ilocked(m, rb_entry(n, struct binder_thread, rb_node), print_all); - binder_inner_proc_unlock(proc); + for (n = rb_first(&proc->nodes); n != NULL; n = rb_next(n)) { struct binder_node *node = rb_entry(n, struct binder_node, rb_node); - binder_node_lock(node); - if (print_all || node->has_async_transaction) - print_binder_node_nlocked(m, node); - binder_node_unlock(node); + /* + * take a temporary reference on the node so it + * survives and isn't removed from the tree + * while we print it. + */ + binder_inc_node_tmpref_ilocked(node); + /* Need to drop inner lock to take node lock */ + binder_inner_proc_unlock(proc); + if (last_node) + binder_put_node(last_node); + binder_node_inner_lock(node); + print_binder_node_nilocked(m, node); + binder_node_inner_unlock(node); + last_node = node; + binder_inner_proc_lock(proc); } + binder_inner_proc_unlock(proc); + if (last_node) + binder_put_node(last_node); + if (print_all) { for (n = rb_first(&proc->refs_by_desc); n != NULL; @@ -4822,8 +4886,10 @@ static void print_binder_proc_stats(struct seq_file *m, proc->ready_threads, binder_alloc_get_free_async_space(&proc->alloc)); count = 0; + binder_inner_proc_lock(proc); for (n = rb_first(&proc->nodes); n != NULL; n = rb_next(n)) count++; + binder_inner_proc_unlock(proc); seq_printf(m, " nodes: %d\n", count); count = 0; strong = 0; @@ -4877,7 +4943,7 @@ static int binder_state_show(struct seq_file *m, void *unused) if (last_node) binder_put_node(last_node); binder_node_lock(node); - print_binder_node_nlocked(m, node); + print_binder_node_nilocked(m, node); binder_node_unlock(node); last_node = node; spin_lock(&binder_dead_nodes_lock); -- 2.20.1