[PATCH] android: binder: print error message on failure of creating proc file

[PATCH] android: binder: print error message on failure of creating proc file

[Leesoo Ahn]

It better prints out an error message to give more information if
calling debugfs_create_file() is failure and the return value has an
error code.

Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
---
 drivers/android/binder.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b21a7b246a0d..eb0fd1443d69 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5673,6 +5673,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
 
 	if (binder_debugfs_dir_entry_proc && !existing_pid) {
 		char strbuf[11];
+		struct dentry *debugfs_entry;
 
 		snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
 		/*
@@ -5681,10 +5682,19 @@ static int binder_open(struct inode *nodp, struct file *filp)
 		 * The printing code will anyway print all contexts for a given
 		 * PID so this is not a problem.
 		 */
-		proc->debugfs_entry = debugfs_create_file(strbuf, 0444,
+		debugfs_entry = debugfs_create_file(strbuf, 0444,
 			binder_debugfs_dir_entry_proc,
 			(void *)(unsigned long)proc->pid,
 			&proc_fops);
+		if (!IS_ERR(debugfs_entry)) {
+			proc->debugfs_entry = debugfs_entry;
+		} else {
+			int error;
+
+			error = PTR_ERR(debugfs_entry);
+			pr_warn("Unable to create file %s in debugfs (error %d)\n",
+				strbuf, error);
+		}
 	}
 
 	if (binder_binderfs_dir_entry_proc && !existing_pid) {
-- 
2.34.1

Re: [PATCH] android: binder: print error message on failure of creating proc file

[Carlos Llamas]

On Fri, Jul 12, 2024 at 12:21:40PM +0900, Leesoo Ahn wrote:
> It better prints out an error message to give more information if
> calling debugfs_create_file() is failure and the return value has an
> error code.
> 
> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> ---

What are you trying to fix? My understanding is that users of the
debugfs API can safely ignore any errors and move on. IMO it doesn't
make sense to add this without a real reason.

--
Carlos Llamas

Re: [PATCH] android: binder: print error message on failure of creating proc file

[Greg Kroah-Hartman]

On Fri, Jul 12, 2024 at 04:01:20AM +0000, Carlos Llamas wrote:
> On Fri, Jul 12, 2024 at 12:21:40PM +0900, Leesoo Ahn wrote:
> > It better prints out an error message to give more information if
> > calling debugfs_create_file() is failure and the return value has an
> > error code.
> > 
> > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> > ---
> 
> What are you trying to fix? My understanding is that users of the
> debugfs API can safely ignore any errors and move on. IMO it doesn't
> make sense to add this without a real reason.

Agreed, no one should care if debugfs works or not.

thanks,

greg k-h

Re: [PATCH] android: binder: print error message on failure of creating proc file

[Greg Kroah-Hartman]

On Fri, Jul 12, 2024 at 12:21:40PM +0900, Leesoo Ahn wrote:
> It better prints out an error message to give more information if
> calling debugfs_create_file() is failure and the return value has an
> error code.
> 
> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> ---
>  drivers/android/binder.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index b21a7b246a0d..eb0fd1443d69 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5673,6 +5673,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
>  
>  	if (binder_debugfs_dir_entry_proc && !existing_pid) {
>  		char strbuf[11];
> +		struct dentry *debugfs_entry;
>  
>  		snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
>  		/*
> @@ -5681,10 +5682,19 @@ static int binder_open(struct inode *nodp, struct file *filp)
>  		 * The printing code will anyway print all contexts for a given
>  		 * PID so this is not a problem.
>  		 */
> -		proc->debugfs_entry = debugfs_create_file(strbuf, 0444,
> +		debugfs_entry = debugfs_create_file(strbuf, 0444,
>  			binder_debugfs_dir_entry_proc,
>  			(void *)(unsigned long)proc->pid,
>  			&proc_fops);
> +		if (!IS_ERR(debugfs_entry)) {
> +			proc->debugfs_entry = debugfs_entry;
> +		} else {
> +			int error;
> +
> +			error = PTR_ERR(debugfs_entry);
> +			pr_warn("Unable to create file %s in debugfs (error %d)\n",
> +				strbuf, error);

Even if we wanted to warn about this (hint, you don't, see previous
response), this way to check is incorrect and will fail if debugfs is
not enabled, which you don't want to have happen.

So I'm guessing you did not test this with that config option disabled?

thanks,

greg k-h

Re: [PATCH] android: binder: print error message on failure of creating proc file

[Leesoo Ahn]

2024년 7월 12일 (금) 오후 2:40, Greg Kroah-Hartman <gregkh@linuxfoundation.org>님이 작성:
>
> On Fri, Jul 12, 2024 at 12:21:40PM +0900, Leesoo Ahn wrote:
> > It better prints out an error message to give more information if
> > calling debugfs_create_file() is failure and the return value has an
> > error code.
> >
> > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> > ---
> >  drivers/android/binder.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index b21a7b246a0d..eb0fd1443d69 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -5673,6 +5673,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
> >
> >       if (binder_debugfs_dir_entry_proc && !existing_pid) {
> >               char strbuf[11];
> > +             struct dentry *debugfs_entry;
> >
> >               snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
> >               /*
> > @@ -5681,10 +5682,19 @@ static int binder_open(struct inode *nodp, struct file *filp)
> >                * The printing code will anyway print all contexts for a given
> >                * PID so this is not a problem.
> >                */
> > -             proc->debugfs_entry = debugfs_create_file(strbuf, 0444,
> > +             debugfs_entry = debugfs_create_file(strbuf, 0444,
> >                       binder_debugfs_dir_entry_proc,
> >                       (void *)(unsigned long)proc->pid,
> >                       &proc_fops);
> > +             if (!IS_ERR(debugfs_entry)) {
> > +                     proc->debugfs_entry = debugfs_entry;
> > +             } else {
> > +                     int error;
> > +
> > +                     error = PTR_ERR(debugfs_entry);
> > +                     pr_warn("Unable to create file %s in debugfs (error %d)\n",
> > +                             strbuf, error);
>
> Even if we wanted to warn about this (hint, you don't, see previous
> response), this way to check is incorrect and will fail if debugfs is
> not enabled, which you don't want to have happen.
>
> So I'm guessing you did not test this with that config option disabled?

Oh, I haven't thought about this and just figured out it would work weird.
Thank you for mentioning it.

>
> thanks,
>
> greg k-h

Re: [PATCH] android: binder: print error message on failure of creating proc file

[Leesoo Ahn]

2024년 7월 12일 (금) 오후 1:01, Carlos Llamas <cmllamas@google.com>님이 작성:
>
> On Fri, Jul 12, 2024 at 12:21:40PM +0900, Leesoo Ahn wrote:
> > It better prints out an error message to give more information if
> > calling debugfs_create_file() is failure and the return value has an
> > error code.
> >
> > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> > ---
>
> What are you trying to fix? My understanding is that users of the
> debugfs API can safely ignore any errors and move on. IMO it doesn't
> make sense to add this without a real reason.

What I was trying to say, users would predict that a file under
debugfs will be created while they are opening a binder device. But if
it failed for some reason without any debug message, they would get
confused that the file doesn't exist and have no clue what happened
without a message.

BR,
Leesoo

(send this again by missing CCs)

Re: [PATCH] android: binder: print error message on failure of creating proc file

[Greg Kroah-Hartman]

On Fri, Jul 12, 2024 at 03:52:32PM +0900, Leesoo Ahn wrote:
> 2024년 7월 12일 (금) 오후 1:01, Carlos Llamas <cmllamas@google.com>님이 작성:
> >
> > On Fri, Jul 12, 2024 at 12:21:40PM +0900, Leesoo Ahn wrote:
> > > It better prints out an error message to give more information if
> > > calling debugfs_create_file() is failure and the return value has an
> > > error code.
> > >
> > > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> > > ---
> >
> > What are you trying to fix? My understanding is that users of the
> > debugfs API can safely ignore any errors and move on. IMO it doesn't
> > make sense to add this without a real reason.
> 
> What I was trying to say, users would predict that a file under
> debugfs will be created while they are opening a binder device. But if
> it failed for some reason without any debug message, they would get
> confused that the file doesn't exist and have no clue what happened
> without a message.

And that's fine, again, the kernel does not care if debugfs is working
or not.  It's just a debugging help, it does not affect the normal
operation of a system at all, and as such, userspace can't rely on it
being present for any functionality other than debugging issues that
might happen at times.

thanks,

greg k-h

Re: [PATCH] android: binder: print error message on failure of creating proc file

[Leesoo Ahn]

2024년 7월 12일 (금) 오후 4:19, Greg Kroah-Hartman <gregkh@linuxfoundation.org>님이 작성:
>
> On Fri, Jul 12, 2024 at 03:52:32PM +0900, Leesoo Ahn wrote:
> > 2024년 7월 12일 (금) 오후 1:01, Carlos Llamas <cmllamas@google.com>님이 작성:
> > >
> > > On Fri, Jul 12, 2024 at 12:21:40PM +0900, Leesoo Ahn wrote:
> > > > It better prints out an error message to give more information if
> > > > calling debugfs_create_file() is failure and the return value has an
> > > > error code.
> > > >
> > > > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
> > > > ---
> > >
> > > What are you trying to fix? My understanding is that users of the
> > > debugfs API can safely ignore any errors and move on. IMO it doesn't
> > > make sense to add this without a real reason.
> >
> > What I was trying to say, users would predict that a file under
> > debugfs will be created while they are opening a binder device. But if
> > it failed for some reason without any debug message, they would get
> > confused that the file doesn't exist and have no clue what happened
> > without a message.
>
> And that's fine, again, the kernel does not care if debugfs is working
> or not.  It's just a debugging help, it does not affect the normal
> operation of a system at all, and as such, userspace can't rely on it
> being present for any functionality other than debugging issues that
> might happen at times.

Thank you for explaining in detail.
I hadn't thought about it from that perspective.

Let me close this issue.

BR,
Leesoo