Re: kobject_init_and_add() confusion

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 10:34:12AM +0200, Petr Mladek wrote:
> On Wed 2019-05-01 09:38:03, 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.
> > 
> > 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;
> > 		}
> 
> It is strange to make the structure visible in sysfs before
> we initialize it.

Yes, that is not a good patern, some_init_fn() should happen first.

> > 	        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);
> 
> IMHO, separate kobject_del() makes only sense when the sysfs
> interface must be destroyed before some other actions.

Yes, kobject_del() should not be used unless you really know what you
are doing.

> I guess that we need two examples. I currently understand
> it the following way:
> 
> 1. sysfs interface and the structure can be freed anytime:
> 
>    	struct A
> 	{
> 		struct kobject kobj;
> 		...
> 	};
> 
> 	void fn(void)
> 	{
> 		struct A *a;
> 		int ret;
> 
> 		a = kzalloc(sizeof(*a), GFP_KERNEL);
> 		if (!a)
> 			return;
> 
> 		/*
> 		 * Initialize structure before we make it accessible via
> 		 * sysfs.
> 		 */
> 		ret = some_init_fn();
> 		if (ret) {
> 			goto init_err;
> 		}
> 
> 		ret = kobject_init_and_add(&a->kobj, ktype, NULL, "foo");
> 		if (ret)
> 			goto kobj_err;
> 
> 		return 0;
> 
> 	kobj_err:


need to unwind some_init_fn() here too.

> 		/* kobject_init() always succeds and take reference. */
> 		kobject_put(kobj);
> 		return ret;
> 
> 	init_err:
> 		/* kobject was not initialized, simple free is enough */
> 		kfree(a);
> 		return ret;
> 	}


Yes.

> 2. Structure must be registered into the subsystem before
>    it can be made visible via sysfs:
> 
>    	struct A
> 	{
> 		struct kobject kobj;
> 		...
> 	};
> 
> 	void fn(void)
> 	{
> 		struct A *a;
> 		int ret;
> 
> 		a = kzalloc(sizeof(*a), GFP_KERNEL);
> 		if (!a)
> 			return;
> 
> 		ret = some_init_fn();
> 		if (ret) {
> 			goto init_err;
> 		}
> 
> 		/*
> 		 * Structure is in a reasonable state and can be freed
> 		 * via the kobject release callback.
> 		 */
> 		kobject_init(&a->kobj);
> 
> 		/*
> 		 * Register the structure so that it can cooperate
> 		 * with the rest of the system.
> 		 */
> 		ret = register_fn(a);
> `		if (ret)
> 			goto register_err;
> 
> 
> 		/* Make it visible via sysfs */
> 		ret = kobject_add(&a->kobj, ktype, NULL, "foo");
> 		if (ret) {
> 			goto kobj_add_err;
> 		}
> 
> 		/* Manipulate the structure somehow */
> 		ret = action_fn(a);
> 		if (ret)
> 			goto action_err;
> 
> 		mutex_unlock(&my_mutex);
> 		return 0;
> 
> 	action_err:
> 		/*
> 		 * Destroy sysfs interface but the structure
> 		 * is still needed.
> 		 */
> 		kobject_del(&a->kboject);
> 	kobject_add_err:
> 		/* Make it invisible to the system. */
> 		unregister_fn(a);
> 	register_err:
> 		/* Release the structure unsing the kobject callback */
> 		kobject_put(&a->kobj);
> 		return;
> 
> 	init_err:
> 		/*
> 		 * Custom init failed. Kobject release callback would do
> 		 * a double free or so. Simple free is enough.
> 		 */
> 		 kfree(a);
> 	}
> 
> I would really prefer if we clearly understand where each variant makes
> sense before we start modifying the code and documentation.

The second variant is much more rare (or at least it should be), but
your example is a good one.

thanks,

greg k-h