On Wed, May 01, 2019 at 09:38:03AM +1000, 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.
s/are leaking/have the potential to leak/
Note, no one ever hits these error paths, so it isn't a big issue, and
is why no one has seen this except for the use of syzbot at times.
> 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.
You could take the example I wrote in that old email and use it, or your
version below as well.
> 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
kobject_init() can not fail except in fun ways that dumps the stack and
then keeps on going due to the failure being on the caller, not the
kobject code itself.
> * 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() 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);
>
> return ret;
> }
>
>
> Have I got this correct?
>From what I can tell, yes.
> TODO
> ----
>
> - Fix all the callsites to kobject_init_and_add()
> - Further clarify the function docstring for kobject_init_and_add() [perhaps]
More documentation, sure!
> - Add a section to Documentation/kobject.txt [optional]
That file should probably be reviewed and converted to .rst, I haven't
looked at it in years.
> - Add a sample usage file under samples/kobject [optional]
Would be a good idea, so we can point people at it.
thanks,
greg k-h
