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