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.
