[PATCH v2 1/2] binder: Refactor binder_node print synchronization

[PATCH v2 1/2] binder: Refactor binder_node print synchronization

[Tiffany Yang]

From: "Tiffany Y. Yang" <ynaffit@google.com>

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.

Acked-by: Carlos Llamas <cmllamas@google.com>
Signed-off-by: Tiffany Y. Yang <ynaffit@google.com>
---
V2 -> V1: Updated function comment to note dropped locks

 drivers/android/binder.c | 115 ++++++++++++++++++++++-----------------
 1 file changed, 64 insertions(+), 51 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 5fc2c8ee61b1..960d055c27d5 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,51 @@ 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. This lock will be released during the body of this
+ * function, but it will be reacquired before returning to the caller.
+ *
+ * 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 +6562,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 +6580,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 +6723,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 +6737,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 +6745,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 +6783,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 +6820,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 +6835,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.987.g0cc8ee98dc-goog

[PATCH v2 2/2] binder: Create safe versions of binder log files

[Tiffany Yang]

From: "Tiffany Y. Yang" <ynaffit@google.com>

Binder defines several seq_files that can be accessed via debugfs or
binderfs. Some of these files (e.g., 'state' and 'transactions')
contain more granular information about binder's internal state that
is helpful for debugging, but they also leak userspace address data
through user-defined 'cookie' or 'ptr' values. Consequently, access
to these files must be heavily restricted.

Add two new files, 'state_hashed' and 'transactions_hashed', that
reproduce the information in the original files but use the kernel's
raw pointer obfuscation to hash any potential user addresses. This
approach allows systems to grant broader access to the new files
without having to change the security policy around the existing ones.

In practice, userspace populates these fields with user addresses, but
within the driver, these values only serve as unique identifiers for
their associated binder objects. Consequently, binder logs can
obfuscate these values and still retain meaning. While this strategy
prevents leaking information about the userspace memory layout in the
existing log files, it also decouples log messages about binder
objects from their user-defined identifiers.

Acked-by: Carlos Llamas <cmllamas@google.com>
Signed-off-by: Tiffany Y. Yang <ynaffit@google.com>
---
V2->V1: Update commit message

 drivers/android/binder.c | 103 +++++++++++++++++++++++++++++----------
 1 file changed, 77 insertions(+), 26 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 960d055c27d5..a85dc11ffdea 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6380,7 +6380,7 @@ 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_work *w, bool hash_ptrs)
 {
 	struct binder_node *node;
 	struct binder_transaction *t;
@@ -6403,9 +6403,14 @@ static void print_binder_work_ilocked(struct seq_file *m,
 		break;
 	case BINDER_WORK_NODE:
 		node = container_of(w, struct binder_node, work);
-		seq_printf(m, "%snode work %d: u%016llx c%016llx\n",
+		if (hash_ptrs)
+			seq_printf(m, "%snode work %d: u%p c%p\n",
 			   prefix, node->debug_id,
-			   (u64)node->ptr, (u64)node->cookie);
+			   (void *)node->ptr, (void *)node->cookie);
+		else
+			seq_printf(m, "%snode work %d: u%016llx c%016llx\n",
+				   prefix, node->debug_id,
+				   (u64)node->ptr, (u64)node->cookie);
 		break;
 	case BINDER_WORK_DEAD_BINDER:
 		seq_printf(m, "%shas dead binder\n", prefix);
@@ -6430,7 +6435,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,
-					bool print_always)
+					bool print_always, bool hash_ptrs)
 {
 	struct binder_transaction *t;
 	struct binder_work *w;
@@ -6460,14 +6465,16 @@ static void print_binder_thread_ilocked(struct seq_file *m,
 	}
 	list_for_each_entry(w, &thread->todo, entry) {
 		print_binder_work_ilocked(m, thread->proc, "    ",
-					  "    pending transaction", w);
+					  "    pending transaction",
+					  w, hash_ptrs);
 	}
 	if (!print_always && m->count == header_pos)
 		m->count = start_pos;
 }
 
 static void print_binder_node_nilocked(struct seq_file *m,
-				       struct binder_node *node)
+				       struct binder_node *node,
+				       bool hash_ptrs)
 {
 	struct binder_ref *ref;
 	struct binder_work *w;
@@ -6475,8 +6482,13 @@ static void print_binder_node_nilocked(struct seq_file *m,
 
 	count = hlist_count_nodes(&node->refs);
 
-	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,
+	if (hash_ptrs)
+		seq_printf(m, "  node %d: u%p c%p", node->debug_id,
+			   (void *)node->ptr, (void *)node->cookie);
+	else
+		seq_printf(m, "  node %d: u%016llx c%016llx", node->debug_id,
+			   (u64)node->ptr, (u64)node->cookie);
+	seq_printf(m, " hs %d hw %d ls %d lw %d is %d iw %d tr %d",
 		   node->has_strong_ref, node->has_weak_ref,
 		   node->local_strong_refs, node->local_weak_refs,
 		   node->internal_strong_refs, count, node->tmp_refs);
@@ -6489,7 +6501,8 @@ static void print_binder_node_nilocked(struct seq_file *m,
 	if (node->proc) {
 		list_for_each_entry(w, &node->async_todo, entry)
 			print_binder_work_ilocked(m, node->proc, "    ",
-					  "    pending async transaction", w);
+					  "    pending async transaction",
+					  w, hash_ptrs);
 	}
 }
 
@@ -6510,6 +6523,7 @@ static void print_binder_ref_olocked(struct seq_file *m,
  * @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)
+ * @hash_ptrs:  whether to hash @node's binder_uintptr_t fields
  *
  * Helper function to handle synchronization around printing a struct
  * binder_node while iterating through @node->proc->nodes or the dead nodes
@@ -6521,7 +6535,7 @@ static void print_binder_ref_olocked(struct seq_file *m,
  */
 static struct binder_node *
 print_next_binder_node_ilocked(struct seq_file *m, struct binder_node *node,
-			       struct binder_node *prev_node)
+			       struct binder_node *prev_node, bool hash_ptrs)
 {
 	/*
 	 * Take a temporary reference on the node so that isn't removed from
@@ -6539,7 +6553,7 @@ print_next_binder_node_ilocked(struct seq_file *m, struct binder_node *node,
 	if (prev_node)
 		binder_put_node(prev_node);
 	binder_node_inner_lock(node);
-	print_binder_node_nilocked(m, node);
+	print_binder_node_nilocked(m, node, hash_ptrs);
 	binder_node_inner_unlock(node);
 	if (node->proc)
 		binder_inner_proc_lock(node->proc);
@@ -6549,7 +6563,7 @@ print_next_binder_node_ilocked(struct seq_file *m, struct binder_node *node,
 }
 
 static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
-			      bool print_all)
+			      bool print_all, bool hash_ptrs)
 {
 	struct binder_work *w;
 	struct rb_node *n;
@@ -6564,7 +6578,7 @@ static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
 	binder_inner_proc_lock(proc);
 	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);
+						rb_node), print_all, hash_ptrs);
 
 	for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
 		struct binder_node *node = rb_entry(n, struct binder_node,
@@ -6572,7 +6586,8 @@ static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
 		if (!print_all && !node->has_async_transaction)
 			continue;
 
-		last_node = print_next_binder_node_ilocked(m, node, last_node);
+		last_node = print_next_binder_node_ilocked(m, node, last_node,
+							   hash_ptrs);
 	}
 	binder_inner_proc_unlock(proc);
 	if (last_node)
@@ -6590,7 +6605,8 @@ static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
 	binder_inner_proc_lock(proc);
 	list_for_each_entry(w, &proc->todo, entry)
 		print_binder_work_ilocked(m, proc, "  ",
-					  "  pending transaction", w);
+					  "  pending transaction", w,
+					  hash_ptrs);
 	list_for_each_entry(w, &proc->delivered_death, entry) {
 		seq_puts(m, "  has delivered dead binder\n");
 		break;
@@ -6772,7 +6788,7 @@ static void print_binder_proc_stats(struct seq_file *m,
 	print_binder_stats(m, "  ", &proc->stats);
 }
 
-static int state_show(struct seq_file *m, void *unused)
+static void print_binder_state(struct seq_file *m, bool hash_ptrs)
 {
 	struct binder_proc *proc;
 	struct binder_node *node;
@@ -6784,16 +6800,38 @@ static int state_show(struct seq_file *m, void *unused)
 	if (!hlist_empty(&binder_dead_nodes))
 		seq_puts(m, "dead nodes:\n");
 	hlist_for_each_entry(node, &binder_dead_nodes, dead_node)
-		last_node = print_next_binder_node_ilocked(m, node, last_node);
+		last_node = print_next_binder_node_ilocked(m, node, last_node,
+							   hash_ptrs);
 	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, true);
+		print_binder_proc(m, proc, true, hash_ptrs);
 	mutex_unlock(&binder_procs_lock);
+}
+
+static void print_binder_transactions(struct seq_file *m, bool hash_ptrs)
+{
+	struct binder_proc *proc;
+
+	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, false, hash_ptrs);
+	mutex_unlock(&binder_procs_lock);
+}
+
+static int state_show(struct seq_file *m, void *unused)
+{
+	print_binder_state(m, false);
+	return 0;
+}
 
+static int state_hashed_show(struct seq_file *m, void *unused)
+{
+	print_binder_state(m, true);
 	return 0;
 }
 
@@ -6815,14 +6853,13 @@ static int stats_show(struct seq_file *m, void *unused)
 
 static int transactions_show(struct seq_file *m, void *unused)
 {
-	struct binder_proc *proc;
-
-	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, false);
-	mutex_unlock(&binder_procs_lock);
+	print_binder_transactions(m, false);
+	return 0;
+}
 
+static int transactions_hashed_show(struct seq_file *m, void *unused)
+{
+	print_binder_transactions(m, true);
 	return 0;
 }
 
@@ -6835,7 +6872,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, true);
+			print_binder_proc(m, itr, true, false);
 		}
 	}
 	mutex_unlock(&binder_procs_lock);
@@ -6902,8 +6939,10 @@ const struct file_operations binder_fops = {
 };
 
 DEFINE_SHOW_ATTRIBUTE(state);
+DEFINE_SHOW_ATTRIBUTE(state_hashed);
 DEFINE_SHOW_ATTRIBUTE(stats);
 DEFINE_SHOW_ATTRIBUTE(transactions);
+DEFINE_SHOW_ATTRIBUTE(transactions_hashed);
 DEFINE_SHOW_ATTRIBUTE(transaction_log);
 
 const struct binder_debugfs_entry binder_debugfs_entries[] = {
@@ -6913,6 +6952,12 @@ const struct binder_debugfs_entry binder_debugfs_entries[] = {
 		.fops = &state_fops,
 		.data = NULL,
 	},
+	{
+		.name = "state_hashed",
+		.mode = 0444,
+		.fops = &state_hashed_fops,
+		.data = NULL,
+	},
 	{
 		.name = "stats",
 		.mode = 0444,
@@ -6925,6 +6970,12 @@ const struct binder_debugfs_entry binder_debugfs_entries[] = {
 		.fops = &transactions_fops,
 		.data = NULL,
 	},
+	{
+		.name = "transactions_hashed",
+		.mode = 0444,
+		.fops = &transactions_hashed_fops,
+		.data = NULL,
+	},
 	{
 		.name = "transaction_log",
 		.mode = 0444,
-- 
2.49.0.987.g0cc8ee98dc-goog

Re: [PATCH v2 2/2] binder: Create safe versions of binder log files

[kernel test robot]

Hi Tiffany,

kernel test robot noticed the following build warnings:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on staging/staging-next staging/staging-linus linus/master v6.15-rc5 next-20250506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tiffany-Yang/binder-Create-safe-versions-of-binder-log-files/20250506-054556
base:   staging/staging-testing
patch link:    https://lore.kernel.org/r/20250505214306.3843294-4-ynaffit%40google.com
patch subject: [PATCH v2 2/2] binder: Create safe versions of binder log files
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20250507/202505072254.2gHv8kgu-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505072254.2gHv8kgu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505072254.2gHv8kgu-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/android/binder.c: In function 'print_binder_work_ilocked':
>> drivers/android/binder.c:6409:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    6409 |                            (void *)node->ptr, (void *)node->cookie);
         |                            ^
   drivers/android/binder.c:6409:47: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    6409 |                            (void *)node->ptr, (void *)node->cookie);
         |                                               ^
   drivers/android/binder.c: In function 'print_binder_node_nilocked':
   drivers/android/binder.c:6487:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    6487 |                            (void *)node->ptr, (void *)node->cookie);
         |                            ^
   drivers/android/binder.c:6487:47: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    6487 |                            (void *)node->ptr, (void *)node->cookie);
         |                                               ^


vim +6409 drivers/android/binder.c

  6378	
  6379	static void print_binder_work_ilocked(struct seq_file *m,
  6380					      struct binder_proc *proc,
  6381					      const char *prefix,
  6382					      const char *transaction_prefix,
  6383					      struct binder_work *w, bool hash_ptrs)
  6384	{
  6385		struct binder_node *node;
  6386		struct binder_transaction *t;
  6387	
  6388		switch (w->type) {
  6389		case BINDER_WORK_TRANSACTION:
  6390			t = container_of(w, struct binder_transaction, work);
  6391			print_binder_transaction_ilocked(
  6392					m, proc, transaction_prefix, t);
  6393			break;
  6394		case BINDER_WORK_RETURN_ERROR: {
  6395			struct binder_error *e = container_of(
  6396					w, struct binder_error, work);
  6397	
  6398			seq_printf(m, "%stransaction error: %u\n",
  6399				   prefix, e->cmd);
  6400		} break;
  6401		case BINDER_WORK_TRANSACTION_COMPLETE:
  6402			seq_printf(m, "%stransaction complete\n", prefix);
  6403			break;
  6404		case BINDER_WORK_NODE:
  6405			node = container_of(w, struct binder_node, work);
  6406			if (hash_ptrs)
  6407				seq_printf(m, "%snode work %d: u%p c%p\n",
  6408				   prefix, node->debug_id,
> 6409				   (void *)node->ptr, (void *)node->cookie);
  6410			else
  6411				seq_printf(m, "%snode work %d: u%016llx c%016llx\n",
  6412					   prefix, node->debug_id,
  6413					   (u64)node->ptr, (u64)node->cookie);
  6414			break;
  6415		case BINDER_WORK_DEAD_BINDER:
  6416			seq_printf(m, "%shas dead binder\n", prefix);
  6417			break;
  6418		case BINDER_WORK_DEAD_BINDER_AND_CLEAR:
  6419			seq_printf(m, "%shas cleared dead binder\n", prefix);
  6420			break;
  6421		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
  6422			seq_printf(m, "%shas cleared death notification\n", prefix);
  6423			break;
  6424		case BINDER_WORK_FROZEN_BINDER:
  6425			seq_printf(m, "%shas frozen binder\n", prefix);
  6426			break;
  6427		case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION:
  6428			seq_printf(m, "%shas cleared freeze notification\n", prefix);
  6429			break;
  6430		default:
  6431			seq_printf(m, "%sunknown work: type %d\n", prefix, w->type);
  6432			break;
  6433		}
  6434	}
  6435	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] binder: Create safe versions of binder log files

[Carlos Llamas]

On Wed, May 07, 2025 at 08:19:07PM +0800, kernel test robot wrote:
> Hi Tiffany,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on staging/staging-testing]
> [also build test WARNING on staging/staging-next staging/staging-linus linus/master v6.15-rc5 next-20250506]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Tiffany-Yang/binder-Create-safe-versions-of-binder-log-files/20250506-054556
> base:   staging/staging-testing
> patch link:    https://lore.kernel.org/r/20250505214306.3843294-4-ynaffit%40google.com
> patch subject: [PATCH v2 2/2] binder: Create safe versions of binder log files
> config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20250507/202505072254.2gHv8kgu-lkp@intel.com/config)
> compiler: or1k-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505072254.2gHv8kgu-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202505072254.2gHv8kgu-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/android/binder.c: In function 'print_binder_work_ilocked':
> >> drivers/android/binder.c:6409:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>     6409 |                            (void *)node->ptr, (void *)node->cookie);
>          |                            ^
>    drivers/android/binder.c:6409:47: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>     6409 |                            (void *)node->ptr, (void *)node->cookie);
>          |                                               ^
>    drivers/android/binder.c: In function 'print_binder_node_nilocked':
>    drivers/android/binder.c:6487:28: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>     6487 |                            (void *)node->ptr, (void *)node->cookie);
>          |                            ^
>    drivers/android/binder.c:6487:47: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>     6487 |                            (void *)node->ptr, (void *)node->cookie);
>          |                                               ^

Oh right, this is silly. The reason is us _not_ using fixed-sized in our
uapi, which was unfortunate. In Android we don't have 32bit builds that
typedef 'binder_uinptr_t' to __u64 as we always pass -DBINDER_IPC_32BIT
for such cases. It is not the case outside of Android though, so this
needs a double explicit cast. :(

	#ifdef BINDER_IPC_32BIT
	typedef __u32 binder_size_t;
	typedef __u32 binder_uintptr_t;
	#else
	typedef __u64 binder_size_t;
	typedef __u64 binder_uintptr_t;
	#endif

Re: [PATCH v2 1/2] binder: Refactor binder_node print synchronization

[Alice Ryhl]

On Mon, May 05, 2025 at 09:42:32PM +0000, Tiffany Yang wrote:
> +	if (node->proc)
> +		binder_inner_proc_unlock(node->proc);
> +	else
> +		spin_unlock(&binder_dead_nodes_lock);

I don't buy this logic. Imagine the following scenario:

1. print_binder_proc is called, and we loop over proc->nodes.
2. We call binder_inner_proc_unlock(node->proc).
3. On another thread, binder_deferred_release() is called.
4. The node is removed from proc->nodes and node->proc is set to NULL.
5. Back in print_next_binder_node_ilocked(), we now call
   spin_lock(&binder_dead_nodes_lock) and return.
6. In print_binder_proc(), we think that we hold the proc lock, but
   actually we hold the dead nodes lock instead. BOOM.

What happens with the current code is that print_binder_proc() takes the
proc lock again after the node was removed from proc->nodes, and then it
exits the loop because rb_next(n) returns NULL when called on a node not
in any rb-tree.

Alice

Re: [PATCH v2 1/2] binder: Refactor binder_node print synchronization

[Alice Ryhl]

On Thu, May 8, 2025 at 2:18 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Mon, May 05, 2025 at 09:42:32PM +0000, Tiffany Yang wrote:
> > +     if (node->proc)
> > +             binder_inner_proc_unlock(node->proc);
> > +     else
> > +             spin_unlock(&binder_dead_nodes_lock);
>
> I don't buy this logic. Imagine the following scenario:
>
> 1. print_binder_proc is called, and we loop over proc->nodes.
> 2. We call binder_inner_proc_unlock(node->proc).
> 3. On another thread, binder_deferred_release() is called.
> 4. The node is removed from proc->nodes and node->proc is set to NULL.
> 5. Back in print_next_binder_node_ilocked(), we now call
>    spin_lock(&binder_dead_nodes_lock) and return.
> 6. In print_binder_proc(), we think that we hold the proc lock, but
>    actually we hold the dead nodes lock instead. BOOM.
>
> What happens with the current code is that print_binder_proc() takes the
> proc lock again after the node was removed from proc->nodes, and then it
> exits the loop because rb_next(n) returns NULL when called on a node not
> in any rb-tree.

Oh, there's a v3 of this. Let me resend it there.

Alice