On Thu, May 02, 2019 at 10:34:12AM +0200, Petr Mladek wrote:
> On Wed 2019-05-01 09:38:03, Tobin C. Harding wrote:
> > Hi,
> >
> > Looks like I've created a bit of confusion trying to fix memleaks in
> > calls to kobject_init_and_add(). Its spread over various patches and
> > mailing lists so I'm starting a new thread and CC'ing anyone that
> > commented on one of those patches.
> >
> > If there is a better way to go about this discussion please do tell me.
> >
> > The problem
> > -----------
> >
> > Calls to kobject_init_and_add() are leaking memory throughout the kernel
> > because of how the error paths are handled.
> >
> > The solution
> > ------------
> >
> > Write the error path code correctly.
> >
> > Example
> > -------
> >
> > We have samples/kobject/kobject-example.c but it uses
> > kobject_create_and_add(). I thought of adding another example file here
> > but could not think of how to do it off the top of my head without being
> > super contrived. Can add this to the TODO list if it will help.
> >
> > Here is an attempted canonical usage of kobject_init_and_add() typical
> > of the code that currently is getting it wrong. This is the second time
> > I've written this and the first time it was wrong even after review (you
> > know who you are, you are definitely buying the next round of drinks :)
> >
> >
> > Assumes we have an object in memory already that has the kobject
> > embedded in it. Variable 'kobj' below would typically be &ptr->kobj
> >
> >
> > void fn(void)
> > {
> > int ret;
> >
> > ret = kobject_init_and_add(kobj, ktype, NULL, "foo");
> > if (ret) {
> > /*
> > * This means kobject_init() has succeeded
> > * but kobject_add() failed.
> > */
> > goto err_put;
> > }
>
> It is strange to make the structure visible in sysfs before
> we initialize it.
>
> > ret = some_init_fn();
> > if (ret) {
> > /*
> > * We need to wind back kobject_add() AND kobject_put().
> > * kobject_add() incremented the refcount in
> > * kobj->parent, that needs to be decremented THEN we
> > need
> > * the call to kobject_put() to decrement the
> > * refcount of kobj.
> */
> > goto err_del;
> > }
> >
> > ret = some_other_init_fn();
> > if (ret)
> > goto other_err;
> >
> > kobject_uevent(kobj, KOBJ_ADD);
> > return 0;
> >
> > other_err:
> > other_clean_up_fn();
> > err_del:
> > kobject_del(kobj);
> > err_put:
> > kobject_put(kobj);
>
> IMHO, separate kobject_del() makes only sense when the sysfs
> interface must be destroyed before some other actions.
>
> I guess that we need two examples. I currently understand
> it the following way:
>
> 1. sysfs interface and the structure can be freed anytime:
>
> struct A
> {
> struct kobject kobj;
> ...
> };
>
> void fn(void)
> {
> struct A *a;
> int ret;
>
> a = kzalloc(sizeof(*a), GFP_KERNEL);
> if (!a)
> return;
>
> /*
> * Initialize structure before we make it accessible via
> * sysfs.
> */
> ret = some_init_fn();
> if (ret) {
> goto init_err;
> }
>
> ret = kobject_init_and_add(&a->kobj, ktype, NULL, "foo");
> if (ret)
> goto kobj_err;
>
> return 0;
>
> kobj_err:
> /* kobject_init() always succeds and take reference. */
> kobject_put(kobj);
> return ret;
>
> init_err:
> /* kobject was not initialized, simple free is enough */
> kfree(a);
> return ret;
> }
>
>
> 2. Structure must be registered into the subsystem before
> it can be made visible via sysfs:
>
> struct A
> {
> struct kobject kobj;
> ...
> };
>
> void fn(void)
> {
> struct A *a;
> int ret;
>
> a = kzalloc(sizeof(*a), GFP_KERNEL);
> if (!a)
> return;
>
> ret = some_init_fn();
> if (ret) {
> goto init_err;
> }
>
> /*
> * Structure is in a reasonable state and can be freed
> * via the kobject release callback.
> */
> kobject_init(&a->kobj);
>
> /*
> * Register the structure so that it can cooperate
> * with the rest of the system.
> */
> ret = register_fn(a);
> ` if (ret)
> goto register_err;
>
>
> /* Make it visible via sysfs */
> ret = kobject_add(&a->kobj, ktype, NULL, "foo");
> if (ret) {
> goto kobj_add_err;
> }
>
> /* Manipulate the structure somehow */
> ret = action_fn(a);
> if (ret)
> goto action_err;
>
> mutex_unlock(&my_mutex);
> return 0;
>
> action_err:
> /*
> * Destroy sysfs interface but the structure
> * is still needed.
> */
> kobject_del(&a->kboject);
> kobject_add_err:
> /* Make it invisible to the system. */
> unregister_fn(a);
> register_err:
> /* Release the structure unsing the kobject callback */
> kobject_put(&a->kobj);
> return;
>
> init_err:
> /*
> * Custom init failed. Kobject release callback would do
> * a double free or so. Simple free is enough.
> */
> kfree(a);
> }
>
> I would really prefer if we clearly understand where each variant makes
> sense before we start modifying the code and documentation.
Hi Petr,
Shall we work these two examples into samples/kobject/. I'm AFK now for
the rest of the week but I can do it on Monday if you don't mind me
doing it?
Cheers,
Tobin.
