* Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add
[not found] <1564703564-4116-1-git-send-email-mikelley@microsoft.com>
@ 2019-08-02 6:34 ` gregkh
[not found] ` <MWHPR21MB078463AB854A336842118405D7D90@MWHPR21MB0784.namprd21.prod.outlook.com>
2019-08-19 14:01 ` [tip:irq/urgent] genirq: Properly pair kobject_del() with kobject_add() tip-bot for Michael Kelley
1 sibling, 1 reply; 5+ messages in thread
From: gregkh @ 2019-08-02 6:34 UTC (permalink / raw)
To: Michael Kelley; +Cc: linux-kernel, tglx, boqun.feng, kimbrownkd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add
[not found] ` <MWHPR21MB078463AB854A336842118405D7D90@MWHPR21MB0784.namprd21.prod.outlook.com>
@ 2019-08-03 7:15 ` gregkh
2019-08-03 7:43 ` Thomas Gleixner
0 siblings, 1 reply; 5+ messages in thread
From: gregkh @ 2019-08-03 7:15 UTC (permalink / raw)
To: Michael Kelley; +Cc: linux-kernel, tglx, boqun.feng, kimbrownkd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add
2019-08-03 7:15 ` gregkh
@ 2019-08-03 7:43 ` Thomas Gleixner
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Gleixner @ 2019-08-03 7:43 UTC (permalink / raw)
To: gregkh; +Cc: Michael Kelley, linux-kernel, boqun.feng, kimbrownkd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip:irq/urgent] genirq: Properly pair kobject_del() with kobject_add()
[not found] <1564703564-4116-1-git-send-email-mikelley@microsoft.com>
2019-08-02 6:34 ` [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add gregkh
@ 2019-08-19 14:01 ` tip-bot for Michael Kelley
[not found] ` <20190819212758.6D03D22CEC@mail.kernel.org>
1 sibling, 1 reply; 5+ messages in thread
From: tip-bot for Michael Kelley @ 2019-08-19 14:01 UTC (permalink / raw)
To: linux-tip-commits; +Cc: linux-kernel, mikelley, gregkh, hpa, mingo, tglx
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);
/*
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [tip:irq/urgent] genirq: Properly pair kobject_del() with kobject_add()
[not found] ` <20190819212758.6D03D22CEC@mail.kernel.org>
@ 2019-08-19 21:58 ` Michael Kelley
0 siblings, 0 replies; 5+ messages in thread
From: Michael Kelley @ 2019-08-19 21:58 UTC (permalink / raw)
To: Sasha Levin, tip-bot for Michael Kelley, linux-tip-commits
Cc: linux-kernel, stable, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-19 21:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1564703564-4116-1-git-send-email-mikelley@microsoft.com>
2019-08-02 6:34 ` [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add gregkh
[not found] ` <MWHPR21MB078463AB854A336842118405D7D90@MWHPR21MB0784.namprd21.prod.outlook.com>
2019-08-03 7:15 ` gregkh
2019-08-03 7:43 ` Thomas Gleixner
2019-08-19 14:01 ` [tip:irq/urgent] genirq: Properly pair kobject_del() with kobject_add() tip-bot for Michael Kelley
[not found] ` <20190819212758.6D03D22CEC@mail.kernel.org>
2019-08-19 21:58 ` Michael Kelley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.