linux-kernel - Re: [PATCH] irqdomain: remove debugfs_file from struct irq

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:   Thu, 18 Feb 2021 09:54:00 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Michael Walle <michael@...le.cc>, linux-kernel@...r.kernel.org,
        tglx@...utronix.de
Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Thu, Feb 18, 2021 at 08:47:15AM +0000, Marc Zyngier wrote:
> On 2021-02-18 08:38, Greg KH wrote:
> > On Thu, Feb 18, 2021 at 09:27:09AM +0100, Michael Walle wrote:
> > > Am 2021-02-18 08:31, schrieb Greg KH:
> > > > On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
> > > > > On Wed, 17 Feb 2021 20:10:50 +0000,
> > > > > Michael Walle <michael@...le.cc> wrote:
> > > > > >
> > > > > > Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > > > > > > On 2021-02-17 19:57, Michael Walle wrote:
> > > > > > >> Hi Greg,
> > > > > > >>
> > > > > > >>> There's no need to keep around a dentry pointer to a simple file that
> > > > > > >>> debugfs itself can look up when we need to remove it from the system.
> > > > > > >>> So simplify the code by deleting the variable and cleaning up the
> > > > > > >>> logic
> > > > > > >>> around the debugfs file.
> > > > > > >>
> > > > > > >> This will generate the following oops on my board (arm64,
> > > > > > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> > > > > > >> debugfs_mount is NULL.
> > > > > > >
> > > > > > > That's odd. I gave it a go yesterday, and nothing blew up.
> > > > > > > Which makes me wonder whether I had the debug stuff enabled
> > > > > > > the first place...
> > > > > > >
> > > > > > > I've dropped the patch from -next for now until I figure it out
> > > > > > > (probably tomorrow).
> > > > > >
> > > > > > Mh, maybe its my .config, I've attached it. I also noticed that
> > > > > > the board boots just fine in our kernel-ci [1].
> > > > >
> > > > > I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> > > > > reasons, and it caught fire as I re-enabled it.
> > > > >
> > > > > Adding this fixes it for me:
> > > > >
> > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > > > > index 367ff1c35f75..d8a14cf1a7b6 100644
> > > > > --- a/kernel/irq/irqdomain.c
> > > > > +++ b/kernel/irq/irqdomain.c
> > > > > @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
> > > > > irq_domain *d)
> > > > >
> > > > >  static void debugfs_remove_domain_dir(struct irq_domain *d)
> > > > >  {
> > > > > -	debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > > > +	if (domain_dir)
> > > > > +		debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > > >  }
> > > > >
> > > > >  void __init irq_domain_debugfs_init(struct dentry *root)
> > > > >
> > > > >
> > > > > Could you please check whether it works for you?
> > > >
> > > > Can you try this debugfs core change instead?  Callers to debugfs should
> > > > not have to do the above type of checking as debugfs should be much more
> > > > robust than that.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > > >
> > > >
> > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > > index 2fcf66473436..5aa798f52b6e 100644
> > > > --- a/fs/debugfs/inode.c
> > > > +++ b/fs/debugfs/inode.c
> > > > @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
> > > > struct dentry *parent)
> > > >  {
> > > >  	struct dentry *dentry;
> > > >
> > > > -	if (IS_ERR(parent))
> > > > +	if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
> > > >  		return NULL;
> > > >
> > > >  	if (!parent)
> > > 
> > > This doesn't work. name is not NULL when it is called.
> > 
> > Ok, but it is a good check we need to make anyway, I'll add it to a
> > patch series :)
> > 
> > > What has to happen before debugfs_lookup() can be called? Looks like
> > > someone has to initialize the static debugfs_mount, first.
> > 
> > Wow, wait, you are removing a debugfs file _before_ debugfs is even
> > initialized?  Didn't expect that, ok, let me go try this again...
> 
> Yeah, that's a poor man's rename (file being deleted and re-created).

True, but that's not happening here, right?  Some driver is being
initialized and creates a debugfs file, and then decides to unload so it
removes the debugfs file?

Why was it trying to create the file in the first place if it didn't
properly bind to the hardware?

odd...

greg k-h