memleak around kobject_init_and_add()

memleak around kobject_init_and_add()

[Tobin C. Harding]

(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?

$ git grep kobject_init_and_add | wc -l
117

Either way, we will have to go through all 117 call sites and check them.  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);


Fix all the call sites
----------------------

Go through all 117 call sites and add kobj_put() in the error path.

This example from mm/slub.c

diff --git a/mm/slub.c b/mm/slub.c
index d30ede89f4a6..84a9d6c06c27 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5756,8 +5756,10 @@ static int sysfs_slab_add(struct kmem_cache *s)
 
 	s->kobj.kset = kset;
 	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
-	if (err)
+	if (err) {
+		kobject_put(&s->kobj);
 		goto out;
+	}
 
 	err = sysfs_create_group(&s->kobj, &slab_attr_group);
 	if (err)


thanks,
Tobin.


This is a Saturday afternoon 'drinking some wine, hacking on the kernel'
email.  Sending it to the lib/kobject.c maintainers for obvious
reasons.  CC'd a few extra people who I thought might be interested to
take the weight of Greg and Rafael :)

Re: memleak around kobject_init_and_add()

[Greg Kroah-Hartman]

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);
}

Now if kobject_init_and_add() had failed, and called kobject_put() right
away, that would have freed the larger "struct foo", but not cleaned up
the reference to the baz pointer.

Yes, you can move all of the other destruction logic into the release
function, to then get rid of baz, but that really doesn't work in the
real world as there are times you want to drop that when you "know" you
can drop it, not when the last reference goes away as those are
different lifecycles.

Same thing goes for 'struct device'.  It too is a kobject, so think
about if the driver core's call to initialize the kobject failed, would
it be ok at that exact moment in time to free everything?

Look at the "joy" that is device_add().  If kobject_add() fails, we have
to clean up the glue directory that we had created, _before_ we can then
call put_device().  Then stack another layer on top of that, look at
usb_new_device().  If the call to device_add() fails, it needs to do
some housekeeping before it can drop the last reference to the device to
free the memory up.

> Fix all the call sites
> ----------------------
> 
> Go through all 117 call sites and add kobj_put() in the error path.

Yes.

> This example from mm/slub.c
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index d30ede89f4a6..84a9d6c06c27 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5756,8 +5756,10 @@ static int sysfs_slab_add(struct kmem_cache *s)
>  
>  	s->kobj.kset = kset;
>  	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
> -	if (err)
> +	if (err) {
> +		kobject_put(&s->kobj);
>  		goto out;
> +	}

Yup, it sucks, but unless you can think of a better way to do all of
this, that's the requirement here.  Again, same thing for a call to
device_add().  Another subsystem got burned by this just the other day
and we added yet-another-line in the documentation trying to call it out
explicitly.

Kernel programming is hard, sorry, let's go shopping...

> This is a Saturday afternoon 'drinking some wine, hacking on the kernel'
> email.  Sending it to the lib/kobject.c maintainers for obvious
> reasons.  CC'd a few extra people who I thought might be interested to
> take the weight of Greg and Rafael :)

Pass me that wine, I think I need it after writing this email :)

thanks,

greg k-h

Re: memleak around kobject_init_and_add()

[Tobin C. Harding]

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.

Cool, I got it!

> > $ 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);
> }
> 
> Now if kobject_init_and_add() had failed, and called kobject_put() right
> away, that would have freed the larger "struct foo", but not cleaned up
> the reference to the baz pointer.
> 
> Yes, you can move all of the other destruction logic into the release
> function, to then get rid of baz, but that really doesn't work in the
> real world as there are times you want to drop that when you "know" you
> can drop it, not when the last reference goes away as those are
> different lifecycles.
> 
> Same thing goes for 'struct device'.  It too is a kobject, so think
> about if the driver core's call to initialize the kobject failed, would
> it be ok at that exact moment in time to free everything?
> 
> Look at the "joy" that is device_add().  If kobject_add() fails, we have
> to clean up the glue directory that we had created, _before_ we can then
> call put_device().  Then stack another layer on top of that, look at
> usb_new_device().  If the call to device_add() fails, it needs to do
> some housekeeping before it can drop the last reference to the device to
> free the memory up.

Thanks for the detailed response, unusual objects call for unusual
coding practices eh ;)

> > Fix all the call sites
> > ----------------------
> > 
> > Go through all 117 call sites and add kobj_put() in the error path.
> 
> Yes.
> 
> > This example from mm/slub.c
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index d30ede89f4a6..84a9d6c06c27 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -5756,8 +5756,10 @@ static int sysfs_slab_add(struct kmem_cache *s)
> >  
> >  	s->kobj.kset = kset;
> >  	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
> > -	if (err)
> > +	if (err) {
> > +		kobject_put(&s->kobj);
> >  		goto out;
> > +	}
> 
> Yup, it sucks, but unless you can think of a better way to do all of
> this, that's the requirement here.  Again, same thing for a call to
> device_add().  Another subsystem got burned by this just the other day
> and we added yet-another-line in the documentation trying to call it out
> explicitly.
> 
> Kernel programming is hard, sorry, let's go shopping...

lols, and good documentation is at times even harder than kernel
programming.

Thanks, patches to follow.


	Tobin.

Re: memleak around kobject_init_and_add()

[Tobin C. Harding]

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.

Following on from this.  It seems we often also forget to call
kobject_uevent() after calls to kobject_init_and_add().  Before I make a
goose of myself patching the whole tree is there ever any reason why we
would _not_ want to call kobject_uevent() after successfully calling
kobject_add() (or kobject_init_and_add())?

Cheers,
Tobin.

Re: memleak around kobject_init_and_add()

[Greg Kroah-Hartman]

On Sun, Apr 28, 2019 at 11:19:57AM +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.
> 
> Following on from this.  It seems we often also forget to call
> kobject_uevent() after calls to kobject_init_and_add().

Are you sure?  Usually if you don't call it right away, it happens much
later when you have everything "ready to go" to tell userspace that it
then can access that kobject successfully.

Any specific places you feel is not correct?

> Before I make a goose of myself patching the whole tree is there ever
> any reason why we would _not_ want to call kobject_uevent() after
> successfully calling kobject_add() (or kobject_init_and_add())?

You should always do so, but again, sometimes it can be much "later"
after everything is properly set up.

Ok, at quick glance I see some places that do not properly call this.
But, those places should not even be using a "raw" kobject in the first
place, they should be using 'struct device'.  If code using a kobject,
that should be very "rare", and not normal behavior in the first place.

thanks,

greg k-h

Re: memleak around kobject_init_and_add()

[Tobin C. Harding]

On Mon, Apr 29, 2019, at 02:15, Greg Kroah-Hartman wrote:
> On Sun, Apr 28, 2019 at 11:19:57AM +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.
> > 
> > Following on from this.  It seems we often also forget to call
> > kobject_uevent() after calls to kobject_init_and_add().
> 
> Are you sure?  Usually if you don't call it right away, it happens much
> later when you have everything "ready to go" to tell userspace that it
> then can access that kobject successfully.
> 
> Any specific places you feel is not correct?
> 
> > Before I make a goose of myself patching the whole tree is there ever
> > any reason why we would _not_ want to call kobject_uevent() after
> > successfully calling kobject_add() (or kobject_init_and_add())?
> 
> You should always do so, but again, sometimes it can be much "later"
> after everything is properly set up.
> 
> Ok, at quick glance I see some places that do not properly call this.
> But, those places should not even be using a "raw" kobject in the first
> place, they should be using 'struct device'.  If code using a kobject,
> that should be very "rare", and not normal behavior in the first place.

Cool, thanks.

Re: memleak around kobject_init_and_add()

[Tobin C. Harding]

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.

Thanks for taking the time to explain this stuff.

thanks
Tobin.


Leaving below for reference.

> Now if kobject_init_and_add() had failed, and called kobject_put() right
> away, that would have freed the larger "struct foo", but not cleaned up
> the reference to the baz pointer.
> 
> Yes, you can move all of the other destruction logic into the release
> function, to then get rid of baz, but that really doesn't work in the
> real world as there are times you want to drop that when you "know" you
> can drop it, not when the last reference goes away as those are
> different lifecycles.
> 
> Same thing goes for 'struct device'.  It too is a kobject, so think
> about if the driver core's call to initialize the kobject failed, would
> it be ok at that exact moment in time to free everything?
> 
> Look at the "joy" that is device_add().  If kobject_add() fails, we have
> to clean up the glue directory that we had created, _before_ we can then
> call put_device().  Then stack another layer on top of that, look at
> usb_new_device().  If the call to device_add() fails, it needs to do
> some housekeeping before it can drop the last reference to the device to
> free the memory up.

Re: memleak around kobject_init_and_add()

[Greg Kroah-Hartman]

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().

thanks,

greg k-h

Re: memleak around kobject_init_and_add()

[Greg Kroah-Hartman]

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...

thanks,

greg k-h

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.

[PATCH] kobject: clean up the kobject add documentation a bit more

[Greg Kroah-Hartman]

Commit 1fd7c3b438a2 ("kobject: Improve doc clarity kobject_init_and_add()")
tried to provide more clarity, but the reference to kobject_del() was
incorrect.  Fix that up by removing that line, and hopefully be more explicit
as to exactly what needs to happen here once you register a kobject with the
kobject core.

Cc: Tobin C. Harding <tobin@kernel.org>
Fixes: 1fd7c3b438a2 ("kobject: Improve doc clarity kobject_init_and_add()")
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/lib/kobject.c b/lib/kobject.c
index 3f4b7e95b0c2..f2ccdbac8ed9 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -416,8 +416,12 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
  *         to this function be directly freed with a call to kfree(),
  *         that can leak memory.
  *
- *         If this call returns successfully and you later need to unwind
- *         kobject_add() for the error path you should call kobject_del().
+ *         If this function returns success, kobject_put() must also be called
+ *         in order to properly clean up the memory associated with the object.
+ *
+ *         In short, once this function is called, kobject_put() MUST be called
+ *         when the use of the object is finished in order to properly free
+ *         everything.
  */
 int kobject_add(struct kobject *kobj, struct kobject *parent,
 		const char *fmt, ...)

Re: [PATCH] kobject: clean up the kobject add documentation a bit more

[Tobin C. Harding]

On Thu, May 02, 2019 at 12:22:24PM +0200, Greg Kroah-Hartman wrote:
> Commit 1fd7c3b438a2 ("kobject: Improve doc clarity kobject_init_and_add()")
> tried to provide more clarity, but the reference to kobject_del() was
> incorrect.  Fix that up by removing that line, and hopefully be more explicit
> as to exactly what needs to happen here once you register a kobject with the
> kobject core.
> 
> Cc: Tobin C. Harding <tobin@kernel.org>
> Fixes: 1fd7c3b438a2 ("kobject: Improve doc clarity kobject_init_and_add()")
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 3f4b7e95b0c2..f2ccdbac8ed9 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -416,8 +416,12 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
>   *         to this function be directly freed with a call to kfree(),
>   *         that can leak memory.
>   *
> - *         If this call returns successfully and you later need to unwind
> - *         kobject_add() for the error path you should call kobject_del().
> + *         If this function returns success, kobject_put() must also be called
> + *         in order to properly clean up the memory associated with the object.
> + *
> + *         In short, once this function is called, kobject_put() MUST be called
> + *         when the use of the object is finished in order to properly free
> + *         everything.
>   */
>  int kobject_add(struct kobject *kobj, struct kobject *parent,
>  		const char *fmt, ...)

Ack! (Do I get to do those :)

I'm not convinced we have the docs for kobject clear enough for a
kobject noob to read but this patch defiantly fixes the error I
introduced.

thanks,
Tobin.

Re: [PATCH] kobject: clean up the kobject add documentation a bit more

[Greg Kroah-Hartman]

On Fri, May 03, 2019 at 11:25:29AM +1000, Tobin C. Harding wrote:
> On Thu, May 02, 2019 at 12:22:24PM +0200, Greg Kroah-Hartman wrote:
> > Commit 1fd7c3b438a2 ("kobject: Improve doc clarity kobject_init_and_add()")
> > tried to provide more clarity, but the reference to kobject_del() was
> > incorrect.  Fix that up by removing that line, and hopefully be more explicit
> > as to exactly what needs to happen here once you register a kobject with the
> > kobject core.
> > 
> > Cc: Tobin C. Harding <tobin@kernel.org>
> > Fixes: 1fd7c3b438a2 ("kobject: Improve doc clarity kobject_init_and_add()")
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 3f4b7e95b0c2..f2ccdbac8ed9 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -416,8 +416,12 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
> >   *         to this function be directly freed with a call to kfree(),
> >   *         that can leak memory.
> >   *
> > - *         If this call returns successfully and you later need to unwind
> > - *         kobject_add() for the error path you should call kobject_del().
> > + *         If this function returns success, kobject_put() must also be called
> > + *         in order to properly clean up the memory associated with the object.
> > + *
> > + *         In short, once this function is called, kobject_put() MUST be called
> > + *         when the use of the object is finished in order to properly free
> > + *         everything.
> >   */
> >  int kobject_add(struct kobject *kobj, struct kobject *parent,
> >  		const char *fmt, ...)
> 
> Ack! (Do I get to do those :)

Yes you do, thanks, now added to the patch and queued up.

greg k-h