* [Ocfs2-devel] [PATCH] lib/kobject: fix memory leak in error path of kset_create_and_add()
[not found] <871tbpebjw.fsf@gmail.com>
@ 2015-11-17 4:12 ` Greg Kroah-Hartman
[not found] ` <87k2pfcl73.fsf_-_@gmail.com>
0 siblings, 1 reply; 2+ messages in thread
From: Greg Kroah-Hartman @ 2015-11-17 4:12 UTC (permalink / raw)
To: Nicolai Stange
Cc: Theodore Ts'o, Andreas Dilger, Mark Fasheh, Joel Becker,
Andrew Morton, Joe Perches, linux-kernel, linux-ext4, ocfs2-devel
On Tue, Nov 17, 2015 at 01:04:19AM +0100, Nicolai Stange wrote:
> In kset_create_and_add(), the name duped into the kset's kobject by
> kset_create() gets leaked if the call to kset_register() fails.
>
> Indeed, triggering failure by invoking kset_create_and_add() with a
> duplicate name makes kmemleak reporting
>
> unreferenced object 0xffff8800b4a1f428 (size 16):
> comm "insmod", pid 682, jiffies 4294745489 (age 50.477s)
> hex dump (first 16 bytes):
> 64 75 70 6c 69 63 61 74 65 00 6b 6b 6b 6b 6b a5 duplicate.kkkkk.
> backtrace:
> [<ffffffff814380ee>] kmemleak_alloc+0x4e/0xb0
> [<ffffffff811aab01>] __kmalloc_track_caller+0x2b1/0x390
> [<ffffffff811797f1>] kstrdup+0x31/0x60
> [<ffffffff81179843>] kstrdup_const+0x23/0x30
> [<ffffffff81290254>] kvasprintf_const+0x54/0x90
> [<ffffffff81284d21>] kobject_set_name_vargs+0x21/0xa0
> [<ffffffff81284dee>] kobject_set_name+0x4e/0x70
> [<ffffffff812854f5>] kset_create_and_add+0x45/0xa0
> [...]
>
> Similar problems exist at all call sites of kset_register(), that is
> in drivers/base, fs/ext4 and in fs/ocfs2.
Yes, but those calls all succeed, so this isn't a problem in the "real"
world :)
> The deeper cause behind this are the current semantics of the kset
> initialization, creation and registration functions which differ from the
> ones of their corresponding kobject counterparts.
> Namely,
> - there is no _exported_ kset initialization function,
> - the (not exported) kset_create() creates a halfway initialized kset
> object,
> - and the kset_register() finishes a kset object's initialization but
> expects its name to already have been set at its entry.
>
> To fix this:
> - Export kset_init() and let it mimic the semantics of kobject_init().
> Especially it takes and sets a kobj_type which makes the kset object
> kset_put()able upon return.
> - Let the internal kset_create() do the complete initialization by means
> of kset_init().
> - Remove any kset initialization from kset_register(): it expects a
> readily initialized kset object now. Furthermore, let kset_register()
> take and set the kset object's name. This resembles the behaviour of
> kobject_add(). In analogy to the latter, require the caller to call
> kset_put() or kset_unregister() unconditionally, even on failure.
>
> At the call sites of kset_register():
> - Replace the open coded kset object initialization by a call to
> kset_init().
> - Remove the explicit name setting and let the kset_register() call do
> that work.
> - Replace any kfree()s by kset_put()s where appropriate.
>
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
> To the maintainers of ext4 and ocfs2: although several subsystems are
> touched, the changes come in one single patch in order to keep them bisectable.
>
>
> drivers/base/bus.c | 14 ++-----
> drivers/base/class.c | 39 ++++++++++++++-----
> fs/ext4/sysfs.c | 13 +++----
> fs/ocfs2/cluster/masklog.c | 13 ++++---
> include/linux/kobject.h | 6 ++-
> lib/kobject.c | 94 ++++++++++++++++++++++++++++------------------
> 6 files changed, 110 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 5005924..a81227c 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -900,15 +900,11 @@ int bus_register(struct bus_type *bus)
>
> BLOCKING_INIT_NOTIFIER_HEAD(&priv->bus_notifier);
>
> - retval = kobject_set_name(&priv->subsys.kobj, "%s", bus->name);
> - if (retval)
> - goto out;
> -
> - priv->subsys.kobj.kset = bus_kset;
> - priv->subsys.kobj.ktype = &bus_ktype;
> priv->drivers_autoprobe = 1;
>
> - retval = kset_register(&priv->subsys);
> + kset_init(&priv->subsys, &bus_ktype, NULL);
> + priv->subsys.kobj.kset = bus_kset;
> + retval = kset_register(&priv->subsys, bus->name, NULL);
> if (retval)
> goto out;
>
> @@ -955,10 +951,8 @@ bus_drivers_fail:
> bus_devices_fail:
> bus_remove_file(bus, &bus_attr_uevent);
> bus_uevent_fail:
> - kset_unregister(&bus->p->subsys);
> out:
> - kfree(bus->p);
> - bus->p = NULL;
> + kset_unregister(&bus->p->subsys);
> return retval;
> }
> EXPORT_SYMBOL_GPL(bus_register);
> diff --git a/drivers/base/class.c b/drivers/base/class.c
> index 71059e3..94a5b040 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -19,6 +19,7 @@
> #include <linux/slab.h>
> #include <linux/genhd.h>
> #include <linux/mutex.h>
> +#include <linux/printk.h>
> #include "base.h"
>
> #define to_class_attr(_attr) container_of(_attr, struct class_attribute, attr)
> @@ -82,6 +83,24 @@ static struct kobj_type class_ktype = {
> .child_ns_type = class_child_ns_type,
> };
>
> +static void glue_dirs_release_dummy(struct kobject *kobj)
> +{
> + /*
> + * The glue_dirs kset member of struct subsys_private is never
> + * registered and thus, never unregistered.
> + * This release function is a dummy to make kset_init() happy.
> + */
> + pr_err(
> + "class (%p): unexpected kset_put() on glue_dirs, something is broken.",
> + container_of(kobj, struct subsys_private,
> + glue_dirs.kobj)->class);
> + dump_stack();
How can this ever happen?
> +}
> +
> +static struct kobj_type glue_dirs_ktype = {
> + .release = glue_dirs_release_dummy,
> +};
> +
> /* Hotplug events for classes go to the class subsys */
> static struct kset *class_kset;
>
> @@ -175,18 +194,14 @@ int __class_register(struct class *cls, struct lock_class_key *key)
> return -ENOMEM;
> klist_init(&cp->klist_devices, klist_class_dev_get, klist_class_dev_put);
> INIT_LIST_HEAD(&cp->interfaces);
> - kset_init(&cp->glue_dirs);
> + kset_init(&cp->glue_dirs, &glue_dirs_ktype, NULL);
> __mutex_init(&cp->mutex, "subsys mutex", key);
> - error = kobject_set_name(&cp->subsys.kobj, "%s", cls->name);
> - if (error) {
> - kfree(cp);
> - return error;
> - }
>
> /* set the default /sys/dev directory for devices of this class */
> if (!cls->dev_kobj)
> cls->dev_kobj = sysfs_dev_char_kobj;
>
> + kset_init(&cp->subsys, &class_ktype, NULL);
> #if defined(CONFIG_BLOCK)
> /* let the block class directory show up in the root of sysfs */
> if (!sysfs_deprecated || cls != &block_class)
> @@ -194,13 +209,19 @@ int __class_register(struct class *cls, struct lock_class_key *key)
> #else
> cp->subsys.kobj.kset = class_kset;
> #endif
> - cp->subsys.kobj.ktype = &class_ktype;
> cp->class = cls;
> cls->p = cp;
>
> - error = kset_register(&cp->subsys);
> + error = kset_register(&cp->subsys, cls->name, NULL);
> if (error) {
> - kfree(cp);
> + /*
> + * class->release() would be called by cp->subsys'
> + * release function. Prevent this from happening in
> + * the error case by zeroing cp->class out.
> + */
> + cp->class = NULL;
> + cls->p = NULL;
> + kset_put(&cp->subsys);
That seems pretty messy, why can't we allow the release to be called? I
don't think this is really correct :(
But overall I like the patch, nice job. Any way to fix this last bit
up?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 2+ messages in thread