linux-kernel - Re: [PATCH] iio: core: use debugfs_remove

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] [day] [month] [year] [list]

Date:   Mon, 23 Dec 2019 17:29:27 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "lars@...afoo.de" <lars@...afoo.de>
Subject: Re: [PATCH] iio: core: use debugfs_remove_recursive() on IIO unload

On Mon, 16 Dec 2019 07:43:30 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@...log.com> wrote:

> On Sun, 2019-12-15 at 16:25 +0000, Jonathan Cameron wrote:
> > [External]
> > 
> > On Wed, 11 Dec 2019 16:59:03 +0200
> > Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> >   
> > > From: Lars-Peter Clausen <lars@...afoo.de>
> > > 
> > > The debugfs dir may have multiple files/dirs attached to it. Use
> > > debugfs_remove_recursive() to clean it up when the IIO module unloads.
> > > 
> > > Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> > > ---
> > > 
> > > Note: note sure if this needs a Fixes tag. It's been in here for ages
> > > now,
> > >       so it may not be a big issue.  
> > 
> > There is some argument in favour of doing this as a defensive measure,
> > but
> > in current IIO debugfs directories are only created by drivers that call
> > (indirectly) iio_device_register_debugfs and those must call
> > (again indirectly) iio_device_unregister_debugfs which has called
> > the recursive free on everything in their directories.  So unless
> > something
> > very odd is going on this isn't a bug.
> > 
> > Am I missing something?  
> 
> So, I admit my note is open for interpretation.
> [ Rergarding the "Fixes" tag, I'm still not super-clear on when/why/how
> it's needed. I'll get there soon though. ]
> 
> This patch has been in our tree since 2012, and I'm not sure why it's using
> debugfs_remove_recursive() vs plain debugfs_remove().
> 
> I sometimes take a look at these old patches, try to make some sense of
> them and upstream them, or override them with the version of upstream code.
> My intent is to sync ADI code [for IIO] closer to upstream. Which would
> make it easier to track any divergence.
> 
> We're not seeing any bug with/without this.
> I guess my note about the "Fixes" tag may be a bit clumsy.
> 
> From my side, both debugfs_remove_recursive() & plain debugfs_remove() are
> fine.
I'd rather leave it be as it kind of avoids people thinking they can get away
without cleaning up properly at the higher levels.

Thanks,

Jonathan

> 
> Thanks
> Alex
> 
> > 
> >   
> > >  drivers/iio/industrialio-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/industrialio-core.c
> > > b/drivers/iio/industrialio-core.c
> > > index 9a3579943574..dab67cb69fe6 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -292,7 +292,7 @@ static void __exit iio_exit(void)
> > >  	if (iio_devt)
> > >  		unregister_chrdev_region(iio_devt, IIO_DEV_MAX);
> > >  	bus_unregister(&iio_bus_type);
> > > -	debugfs_remove(iio_debugfs_dentry);
> > > +	debugfs_remove_recursive(iio_debugfs_dentry);
> > >  }
> > >  
> > >  #if defined(CONFIG_DEBUG_FS)