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

[gregkh@xxxxxxxxxxxxxxxxxxx]

On Fri, Aug 02, 2019 at 08:19:37PM +0000, Michael Kelley wrote:
> From: gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxxxxxxxxxxx>

greg k-h