linux-kernel - Re: [PATCH] remoteproc: Don't bother checking the return value of debugfs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Mar 2022 20:01:46 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     bjorn.andersson@...aro.org, linux-remoteproc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] remoteproc: Don't bother checking the return value of
 debugfs_create*

Hi Mathieu,

On Mon, Mar 28, 2022 at 09:51:23AM -0600, Mathieu Poirier wrote:
> Hi Mani,
> 
> On Thu, Mar 24, 2022 at 11:42:24PM +0530, Manivannan Sadhasivam wrote:
> > DebugFS APIs are designed to return only the error pointers and not NULL
> > in the case of failure. So these return pointers are safe to be passed on
> > to the successive debugfs_create* APIs.
> > 
> > Therefore, let's just get rid of the checks.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > ---
> >  drivers/remoteproc/remoteproc_debugfs.c | 17 ++---------------
> >  1 file changed, 2 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > index b5a1e3b697d9..2e2c4a31c154 100644
> > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > @@ -386,16 +386,8 @@ void rproc_remove_trace_file(struct dentry *tfile)
> >  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> >  				       struct rproc_debug_trace *trace)
> >  {
> > -	struct dentry *tfile;
> > -
> > -	tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> > +	return debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> >  				    &trace_rproc_ops);
> > -	if (!tfile) {
> > -		dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
> > -		return NULL;
> > -	}
> > -
> > -	return tfile;
> 
> Please see this thread [1] for an earlier conversation on this topic.
> 
> [1]. https://lore.kernel.org/lkml/20220105131022.25247-1-linmq006@gmail.com/T/
> 

Thanks for the pointer! I believe the conclusion was to return 0 here
and ignore the return from debugfs_create_file(). If that's the case, it looks
fine to me and I'll send a follow-up patch.

> >  }
> >  
> >  void rproc_delete_debug_dir(struct rproc *rproc)
> > @@ -411,8 +403,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
> >  		return;
> >  
> >  	rproc->dbg_dir = debugfs_create_dir(dev_name(dev), rproc_dbg);
> > -	if (!rproc->dbg_dir)
> > -		return;
> > 
> >  	debugfs_create_file("name", 0400, rproc->dbg_dir,
> >  			    rproc, &rproc_name_ops);
> > @@ -430,11 +420,8 @@ void rproc_create_debug_dir(struct rproc *rproc)
> >  
> >  void __init rproc_init_debugfs(void)
> >  {
> > -	if (debugfs_initialized()) {
> > +	if (debugfs_initialized())
> >  		rproc_dbg = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > -		if (!rproc_dbg)
> > -			pr_err("can't create debugfs dir\n");
> > -	}
> 
> The above two are fine since debugfs_create_file() and debugfs_create_dir() can
> deal with @parent being an error code.
> 

debugfs_create_* APIs would never return NULL, so these checks are wrong.
Moreover, Greg recommends not to check the return value for any of these
functions.

I've found the mail thread where Greg explained the reasoning behind it:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1907800.html

Thanks,
Mani

> Thanks,
> Mathieu
> 
> >  }
> >  
> >  void __exit rproc_exit_debugfs(void)
> > -- 
> > 2.25.1
> >