From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753745AbdF2TI3 (ORCPT ); Thu, 29 Jun 2017 15:08:29 -0400 Received: from mail-pg0-f50.google.com ([74.125.83.50]:34808 "EHLO mail-pg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752506AbdF2TD2 (ORCPT ); Thu, 29 Jun 2017 15:03:28 -0400 From: Todd Kjos X-Google-Original-From: Todd Kjos To: gregkh@linuxfoundation.org, arve@android.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, maco@google.com, tkjos@google.com Subject: [PATCH 25/37] binder: use node->tmp_refs to ensure node safety Date: Thu, 29 Jun 2017 12:01:59 -0700 Message-Id: <20170629190211.16927-26-tkjos@google.com> X-Mailer: git-send-email 2.13.2.725.g09c95d1e9-goog In-Reply-To: <20170629190211.16927-1-tkjos@google.com> References: <20170629190211.16927-1-tkjos@google.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org When obtaining a node via binder_get_node(), binder_get_node_from_ref() or binder_new_node(), increment node->tmp_refs to take a temporary reference on the node to ensure the node persists while being used. binder_put_node() must be called to remove the temporary reference. Signed-off-by: Todd Kjos --- drivers/android/binder.c | 124 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 104 insertions(+), 20 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 4d0b99862339..ec050c6d1192 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -274,6 +274,7 @@ struct binder_node { int internal_strong_refs; int local_weak_refs; int local_strong_refs; + int tmp_refs; binder_uintptr_t ptr; binder_uintptr_t cookie; unsigned has_strong_ref:1; @@ -427,6 +428,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 int task_get_unused_fd_flags(struct binder_proc *proc, int flags) { @@ -521,8 +523,15 @@ static struct binder_node *binder_get_node(struct binder_proc *proc, n = n->rb_left; else if (ptr > node->ptr) n = n->rb_right; - else + else { + /* + * take an implicit weak reference + * to ensure node stays alive until + * call to binder_put_node() + */ + binder_inc_node_tmpref(node); return node; + } } return NULL; } @@ -551,6 +560,7 @@ static struct binder_node *binder_new_node(struct binder_proc *proc, if (node == NULL) return NULL; binder_stats_created(BINDER_STAT_NODE); + node->tmp_refs++; rb_link_node(&node->rb_node, parent, p); rb_insert_color(&node->rb_node, &proc->nodes); node->debug_id = atomic_inc_return(&binder_last_id); @@ -615,7 +625,8 @@ static int binder_dec_node(struct binder_node *node, int strong, int internal) } else { if (!internal) node->local_weak_refs--; - if (node->local_weak_refs || !hlist_empty(&node->refs)) + if (node->local_weak_refs || node->tmp_refs || + !hlist_empty(&node->refs)) return 0; } if (node->proc && (node->has_strong_ref || node->has_weak_ref)) { @@ -625,7 +636,7 @@ static int binder_dec_node(struct binder_node *node, int strong, int internal) } } else { if (hlist_empty(&node->refs) && !node->local_strong_refs && - !node->local_weak_refs) { + !node->local_weak_refs && !node->tmp_refs) { list_del_init(&node->work.entry); if (node->proc) { rb_erase(&node->rb_node, &node->proc->nodes); @@ -648,6 +659,46 @@ static int binder_dec_node(struct binder_node *node, int strong, int internal) return 0; } +/** + * binder_inc_node_tmpref() - take a temporary reference on node + * @node: node to reference + * + * Take reference on node to prevent the node from being freed + * while referenced only by a local variable + */ +static void binder_inc_node_tmpref(struct binder_node *node) +{ + /* + * No call to binder_inc_node() is needed since we + * don't need to inform userspace of any changes to + * tmp_refs + */ + node->tmp_refs++; +} + +/** + * binder_dec_node_tmpref() - remove a temporary reference on node + * @node: node to reference + * + * Release temporary reference on node taken via binder_inc_node_tmpref() + */ +static void binder_dec_node_tmpref(struct binder_node *node) +{ + node->tmp_refs--; + BUG_ON(node->tmp_refs < 0); + /* + * Call binder_dec_node() to check if all refcounts are 0 + * and cleanup is needed. Calling with strong=0 and internal=1 + * causes no actual reference to be released in binder_dec_node(). + * If that changes, a change is needed here too. + */ + binder_dec_node(node, 0, 1); +} + +static void binder_put_node(struct binder_node *node) +{ + binder_dec_node_tmpref(node); +} static struct binder_ref *binder_get_ref(struct binder_proc *proc, u32 desc, bool need_strong_ref) @@ -888,6 +939,11 @@ static struct binder_node *binder_get_node_from_ref( if (!ref) goto err_no_ref; node = ref->node; + /* + * Take an implicit reference on the node to ensure + * it stays alive until the call to binder_put_node() + */ + binder_inc_node_tmpref(node); if (rdata) *rdata = ref->data; @@ -1348,6 +1404,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, node->debug_id, (u64)node->ptr); binder_dec_node(node, hdr->type == BINDER_TYPE_BINDER, 0); + binder_put_node(node); } break; case BINDER_TYPE_HANDLE: case BINDER_TYPE_WEAK_HANDLE: { @@ -1441,7 +1498,7 @@ static int binder_translate_binder(struct flat_binder_object *fp, struct binder_proc *proc = thread->proc; struct binder_proc *target_proc = t->to_proc; struct binder_ref_data rdata; - int ret; + int ret = 0; node = binder_get_node(proc, fp->binder); if (!node) { @@ -1457,16 +1514,19 @@ static int binder_translate_binder(struct flat_binder_object *fp, proc->pid, thread->pid, (u64)fp->binder, node->debug_id, (u64)fp->cookie, (u64)node->cookie); - return -EINVAL; + ret = -EINVAL; + goto done; + } + if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) { + ret = -EPERM; + goto done; } - if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) - return -EPERM; ret = binder_inc_ref_for_node(target_proc, node, fp->hdr.type == BINDER_TYPE_BINDER, &thread->todo, &rdata); if (ret) - return ret; + goto done; if (fp->hdr.type == BINDER_TYPE_BINDER) fp->hdr.type = BINDER_TYPE_HANDLE; @@ -1481,7 +1541,9 @@ static int binder_translate_binder(struct flat_binder_object *fp, " node %d u%016llx -> ref %d desc %d\n", node->debug_id, (u64)node->ptr, rdata.debug_id, rdata.desc); - return 0; +done: + binder_put_node(node); + return ret; } static int binder_translate_handle(struct flat_binder_object *fp, @@ -1492,6 +1554,7 @@ static int binder_translate_handle(struct flat_binder_object *fp, struct binder_proc *target_proc = t->to_proc; struct binder_node *node; struct binder_ref_data src_rdata; + int ret = 0; node = binder_get_node_from_ref(proc, fp->handle, fp->hdr.type == BINDER_TYPE_HANDLE, &src_rdata); @@ -1500,8 +1563,10 @@ static int binder_translate_handle(struct flat_binder_object *fp, proc->pid, thread->pid, fp->handle); return -EINVAL; } - if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) - return -EPERM; + if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) { + ret = -EPERM; + goto done; + } if (node->proc == target_proc) { if (fp->hdr.type == BINDER_TYPE_HANDLE) @@ -1526,7 +1591,7 @@ static int binder_translate_handle(struct flat_binder_object *fp, fp->hdr.type == BINDER_TYPE_HANDLE, NULL, &dest_rdata); if (ret) - return ret; + goto done; fp->binder = 0; fp->handle = dest_rdata.desc; @@ -1539,7 +1604,9 @@ static int binder_translate_handle(struct flat_binder_object *fp, dest_rdata.debug_id, dest_rdata.desc, node->debug_id); } - return 0; +done: + binder_put_node(node); + return ret; } static int binder_translate_fd(int fd, @@ -2381,6 +2448,7 @@ static int binder_thread_write(struct binder_proc *proc, "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE", (u64)node_ptr, node->debug_id, (u64)cookie, (u64)node->cookie); + binder_put_node(node); break; } if (cmd == BC_ACQUIRE_DONE) { @@ -2388,6 +2456,7 @@ static int binder_thread_write(struct binder_proc *proc, binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n", proc->pid, thread->pid, node->debug_id); + binder_put_node(node); break; } node->pending_strong_ref = 0; @@ -2396,16 +2465,19 @@ static int binder_thread_write(struct binder_proc *proc, binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n", proc->pid, thread->pid, node->debug_id); + binder_put_node(node); 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", + "%d:%d %s node %d ls %d lw %d tr %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); + node->debug_id, node->local_strong_refs, + node->local_weak_refs, node->tmp_refs); + binder_put_node(node); break; } case BC_ATTEMPT_ACQUIRE: @@ -2845,7 +2917,8 @@ static int binder_thread_read(struct binder_proc *proc, strong = node->internal_strong_refs || node->local_strong_refs; weak = !hlist_empty(&node->refs) || - node->local_weak_refs || strong; + node->local_weak_refs || + node->tmp_refs || strong; has_strong_ref = node->has_strong_ref; has_weak_ref = node->has_weak_ref; @@ -3357,6 +3430,7 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp) new_node->has_strong_ref = 1; new_node->has_weak_ref = 1; context->binder_context_mgr_node = new_node; + binder_put_node(new_node); out: mutex_unlock(&context->context_mgr_node_lock); return ret; @@ -3615,8 +3689,11 @@ static int binder_node_release(struct binder_node *node, int refs) list_del_init(&node->work.entry); binder_release_work(&node->async_todo); - - if (hlist_empty(&node->refs)) { + /* + * The caller must have taken a temporary ref on the node, + */ + BUG_ON(!node->tmp_refs); + if (hlist_empty(&node->refs) && node->tmp_refs == 1) { kfree(node); binder_stats_deleted(BINDER_STAT_NODE); @@ -3651,6 +3728,7 @@ static int binder_node_release(struct binder_node *node, int refs) binder_debug(BINDER_DEBUG_DEAD_BINDER, "node %d now dead, refs %d, death %d\n", node->debug_id, refs, death); + binder_put_node(node); return refs; } @@ -3700,6 +3778,12 @@ static void binder_deferred_release(struct binder_proc *proc) node = rb_entry(n, struct binder_node, rb_node); nodes++; + /* + * take a temporary ref on the node before + * calling binder_node_release() which will either + * kfree() the node or call binder_put_node() + */ + binder_inc_node_tmpref(node); rb_erase(&node->rb_node, &proc->nodes); incoming_refs = binder_node_release(node, incoming_refs); } @@ -3895,11 +3979,11 @@ static void print_binder_node(struct seq_file *m, struct binder_node *node) hlist_for_each_entry(ref, &node->refs, node_entry) count++; - seq_printf(m, " node %d: u%016llx c%016llx hs %d hw %d ls %d lw %d is %d iw %d", + seq_printf(m, " node %d: u%016llx c%016llx hs %d hw %d ls %d lw %d is %d iw %d tr %d", node->debug_id, (u64)node->ptr, (u64)node->cookie, node->has_strong_ref, node->has_weak_ref, node->local_strong_refs, node->local_weak_refs, - node->internal_strong_refs, count); + node->internal_strong_refs, count, node->tmp_refs); if (count) { seq_puts(m, " proc"); hlist_for_each_entry(ref, &node->refs, node_entry) -- 2.13.2.725.g09c95d1e9-goog