On Fri, Aug 02, 2019 at 08:19:37PM +, Michael Kelley wrote:
> From: gre...@linuxfoundation.org Sent: Thursday,
> August 1, 2019 11:34 PM
> > On Thu, Aug 01, 2019 at 11:53:53PM +, 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
> > > ---
> > > 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(>kobj);
> > > + if (irq_kobj_base)
> > > + kobject_del(>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
greg k-h
