linux-kernel - kobject_init_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190430233803.GB10777@eros.localdomain>
Date:   Wed, 1 May 2019 09:38:03 +1000
From:   "Tobin C. Harding" <me@...in.cc>
To:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Petr Mladek <pmladek@...e.com>,
        Miroslav Benes <mbenes@...e.cz>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: kobject_init_and_add() confusion

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() 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?

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]

Thanks,
Tobin.