linux-kernel - Re: 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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 May 2019 13:10:22 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     "Tobin C. Harding" <me@...in.cc>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        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: Re: kobject_init_and_add() confusion

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.

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

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

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

> - Add a sample usage file under samples/kobject [optional]

Would be a good idea, so we can point people at it.

thanks,

greg k-h