| 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
| ||
|
Message-ID: <20190502090647.GB25154@kroah.com>
Date: Thu, 2 May 2019 11:06:47 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Petr Mladek <pmladek@...e.com>
Cc: "Tobin C. Harding" <me@...in.cc>,
"Rafael J. Wysocki" <rafael@...nel.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 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
Powered by blists - more mailing lists