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.
