| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aBJvBRPlh46j9Q9e@google.com>
Date: Wed, 30 Apr 2025 18:42:13 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: "Tiffany Y. Yang" <ynaffit@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joel@...lfernandes.org>,
Christian Brauner <brauner@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>,
linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH 1/2] binder: Refactor binder_node print synchronization
On Thu, Mar 27, 2025 at 10:02:25PM +0000, Tiffany Y. Yang wrote:
> The binder driver outputs information about each dead binder node by
> iterating over the dead nodes list, and it prints the state of each live
> node in the system by traversing each binder_proc's proc->nodes tree.
> Both cases require similar logic to maintain the global lock ordering
> while accessing each node.
>
> Create a helper function to synchronize around printing binder nodes in
> a list. Opportunistically make minor cosmetic changes to binder print
> functions.
>
> Signed-off-by: Tiffany Y. Yang <ynaffit@...gle.com>
> ---
> drivers/android/binder.c | 114 +++++++++++++++++++++------------------
> 1 file changed, 63 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 76052006bd87..d963a7860aa3 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6377,10 +6377,10 @@ static void print_binder_transaction_ilocked(struct seq_file *m,
> }
>
> static void print_binder_work_ilocked(struct seq_file *m,
> - struct binder_proc *proc,
> - const char *prefix,
> - const char *transaction_prefix,
> - struct binder_work *w)
> + struct binder_proc *proc,
> + const char *prefix,
> + const char *transaction_prefix,
> + struct binder_work *w)
> {
> struct binder_node *node;
> struct binder_transaction *t;
> @@ -6430,7 +6430,7 @@ static void print_binder_work_ilocked(struct seq_file *m,
>
> static void print_binder_thread_ilocked(struct seq_file *m,
> struct binder_thread *thread,
> - int print_always)
> + bool print_always)
> {
> struct binder_transaction *t;
> struct binder_work *w;
> @@ -6505,8 +6505,50 @@ static void print_binder_ref_olocked(struct seq_file *m,
> binder_node_unlock(ref->node);
> }
>
> -static void print_binder_proc(struct seq_file *m,
> - struct binder_proc *proc, int print_all)
> +/**
> + * print_next_binder_node_ilocked() - Print binder_node from a locked list
> + * @m: struct seq_file for output via seq_printf()
> + * @node: struct binder_node to print fields of
> + * @prev_node: struct binder_node we hold a temporary reference to (if any)
> + *
> + * Helper function to handle synchronization around printing a struct
> + * binder_node while iterating through @node->proc->nodes or the dead nodes
> + * list. Caller must hold either @node->proc->inner_lock (for live nodes) or
> + * binder_dead_nodes_lock.
nit: kinda obvious but could you specify that these locks get released
and reaquired? only to be explicit about this functionality.
> + *
> + * Return: pointer to the struct binder_node we hold a tmpref on
> + */
> +static struct binder_node *
> +print_next_binder_node_ilocked(struct seq_file *m, struct binder_node *node,
> + struct binder_node *prev_node)
> +{
> + /*
> + * Take a temporary reference on the node so that isn't removed from
> + * its proc's tree or the dead nodes list while we print it.
> + */
> + binder_inc_node_tmpref_ilocked(node);
> + /*
> + * Live nodes need to drop the inner proc lock and dead nodes need to
> + * drop the binder_dead_nodes_lock before trying to take the node lock.
> + */
> + if (node->proc)
> + binder_inner_proc_unlock(node->proc);
> + else
> + spin_unlock(&binder_dead_nodes_lock);
> + if (prev_node)
> + binder_put_node(prev_node);
> + binder_node_inner_lock(node);
> + print_binder_node_nilocked(m, node);
> + binder_node_inner_unlock(node);
> + if (node->proc)
> + binder_inner_proc_lock(node->proc);
> + else
> + spin_lock(&binder_dead_nodes_lock);
> + return node;
> +}
> +
> +static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
> + bool print_all)
> {
> struct binder_work *w;
> struct rb_node *n;
> @@ -6519,31 +6561,17 @@ static void print_binder_proc(struct seq_file *m,
> header_pos = m->count;
>
> binder_inner_proc_lock(proc);
> - for (n = rb_first(&proc->threads); n != NULL; n = rb_next(n))
> + for (n = rb_first(&proc->threads); n; n = rb_next(n))
> print_binder_thread_ilocked(m, rb_entry(n, struct binder_thread,
> rb_node), print_all);
>
> - for (n = rb_first(&proc->nodes); n != NULL; n = rb_next(n)) {
> + for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
> struct binder_node *node = rb_entry(n, struct binder_node,
> rb_node);
> if (!print_all && !node->has_async_transaction)
> continue;
>
> - /*
> - * 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);
> + last_node = print_next_binder_node_ilocked(m, node, last_node);
> }
> binder_inner_proc_unlock(proc);
> if (last_node)
> @@ -6551,12 +6579,10 @@ static void print_binder_proc(struct seq_file *m,
>
> if (print_all) {
> binder_proc_lock(proc);
> - for (n = rb_first(&proc->refs_by_desc);
> - n != NULL;
> - n = rb_next(n))
> + for (n = rb_first(&proc->refs_by_desc); n; n = rb_next(n))
> print_binder_ref_olocked(m, rb_entry(n,
> - struct binder_ref,
> - rb_node_desc));
> + struct binder_ref,
> + rb_node_desc));
> binder_proc_unlock(proc);
> }
> binder_alloc_print_allocated(m, &proc->alloc);
> @@ -6696,7 +6722,7 @@ static void print_binder_proc_stats(struct seq_file *m,
> count = 0;
> ready_threads = 0;
> binder_inner_proc_lock(proc);
> - for (n = rb_first(&proc->threads); n != NULL; n = rb_next(n))
> + for (n = rb_first(&proc->threads); n; n = rb_next(n))
> count++;
>
> list_for_each_entry(thread, &proc->waiting_threads, waiting_thread_node)
> @@ -6710,7 +6736,7 @@ static void print_binder_proc_stats(struct seq_file *m,
> ready_threads,
> free_async_space);
> count = 0;
> - for (n = rb_first(&proc->nodes); n != NULL; n = rb_next(n))
> + for (n = rb_first(&proc->nodes); n; n = rb_next(n))
> count++;
> binder_inner_proc_unlock(proc);
> seq_printf(m, " nodes: %d\n", count);
> @@ -6718,7 +6744,7 @@ static void print_binder_proc_stats(struct seq_file *m,
> strong = 0;
> weak = 0;
> binder_proc_lock(proc);
> - for (n = rb_first(&proc->refs_by_desc); n != NULL; n = rb_next(n)) {
> + for (n = rb_first(&proc->refs_by_desc); n; n = rb_next(n)) {
> struct binder_ref *ref = rb_entry(n, struct binder_ref,
> rb_node_desc);
> count++;
> @@ -6756,29 +6782,15 @@ static int state_show(struct seq_file *m, void *unused)
> spin_lock(&binder_dead_nodes_lock);
> if (!hlist_empty(&binder_dead_nodes))
> seq_puts(m, "dead nodes:\n");
> - hlist_for_each_entry(node, &binder_dead_nodes, dead_node) {
> - /*
> - * take a temporary reference on the node so it
> - * survives and isn't removed from the list
> - * while we print it.
> - */
> - node->tmp_refs++;
> - spin_unlock(&binder_dead_nodes_lock);
> - if (last_node)
> - binder_put_node(last_node);
> - binder_node_lock(node);
> - print_binder_node_nilocked(m, node);
> - binder_node_unlock(node);
> - last_node = node;
> - spin_lock(&binder_dead_nodes_lock);
> - }
> + hlist_for_each_entry(node, &binder_dead_nodes, dead_node)
> + last_node = print_next_binder_node_ilocked(m, node, last_node);
> spin_unlock(&binder_dead_nodes_lock);
> if (last_node)
> binder_put_node(last_node);
>
> mutex_lock(&binder_procs_lock);
> hlist_for_each_entry(proc, &binder_procs, proc_node)
> - print_binder_proc(m, proc, 1);
> + print_binder_proc(m, proc, true);
> mutex_unlock(&binder_procs_lock);
>
> return 0;
> @@ -6807,7 +6819,7 @@ static int transactions_show(struct seq_file *m, void *unused)
> seq_puts(m, "binder transactions:\n");
> mutex_lock(&binder_procs_lock);
> hlist_for_each_entry(proc, &binder_procs, proc_node)
> - print_binder_proc(m, proc, 0);
> + print_binder_proc(m, proc, false);
> mutex_unlock(&binder_procs_lock);
>
> return 0;
> @@ -6822,7 +6834,7 @@ static int proc_show(struct seq_file *m, void *unused)
> hlist_for_each_entry(itr, &binder_procs, proc_node) {
> if (itr->pid == pid) {
> seq_puts(m, "binder proc state:\n");
> - print_binder_proc(m, itr, 1);
> + print_binder_proc(m, itr, true);
> }
> }
> mutex_unlock(&binder_procs_lock);
> --
> 2.49.0.395.g12beb8f557-goog
>
Other than the minor nit feel free to add:
Acked-by: Carlos Llamas <cmllamas@...gle.com>
--
Carlos Llamas
Powered by blists - more mailing lists