Messages in this thread | | | Date | Wed, 1 May 2019 13:10:22 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: kobject_init_and_add() confusion |
| |
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
| |