Re: [PATCH] lib/kobject: fix memory leak in error path of kset_create_and_add()
Nicolai Stange writes:
> Greg Kroah-Hartman writes:
>
>> On Tue, Nov 17, 2015 at 01:04:19AM +0100, Nicolai Stange wrote:
>>> +}
>>> +
>>> +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 :(
>
> I don't know whether it is actually correct, but before the introduction
> of this patch, the class->release() would not have been called on error
> either. I did not want to change that behaviour and thus, I have put
> this "messy" and admittedly ugly clear-the-class-pointer code into the
> error handling above.
>
> I will check whether calling class->release() on error would be a
> problem for the callers of class_register() (there are 64 of them). If
> it turns out not to be one, I could simply remove the above class
> pointer purging.
The very first candidate I've looked at, __class_create() from
drivers/base/class.c, relies on class_register() not to call
class->release() on error: upon a non-zero return from
__class_register() it explicitly kfree()s the struct class
instance. Together with the kfree() in class_create_release(), this
would result in a double free. Thus, as it stands,
class_create_release() must not get called upon encountering an error in
__class_register() and the messy clearance of cp->class before the call
to kset_put() is necessary.
The current semantics of __class_register() seem to be:
- if the call succeeds, the registered class' release function will
eventually get called by the kobject system.
- if it doesn't succeed, the registered class' release function won't
ever be called: clean up after yourself.
One could change this behaviour and indeed it might make sense. But in
my opinion, this should be done in a different patch. After all, there
are ~64 call sites involved which would have to get changed accordingly.
As a sidenote, note that __class_register()'s error behaviour is a
little bit inconsistent: if the final add_class_attrs() call fails, the
cp->subsys won't get deregistered by __class_register() itsef. Upon
encountering a non-zero return value from __class_register(), a caller
would not be able to decide whether kset_register() or add_class_attrs()
failed and thus, it can't tell whether a class_unregister() is
necessary/allowed or not. I think the place to fix this should be a
separate patch, too. It would be a straight forward one though:
add_class_attrs() has got all-or-nothing semantics already and thus,
furnishing __class_register() with similiar behaviour is easy.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] lib/kobject: fix memory leak in error path of kset_create_and_add()
Greg Kroah-Hartman writes:
> On Tue, Nov 17, 2015 at 01:04:19AM +0100, Nicolai Stange wrote:
>> 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 :)
>
No, it is not an issue at runtime. It's more about puzzling code readers
like me.
>> 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>> #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?
>
Actually it can't.
To quote you from Documentation/kobject.txt:
Do not try to get rid of this warning by providing an "empty" release
function; you will be mocked mercilessly by the kobject maintainer if
you attempt this.
Well, the glue_dirs_release_dummy() is quite empty, but my reasoning was
like this:
1. It is a good thing to initialize glue_dirs by kset_init().
2. In the current patch's version, kset_init() does not allow for NULL
Re: [PATCH] lib/kobject: fix memory leak in error path of kset_create_and_add()
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 0x8800b4a1f428 (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.k.
> backtrace:
> [] kmemleak_alloc+0x4e/0xb0
> [] __kmalloc_track_caller+0x2b1/0x390
> [] kstrdup+0x31/0x60
> [] kstrdup_const+0x23/0x30
> [] kvasprintf_const+0x54/0x90
> [] kobject_set_name_vargs+0x21/0xa0
> [] kobject_set_name+0x4e/0x70
> [] 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
> ---
> 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
> #include
> #include
> +#include
> #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.",
> +
[PATCH] lib/kobject: fix memory leak in error path of kset_create_and_add()
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 0x8800b4a1f428 (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.k.
backtrace:
[] kmemleak_alloc+0x4e/0xb0
[] __kmalloc_track_caller+0x2b1/0x390
[] kstrdup+0x31/0x60
[] kstrdup_const+0x23/0x30
[] kvasprintf_const+0x54/0x90
[] kobject_set_name_vargs+0x21/0xa0
[] kobject_set_name+0x4e/0x70
[] 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.
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
---
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
#include
#include
+#include
#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();
+}
+
+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 __
