Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add

Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add

[gregkh]

On Thu, Aug 01, 2019 at 11:53:53PM +0000, Michael Kelley wrote:
> If alloc_descs fails before irq_sysfs_init has run, free_desc in the
> cleanup path will call kobject_del even though the kobject has not
> been added with kobject_add. Fix this by making the call to
> kobject_del conditional on whether irq_sysfs_init has run.
> 
> This problem surfaced because commit aa30f47cf666
> ("kobject: Add support for default attribute groups to kobj_type")
> makes kobject_del stricter about pairing with kobject_add. If the
> pairing is incorrrect, a WARNING and backtrace occur in
> sysfs_remove_group because there is no parent.
> 
> Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  kernel/irq/irqdesc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 9484e88..5447760 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -438,7 +438,8 @@ static void free_desc(unsigned int irq)
>  	 * The sysfs entry must be serialized against a concurrent
>  	 * irq_sysfs_init() as well.
>  	 */
> -	kobject_del(&desc->kobj);
> +	if (irq_kobj_base)
> +		kobject_del(&desc->kobj);

But now you leak the memory of desc as there is no chance it could be
freed, because the kobject release function is never called :(

Relying on irq_kobj_bas to be present or not seems like an odd test
here.

thanks,

greg k-h

Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add

[gregkh]

On Fri, Aug 02, 2019 at 08:19:37PM +0000, Michael Kelley wrote:
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> Sent: Thursday, August 1, 2019 11:34 PM
> > On Thu, Aug 01, 2019 at 11:53:53PM +0000, Michael Kelley wrote:
> > > If alloc_descs fails before irq_sysfs_init has run, free_desc in the
> > > cleanup path will call kobject_del even though the kobject has not
> > > been added with kobject_add. Fix this by making the call to
> > > kobject_del conditional on whether irq_sysfs_init has run.
> > >
> > > This problem surfaced because commit aa30f47cf666
> > > ("kobject: Add support for default attribute groups to kobj_type")
> > > makes kobject_del stricter about pairing with kobject_add. If the
> > > pairing is incorrrect, a WARNING and backtrace occur in
> > > sysfs_remove_group because there is no parent.
> > >
> > > Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
> > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > > ---
> > >  kernel/irq/irqdesc.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> > > index 9484e88..5447760 100644
> > > --- a/kernel/irq/irqdesc.c
> > > +++ b/kernel/irq/irqdesc.c
> > > @@ -438,7 +438,8 @@ static void free_desc(unsigned int irq)
> > >  	 * The sysfs entry must be serialized against a concurrent
> > >  	 * irq_sysfs_init() as well.
> > >  	 */
> > > -	kobject_del(&desc->kobj);
> > > +	if (irq_kobj_base)
> > > +		kobject_del(&desc->kobj);
> > 
> > But now you leak the memory of desc as there is no chance it could be
> > freed, because the kobject release function is never called :(
> 
> In the alloc_descs error path, when irq_kobj_base is still NULL, the
> kobject code sequence is:
> 	kobject_init()   [as called by alloc_desc]
> 	kobject_put()   [as called by delayed_free_desc]
> 
> So I don't think anything leaks.
> 
> If irq_kobj_base is not NULL, the kobject code sequence is:
> 	kobject_init()   [as called by alloc_desc]
> 	kobject_add()  [as called by irq_sysfs_add]
> 	kobject_del()   [as called by free_desc]
> 	kobject_put()   [as called by delayed_free_desc]
> 
> Again, everything is paired up properly.
> 
> > 
> > Relying on irq_kobj_base to be present or not seems like an odd test
> > here.
> > 
> 
> It's the same test that is used in irq_sysfs_add to decide whether to
> call kobject_add.  So it makes everything paired up and symmetrical.

Ugh, that's a tangled mess and totally not obvious at all.  I'm sure
there's a good reason for all of that, and I really don't want to know
:)

Anyway, yes, you are right, the patch is fine, sorry for the noise.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

greg k-h

Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add

[Thomas Gleixner]

On Sat, 3 Aug 2019, gregkh@linuxfoundation.org wrote:
> On Fri, Aug 02, 2019 at 08:19:37PM +0000, Michael Kelley wrote:
> > > Relying on irq_kobj_base to be present or not seems like an odd test
> > > here.
> > > 
> > 
> > It's the same test that is used in irq_sysfs_add to decide whether to
> > call kobject_add.  So it makes everything paired up and symmetrical.
> 
> Ugh, that's a tangled mess and totally not obvious at all.  I'm sure
> there's a good reason for all of that, and I really don't want to know
> :)

It's all your fault that the sysfs stuff does not work in very early boot.

/me runs

[tip:irq/urgent] genirq: Properly pair kobject_del() with kobject_add()

[tip-bot for Michael Kelley]

Commit-ID:  e1ee29624746fbf667f80e8ae3815a76e4d1bd5b
Gitweb:     https://git.kernel.org/tip/e1ee29624746fbf667f80e8ae3815a76e4d1bd5b
Author:     Michael Kelley <mikelley@microsoft.com>
AuthorDate: Thu, 1 Aug 2019 23:53:53 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 19 Aug 2019 15:56:28 +0200

genirq: Properly pair kobject_del() with kobject_add()

If alloc_descs() fails before irq_sysfs_init() has run, free_desc() in the
cleanup path will call kobject_del() even though the kobject has not been
added with kobject_add().

Fix this by making the call to kobject_del() conditional on whether
irq_sysfs_init() has run.

This problem surfaced because commit aa30f47cf666 ("kobject: Add support
for default attribute groups to kobj_type") makes kobject_del() stricter
about pairing with kobject_add(). If the pairing is incorrrect, a WARNING
and backtrace occur in sysfs_remove_group() because there is no parent.

[ tglx: Add a comment to the code ]

Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/1564703564-4116-1-git-send-email-mikelley@microsoft.com

---
 kernel/irq/irqdesc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9484e88dabc2..51f42f3caf09 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -437,8 +437,14 @@ static void free_desc(unsigned int irq)
 	 *
 	 * The sysfs entry must be serialized against a concurrent
 	 * irq_sysfs_init() as well.
+	 *
+	 * If irq_sysfs_init() has not yet been invoked (early boot), then
+	 * irq_kobj_base is NULL and the descriptor was never added.
+	 * kobject_del() complains about a object with no parent, so make
+	 * it conditional.
 	 */
-	kobject_del(&desc->kobj);
+	if (irq_kobj_base)
+		kobject_del(&desc->kobj);
 	delete_irq_desc(irq);
 
 	/*

RE: [tip:irq/urgent] genirq: Properly pair kobject_del() with kobject_add()

[Michael Kelley]

From: Sasha Levin <sashal@kernel.org>  Sent: Monday, August 19, 2019 2:28 PM
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: ecb3f394c5db genirq: Expose interrupt information through sysfs.
> 
> The bot has tested the following trees: v5.2.9, v4.19.67, v4.14.139, v4.9.189.
> 
> v5.2.9: Build failed! Errors:
>     kernel/irq/irqdesc.c:446:6: error: ‘irq_kobj_base’ undeclared (first use in this function);
> did you mean ‘irq_kobj_type’?
> 
> v4.19.67: Build failed! Errors:
>     kernel/irq/irqdesc.c:445:6: error: ‘irq_kobj_base’ undeclared (first use in this function);
> did you mean ‘irq_kobj_type’?
> 
> v4.14.139: Build failed! Errors:
>     kernel/irq/irqdesc.c:428:6: error: ‘irq_kobj_base’ undeclared (first use in this function);
> did you mean ‘irq_kobj_type’?
> 
> v4.9.189: Build failed! Errors:
>     kernel/irq/irqdesc.c:414:6: error: ‘irq_kobj_base’ undeclared (first use in this function);
> did you mean ‘irq_kobj_type’?
> 
> 
> NOTE: The patch will not be queued to stable trees until it is upstream.
> 
> How should we proceed with this patch?

Compile error occurs when CONFIG_SYSFS is not selected.  It's probably cleanest to
revert the current patch.   I'll send out a new version that fixes the problem.

Michael