Messages in this thread | | | Date | Thu, 2 May 2019 07:44:23 +1000 | From | "Tobin C. Harding" <> | Subject | Re: kobject_init_and_add() confusion |
| |
On Wed, May 01, 2019 at 09:54:16AM +0200, Rafael J. Wysocki wrote: > On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@tobin.cc> 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?
You are correct, my mistake.
> > * 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.
Thanks for this explanation. Points noted.
> > > 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.
During docs fixes I'll try to work this in.
Thanks, Tobin.
| |