Re: memleak around kobject_init_and_add()

[Tobin C. Harding]

On Thu, May 02, 2019 at 09:28:08AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 02, 2019 at 09:17:42AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 02, 2019 at 07:56:16AM +1000, Tobin C. Harding wrote:
> > > On Sat, Apr 27, 2019 at 09:28:09PM +0200, Greg Kroah-Hartman wrote:
> > > > On Sat, Apr 27, 2019 at 06:13:30PM +1000, Tobin C. Harding wrote:
> > > > > (Note at bottom on reasons for 'To' list 'Cc' list)
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > kobject_init_and_add() seems to be routinely misused.  A failed call to this
> > > > > function requires a call to kobject_put() otherwise we leak memory.
> > > > > 
> > > > > Examples memleaks can be seen in:
> > > > > 
> > > > > 	mm/slub.c
> > > > > 	fs/btrfs/sysfs.c
> > > > > 	fs/xfs/xfs_sysfs.h: xfs_sysfs_init()
> > > > > 
> > > > >  Question: Do we fix the misuse or fix the API?
> > > > 
> > > > Fix the misuse.
> > > > 
> > > > > $ git grep kobject_init_and_add | wc -l
> > > > > 117
> > > > > 
> > > > > Either way, we will have to go through all 117 call sites and check them.
> > > > 
> > > > Yes.  Same for other functions like device_add(), that is the "pattern"
> > > > those users must follow.
> > > > 
> > > > > I
> > > > > don't mind fixing them all but I don't want to do it twice because I chose the
> > > > > wrong option.  Reaching out to those more experienced for a suggestion please.
> > > > > 
> > > > > Fix the API
> > > > > -----------
> > > > > 
> > > > > Typically init functions do not require cleanup if they fail, this argument
> > > > > leads to this patch
> > > > > 
> > > > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > > > index aa89edcd2b63..62328054bbd0 100644
> > > > > --- a/lib/kobject.c
> > > > > +++ b/lib/kobject.c
> > > > > @@ -453,6 +453,9 @@ int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype,
> > > > >  	retval = kobject_add_varg(kobj, parent, fmt, args);
> > > > >  	va_end(args);
> > > > >  
> > > > > +	if (retval)
> > > > > +		kobject_put(kobj);
> > > > > +
> > > > >  	return retval;
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(kobject_init_and_add);
> > > > 
> > > > I would _love_ to do this, but realize what a kobject really is.
> > > > 
> > > > It's just a "base object" that is embedded inside of some other object.
> > > > The kobject core has no idea what is going on outside of itself.  If the
> > > > kobject_init_and_add() function fails, it can NOT drop the last
> > > > reference on itself, as that would cause the memory owned by the _WHOLE_
> > > > structure the kobject is embedded in, to be freed.
> > > > 
> > > > And the kobject core can not "know" that something else needed to be
> > > > done _before_ that memory could be freed.  What if the larger structure
> > > > needs to have some other destructor called on it first?  What if
> > > > some other api initialization needs to be torn down.
> > > > 
> > > > As an example, consider this code:
> > > > 
> > > > struct foo {
> > > > 	struct kobject kobj;
> > > > 	struct baz *baz;
> > > > };
> > > > 
> > > > void foo_release(struct kobject *kobj)
> > > > {
> > > > 	struct foo *foo = container_of(kobj, struct foo, kobj);
> > > > 	kfree(foo);
> > > > }
> > > > 
> > > > struct kobj_type foo_ktype = {
> > > > 	.release = foo_release,
> > > > };
> > > > 
> > > > struct foo *foo_create(struct foo *parent, char *name)
> > > > {
> > > > 	struct *foo;
> > > > 
> > > > 	foo = kzalloc(sizeof(*foo), GFP_KERNEL);
> > > > 	if (!foo)
> > > > 		return NULL;
> > > > 
> > > > 	foo->baz = baz_create(name);
> > > > 	if (!foo->baz)
> > > > 		return NULL;
> > > > 
> > > > 	ret = kobject_init_and_add(&foo->kobj, foo_ktype, &parent->kobj, "foo-%s", name);
> > > > 	if (ret) {
> > > > 		baz_destroy(foo->baz);
> > > > 		kobject_put(&foo->kobj);
> > > > 		return NULL;
> > > > 	}
> > > > 
> > > > 	return foo;
> > > > }
> > > > 
> > > > void foo_destroy(struct foo *foo)
> > > > {
> > > > 	baz_destroy(foo->baz);
> > > > 	kobject_del(&foo->kobj);
> > > 	kojbect_put(&foo->kobj);
> > > > }
> > > 
> > > Does this need this extra call to kobject_put()?  Then foo_create()
> > > leaves foo with a refcount of 1 and foo_destroy drops that refcount.
> > 
> > Oops, no, I messed this up, it should _only_ be a call to
> > kobject_put(), kobject_del() is not needed here.
> > 
> > kobject_del() is for people who "really want to control the lifetime" of
> > a kobject.  All it does is remove the kobject from sysfs, and drop the
> > parent reference of the kobject, allowing the kobject to be "free" on
> > it's own.  Later a kobject_put() call must be called on it to really
> > clean it up.
> > 
> > If you just call kobject_put(), and this is the last reference,
> > kobject_del() will be correctly called for you by the kobject code, as
> > it "knows" this is time to clean up the sysfs entities.
> > 
> > A "normal" user should never have to call kobject_del().
> 
> Which means your other patch about the kerneldoc for that function is
> also not correct, I'll go fix that up now...

lols all around with this one :)  Who knew reference counting could get
so knotty

Cheers,
Tobin.