Date:   Thu, 2 May 2019 10:34:12 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     "Tobin C. Harding" <me@...in.cc>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Miroslav Benes <mbenes@...e.cz>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: kobject_init_and_add() confusion

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.

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

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:
		/* 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;
	}


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.

Best Regards,
Petr

Powered by blists - more mailing lists