[PATCH] binder: fix redefinition of seq_file attributes

[PATCH] binder: fix redefinition of seq_file attributes

[Carlos Llamas]

The patchset in [1] exported some definitions to binder_internal.h in
order to make the debugfs entries such as 'stats' and 'transaction_log'
available in a binderfs instance. However, the DEFINE_SHOW_ATTRIBUTE
macro expands into a static function/variable pair, which in turn get
redefined each time a source file includes this internal header.

This problem was made evident after a report from the kernel test robot
<lkp@intel.com> where several W=1 build warnings are seen in downstream
kernels. See the following example:

  include/../drivers/android/binder_internal.h:111:23: warning: 'binder_stats_fops' defined but not used [-Wunused-const-variable=]
     111 | DEFINE_SHOW_ATTRIBUTE(binder_stats);
         |                       ^~~~~~~~~~~~
  include/linux/seq_file.h:174:37: note: in definition of macro 'DEFINE_SHOW_ATTRIBUTE'
     174 | static const struct file_operations __name ## _fops = {                 \
         |                                     ^~~~~~

This patch fixes the above issues by moving back the definitions into
binder.c and instead creates an array of the debugfs entries which is
more convenient to share with binderfs and iterate through.

  [1] https://lore.kernel.org/all/20190903161655.107408-1-hridya@google.com/

Fixes: 0e13e452dafc ("binder: Add stats, state and transactions files")
Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c          | 114 +++++++++++++++++++++---------
 drivers/android/binder_internal.h |  46 +++---------
 drivers/android/binderfs.c        |  47 +++---------
 3 files changed, 100 insertions(+), 107 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 362c0deb65f1..54ac94fed015 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -197,8 +197,32 @@ static inline void binder_stats_created(enum binder_stat_types type)
 	atomic_inc(&binder_stats.obj_created[type]);
 }
 
-struct binder_transaction_log binder_transaction_log;
-struct binder_transaction_log binder_transaction_log_failed;
+struct binder_transaction_log_entry {
+	int debug_id;
+	int debug_id_done;
+	int call_type;
+	int from_proc;
+	int from_thread;
+	int target_handle;
+	int to_proc;
+	int to_thread;
+	int to_node;
+	int data_size;
+	int offsets_size;
+	int return_error_line;
+	uint32_t return_error;
+	uint32_t return_error_param;
+	char context_name[BINDERFS_MAX_NAME + 1];
+};
+
+struct binder_transaction_log {
+	atomic_t cur;
+	bool full;
+	struct binder_transaction_log_entry entry[32];
+};
+
+static struct binder_transaction_log binder_transaction_log;
+static struct binder_transaction_log binder_transaction_log_failed;
 
 static struct binder_transaction_log_entry *binder_transaction_log_add(
 	struct binder_transaction_log *log)
@@ -6197,8 +6221,7 @@ static void print_binder_proc_stats(struct seq_file *m,
 	print_binder_stats(m, "  ", &proc->stats);
 }
 
-
-int binder_state_show(struct seq_file *m, void *unused)
+static int state_show(struct seq_file *m, void *unused)
 {
 	struct binder_proc *proc;
 	struct binder_node *node;
@@ -6237,7 +6260,7 @@ int binder_state_show(struct seq_file *m, void *unused)
 	return 0;
 }
 
-int binder_stats_show(struct seq_file *m, void *unused)
+static int stats_show(struct seq_file *m, void *unused)
 {
 	struct binder_proc *proc;
 
@@ -6253,7 +6276,7 @@ int binder_stats_show(struct seq_file *m, void *unused)
 	return 0;
 }
 
-int binder_transactions_show(struct seq_file *m, void *unused)
+static int transactions_show(struct seq_file *m, void *unused)
 {
 	struct binder_proc *proc;
 
@@ -6309,7 +6332,7 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
 			"\n" : " (incomplete)\n");
 }
 
-int binder_transaction_log_show(struct seq_file *m, void *unused)
+static int transaction_log_show(struct seq_file *m, void *unused)
 {
 	struct binder_transaction_log *log = m->private;
 	unsigned int log_cur = atomic_read(&log->cur);
@@ -6341,6 +6364,45 @@ const struct file_operations binder_fops = {
 	.release = binder_release,
 };
 
+DEFINE_SHOW_ATTRIBUTE(state);
+DEFINE_SHOW_ATTRIBUTE(stats);
+DEFINE_SHOW_ATTRIBUTE(transactions);
+DEFINE_SHOW_ATTRIBUTE(transaction_log);
+
+const struct binder_debugfs_entry binder_debugfs_entries[] = {
+	{
+		.name = "state",
+		.mode = 0444,
+		.fops = &state_fops,
+		.data = NULL,
+	},
+	{
+		.name = "stats",
+		.mode = 0444,
+		.fops = &stats_fops,
+		.data = NULL,
+	},
+	{
+		.name = "transactions",
+		.mode = 0444,
+		.fops = &transactions_fops,
+		.data = NULL,
+	},
+	{
+		.name = "transaction_log",
+		.mode = 0444,
+		.fops = &transaction_log_fops,
+		.data = &binder_transaction_log,
+	},
+	{
+		.name = "failed_transaction_log",
+		.mode = 0444,
+		.fops = &transaction_log_fops,
+		.data = &binder_transaction_log_failed,
+	},
+	{} /* terminator */
+};
+
 static int __init init_binder_device(const char *name)
 {
 	int ret;
@@ -6386,36 +6448,18 @@ static int __init binder_init(void)
 	atomic_set(&binder_transaction_log_failed.cur, ~0U);
 
 	binder_debugfs_dir_entry_root = debugfs_create_dir("binder", NULL);
-	if (binder_debugfs_dir_entry_root)
+	if (binder_debugfs_dir_entry_root) {
+		const struct binder_debugfs_entry *db_entry;
+
+		binder_for_each_debugfs_entry(db_entry)
+			debugfs_create_file(db_entry->name,
+					    db_entry->mode,
+					    binder_debugfs_dir_entry_root,
+					    db_entry->data,
+					    db_entry->fops);
+
 		binder_debugfs_dir_entry_proc = debugfs_create_dir("proc",
 						 binder_debugfs_dir_entry_root);
-
-	if (binder_debugfs_dir_entry_root) {
-		debugfs_create_file("state",
-				    0444,
-				    binder_debugfs_dir_entry_root,
-				    NULL,
-				    &binder_state_fops);
-		debugfs_create_file("stats",
-				    0444,
-				    binder_debugfs_dir_entry_root,
-				    NULL,
-				    &binder_stats_fops);
-		debugfs_create_file("transactions",
-				    0444,
-				    binder_debugfs_dir_entry_root,
-				    NULL,
-				    &binder_transactions_fops);
-		debugfs_create_file("transaction_log",
-				    0444,
-				    binder_debugfs_dir_entry_root,
-				    &binder_transaction_log,
-				    &binder_transaction_log_fops);
-		debugfs_create_file("failed_transaction_log",
-				    0444,
-				    binder_debugfs_dir_entry_root,
-				    &binder_transaction_log_failed,
-				    &binder_transaction_log_fops);
 	}
 
 	if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 8dc0bccf8513..abe19d88c6ec 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -107,41 +107,19 @@ static inline int __init init_binderfs(void)
 }
 #endif
 
-int binder_stats_show(struct seq_file *m, void *unused);
-DEFINE_SHOW_ATTRIBUTE(binder_stats);
-
-int binder_state_show(struct seq_file *m, void *unused);
-DEFINE_SHOW_ATTRIBUTE(binder_state);
-
-int binder_transactions_show(struct seq_file *m, void *unused);
-DEFINE_SHOW_ATTRIBUTE(binder_transactions);
-
-int binder_transaction_log_show(struct seq_file *m, void *unused);
-DEFINE_SHOW_ATTRIBUTE(binder_transaction_log);
-
-struct binder_transaction_log_entry {
-	int debug_id;
-	int debug_id_done;
-	int call_type;
-	int from_proc;
-	int from_thread;
-	int target_handle;
-	int to_proc;
-	int to_thread;
-	int to_node;
-	int data_size;
-	int offsets_size;
-	int return_error_line;
-	uint32_t return_error;
-	uint32_t return_error_param;
-	char context_name[BINDERFS_MAX_NAME + 1];
+struct binder_debugfs_entry {
+	const char *name;
+	umode_t mode;
+	const struct file_operations *fops;
+	void *data;
 };
 
-struct binder_transaction_log {
-	atomic_t cur;
-	bool full;
-	struct binder_transaction_log_entry entry[32];
-};
+extern const struct binder_debugfs_entry binder_debugfs_entries[];
+
+#define binder_for_each_debugfs_entry(entry)	\
+	for ((entry) = binder_debugfs_entries;	\
+	     (entry)->name;			\
+	     (entry)++)
 
 enum binder_stat_types {
 	BINDER_STAT_PROC,
@@ -580,6 +558,4 @@ struct binder_object {
 	};
 };
 
-extern struct binder_transaction_log binder_transaction_log;
-extern struct binder_transaction_log binder_transaction_log_failed;
 #endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 6c5e94f6cb3a..588d753a7a19 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -629,6 +629,7 @@ static int init_binder_features(struct super_block *sb)
 static int init_binder_logs(struct super_block *sb)
 {
 	struct dentry *binder_logs_root_dir, *dentry, *proc_log_dir;
+	const struct binder_debugfs_entry *db_entry;
 	struct binderfs_info *info;
 	int ret = 0;
 
@@ -639,43 +640,15 @@ static int init_binder_logs(struct super_block *sb)
 		goto out;
 	}
 
-	dentry = binderfs_create_file(binder_logs_root_dir, "stats",
-				      &binder_stats_fops, NULL);
-	if (IS_ERR(dentry)) {
-		ret = PTR_ERR(dentry);
-		goto out;
-	}
-
-	dentry = binderfs_create_file(binder_logs_root_dir, "state",
-				      &binder_state_fops, NULL);
-	if (IS_ERR(dentry)) {
-		ret = PTR_ERR(dentry);
-		goto out;
-	}
-
-	dentry = binderfs_create_file(binder_logs_root_dir, "transactions",
-				      &binder_transactions_fops, NULL);
-	if (IS_ERR(dentry)) {
-		ret = PTR_ERR(dentry);
-		goto out;
-	}
-
-	dentry = binderfs_create_file(binder_logs_root_dir,
-				      "transaction_log",
-				      &binder_transaction_log_fops,
-				      &binder_transaction_log);
-	if (IS_ERR(dentry)) {
-		ret = PTR_ERR(dentry);
-		goto out;
-	}
-
-	dentry = binderfs_create_file(binder_logs_root_dir,
-				      "failed_transaction_log",
-				      &binder_transaction_log_fops,
-				      &binder_transaction_log_failed);
-	if (IS_ERR(dentry)) {
-		ret = PTR_ERR(dentry);
-		goto out;
+	binder_for_each_debugfs_entry(db_entry) {
+		dentry = binderfs_create_file(binder_logs_root_dir,
+					      db_entry->name,
+					      db_entry->fops,
+					      db_entry->data);
+		if (IS_ERR(dentry)) {
+			ret = PTR_ERR(dentry);
+			goto out;
+		}
 	}
 
 	proc_log_dir = binderfs_create_dir(binder_logs_root_dir, "proc");
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH] binder: fix redefinition of seq_file attributes

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 06:20:41PM +0000, Carlos Llamas wrote:
> +	binder_for_each_debugfs_entry(db_entry) {
> +		dentry = binderfs_create_file(binder_logs_root_dir,
> +					      db_entry->name,
> +					      db_entry->fops,
> +					      db_entry->data);
> +		if (IS_ERR(dentry)) {
> +			ret = PTR_ERR(dentry);
> +			goto out;
> +		}

I know this is a copy of what is there already, but there is never a
need to check the result of a debugfs_create_* call.  Just call it and
move on, never "abort" based on the result of a debugfs call, that's not
a good idea.

So can you change this here, or want to send a follow-on patch that
removes these checks?

>  	}
>  
>  	proc_log_dir = binderfs_create_dir(binder_logs_root_dir, "proc");

Also there's never a need to save a directory, you can always look it up
when you want to remove it.

thanks,

greg k-h

Re: [PATCH] binder: fix redefinition of seq_file attributes

[Carlos Llamas]

On Mon, Jul 04, 2022 at 06:13:40PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 01, 2022 at 06:20:41PM +0000, Carlos Llamas wrote:
> > +	binder_for_each_debugfs_entry(db_entry) {
> > +		dentry = binderfs_create_file(binder_logs_root_dir,
> > +					      db_entry->name,
> > +					      db_entry->fops,
> > +					      db_entry->data);
> > +		if (IS_ERR(dentry)) {
> > +			ret = PTR_ERR(dentry);
> > +			goto out;
> > +		}
> 
> I know this is a copy of what is there already, but there is never a
> need to check the result of a debugfs_create_* call.  Just call it and
> move on, never "abort" based on the result of a debugfs call, that's not
> a good idea.

This is true, none of these debugfs files seem critical for mounting a
binderfs instance. I'm thinking init_binder_logs() should just return
void. I'm only a bit hesitant to completely ignore the return code as
users specifically ask for these files to be created via mount option
"stats". So probably a pr_warn is what is actually needed here.

> 
> So can you change this here, or want to send a follow-on patch that
> removes these checks?

Sure, I'll send a follow-on patch. I'm currently AFK so setting ETA for
next week until I can actually test this change.

> 
> >  	}
> >  
> >  	proc_log_dir = binderfs_create_dir(binder_logs_root_dir, "proc");
> 
> Also there's never a need to save a directory, you can always look it up
> when you want to remove it.

It seems this is a convenient way to share this path with binder which
otherwise doesn't know where binderfs was mounted. From having a quick
look it doesn't seem that we need to share all the details in struct
binderfs_info though. Maybe there is a better way to handle all this.

Christian, since this is binderfs area WDYT?

--
Carlos Llamas

Re: [PATCH] binder: fix redefinition of seq_file attributes

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 09:21:52PM +0000, Carlos Llamas wrote:
> On Mon, Jul 04, 2022 at 06:13:40PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Jul 01, 2022 at 06:20:41PM +0000, Carlos Llamas wrote:
> > > +	binder_for_each_debugfs_entry(db_entry) {
> > > +		dentry = binderfs_create_file(binder_logs_root_dir,
> > > +					      db_entry->name,
> > > +					      db_entry->fops,
> > > +					      db_entry->data);
> > > +		if (IS_ERR(dentry)) {
> > > +			ret = PTR_ERR(dentry);
> > > +			goto out;
> > > +		}
> > 
> > I know this is a copy of what is there already, but there is never a
> > need to check the result of a debugfs_create_* call.  Just call it and
> > move on, never "abort" based on the result of a debugfs call, that's not
> > a good idea.
> 
> This is true, none of these debugfs files seem critical for mounting a
> binderfs instance. I'm thinking init_binder_logs() should just return
> void. I'm only a bit hesitant to completely ignore the return code as
> users specifically ask for these files to be created via mount option
> "stats". So probably a pr_warn is what is actually needed here.

That would just be too noisy, just let it go, no one cares :)

> > So can you change this here, or want to send a follow-on patch that
> > removes these checks?
> 
> Sure, I'll send a follow-on patch. I'm currently AFK so setting ETA for
> next week until I can actually test this change.
> 
> > 
> > >  	}
> > >  
> > >  	proc_log_dir = binderfs_create_dir(binder_logs_root_dir, "proc");
> > 
> > Also there's never a need to save a directory, you can always look it up
> > when you want to remove it.
> 
> It seems this is a convenient way to share this path with binder which
> otherwise doesn't know where binderfs was mounted. From having a quick
> look it doesn't seem that we need to share all the details in struct
> binderfs_info though. Maybe there is a better way to handle all this.

Why would you need to share this internally with anything, again, it can
always be looked up if you need it.

thanks,

greg k-h

Re: [PATCH] binder: fix redefinition of seq_file attributes

[Carlos Llamas]

On Fri, Jul 08, 2022 at 08:00:50AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 07, 2022 at 09:21:52PM +0000, Carlos Llamas wrote:
> > On Mon, Jul 04, 2022 at 06:13:40PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jul 01, 2022 at 06:20:41PM +0000, Carlos Llamas wrote:
> > > > +	binder_for_each_debugfs_entry(db_entry) {
> > > > +		dentry = binderfs_create_file(binder_logs_root_dir,
> > > > +					      db_entry->name,
> > > > +					      db_entry->fops,
> > > > +					      db_entry->data);
> > > > +		if (IS_ERR(dentry)) {
> > > > +			ret = PTR_ERR(dentry);
> > > > +			goto out;
> > > > +		}
> > > 
> > > I know this is a copy of what is there already, but there is never a
> > > need to check the result of a debugfs_create_* call.  Just call it and
> > > move on, never "abort" based on the result of a debugfs call, that's not
> > > a good idea.
> > 
> > This is true, none of these debugfs files seem critical for mounting a
> > binderfs instance. I'm thinking init_binder_logs() should just return
> > void. I'm only a bit hesitant to completely ignore the return code as
> > users specifically ask for these files to be created via mount option
> > "stats". So probably a pr_warn is what is actually needed here.
> 
> That would just be too noisy, just let it go, no one cares :)

ok, convinced. I'll get rid of these checks.

> 
> > > So can you change this here, or want to send a follow-on patch that
> > > removes these checks?
> > 
> > Sure, I'll send a follow-on patch. I'm currently AFK so setting ETA for
> > next week until I can actually test this change.
> > 
> > > 
> > > >  	}
> > > >  
> > > >  	proc_log_dir = binderfs_create_dir(binder_logs_root_dir, "proc");
> > > 
> > > Also there's never a need to save a directory, you can always look it up
> > > when you want to remove it.
> > 
> > It seems this is a convenient way to share this path with binder which
> > otherwise doesn't know where binderfs was mounted. From having a quick
> > look it doesn't seem that we need to share all the details in struct
> > binderfs_info though. Maybe there is a better way to handle all this.
> 
> Why would you need to share this internally with anything, again, it can
> always be looked up if you need it.

I just looked into this and you are right. Binder can just take
sb->s_root and look up the entries as needed. This means we can also
unexport all the binderfs_info bits from the internal header. Great!

Thanks,
--
Carlos Llamas