Re: kobject_init_and_add() confusion

[Tobin C. Harding]

On Wed, May 01, 2019 at 01:10:22PM +0200, Greg Kroah-Hartman wrote:
> 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.

One day I'll find an important issue to fix in the kernel.  At the
moment sweeping these up is good practice/learning.  If you have any
_real_ issues that need someone to turn the crank on feel free to dump
them on me :)

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

Responded just now to that email.

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

Cheers, writing good documentation is HARD.

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

On my TODO list once I get kobject usage clear in my head.

> > - Add a sample usage file under samples/kobject [optional]
> 
> Would be a good idea, so we can point people at it.

I'll combine your other email example with the extra init/error stuff
from this one and BOOM!

Thanks Greg.

	Tobin