Re: kobject_init_and_add() confusion

[Rafael J. Wysocki]

On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@xxxxxxxx> 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;
>                 }
>
>                 ret = some_init_fn();
>                 if (ret) {
>                         /*
>                          * We need to wind back kobject_add() AND kobject_put().

kobject_add() and kobject_init() I suppose?

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

So actually, if you look at kobject_cleanup(), it calls kobject_del()
if kobj->state_in_sysfs is set.

Now, if you look at kobject_add_internal(), it sets
kobj->state_in_sysfs when about to return 0 (success).

Therefore calling kobject_put() without the preceding kobject_del() is
not a bug technically, even though it will trigger the "auto cleanup
kobject_del" message with debug enabled.

>                         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);
>
>                 return ret;
>         }
>
>
> Have I got this correct?
>
> TODO
> ----
>
> - Fix all the callsites to kobject_init_and_add()
> - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> - Add a section to Documentation/kobject.txt [optional]
> - Add a sample usage file under samples/kobject [optional]

The plan sounds good to me, but there is one thing to note IMO:
kobject_cleanup() invokes the ->release() callback for the ktype, so
these callbacks need to be able to cope with kobjects after a failing
kobject_add() which may not be entirely obvious to developers
introducing them ATM.

Thanks,
Rafael