Messages in this thread Patch in this message | | | Date | Sat, 3 Aug 2019 09:15:55 +0200 | From | "" <> | Subject | Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add |
| |
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
| |