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

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 <[email protected]>

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

drivers/base/bus.c +4 -10

index 5005924..a81227c 100644
@@ -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);

drivers/base/class.c +3 -0

index 71059e3..94a5b040 100644
@@ -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)
+{

drivers/base/class.c +9 -8

+}
+
+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);

Quoted 2 lines · Nicolai Stange

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 :)

Quoted 94 lines

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 <[email protected]>

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

drivers/base/bus.c +4 -10

index 5005924..a81227c 100644
@@ -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);

drivers/base/class.c +1 -0

index 71059e3..94a5b040 100644
@@ -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,
 };
 

drivers/base/class.c static struct kobj_type class_ktype = { · line 97

Quoted 12 lines

drivers/base/class.c +12 -0

+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?

Quoted 38 lines

drivers/base/class.c +8 -7

+}
+
+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;
 

drivers/base/class.c int __class_register(struct class *cls, struct lock_class_key *key) · line 224

Quoted 12 lines

drivers/base/class.c +9 -2

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

Quoted 39 lines · Greg Kroah-Hartman

Quoted 38 lines · Nicolai Stange

drivers/base/class.c +8 -7

+}
+
+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;
 

Quoted 15 lines

drivers/base/class.c int __class_register(struct class *cls, struct lock_class_key *key) · line 224

Quoted 12 lines

drivers/base/class.c +9 -2

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

On Tue, Nov 17, 2015 at 01:04:19AM +0100, Nicolai Stange wrote:
Yes, but those calls all succeed, so this isn't a problem in the "real" world :)

Quoted 6 lines

drivers/base/class.c +6 -0

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

drivers/base/class.c static struct kobj_type class_ktype = { · line 97

Quoted 6 lines

drivers/base/class.c +6 -0

+	 */
+	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?

Quoted 5 lines

drivers/base/class.c +3 -1

 	if (error) {
-		kfree(cp);
+		/*
+		 * class->release() would be called by cp->subsys'
+		 * release function. Prevent this from happening in

drivers/base/class.c int __class_register(struct class *cls, struct lock_class_key *key) · line 224

Quoted 5 lines

drivers/base/class.c +5 -0

+		 * 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 :(

In order to have something to base further discussion on, here comes the second version.

In addition to some changes to the inital patch (now [1/3]), two additional ones have been introduced.

The three patches depend on one another in sequence.

For a detailed changelist, see the end of this mail.

Quoted 3 lines · Greg Kroah-Hartman

On Tue, Nov 17, 2015 at 01:04:19AM +0100, Nicolai Stange wrote:
Yes, but those calls all succeed, so this isn't a problem in the "real" world :)

I added a word about non-impact in the commit message of [1/3].

Quoted 6 lines

Quoted 6 lines

drivers/base/class.c +6 -0

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

Quoted 8 lines

drivers/base/class.c static struct kobj_type class_ktype = { · line 97

Quoted 6 lines

drivers/base/class.c +6 -0

+	 */
+	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?

Not at all. I'm not sure I understand you correctly here.
Do you strictly want to abandon the dummy releaser, or more specifically, the dummy kobj_type?
For the moment, I turned the pr_err()/dump_stack() into a WARN() for the sake of better style.

Quoted 5 lines

Quoted 5 lines

drivers/base/class.c +3 -1

 	if (error) {
-		kfree(cp);
+		/*
+		 * class->release() would be called by cp->subsys'
+		 * release function. Prevent this from happening in

Quoted 8 lines

drivers/base/class.c int __class_register(struct class *cls, struct lock_class_key *key) · line 224

Quoted 5 lines

drivers/base/class.c +5 -0

+		 * 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 :(

At the moment, it is necessary. I've added [3/3] for the case that you want cls->class_release() to get called from __class_register() on error. In [3/3] you will also find the (few) callers expecting the behaviour as it currently is.

Detailed changes from initial version to v2:
[1/3] lib/kobject: fix memory leak in error path of kset_create_and_add() This one is the original patch with a few changes:
- added a word of non-impact to commit message
- use WARN() instead of open coded pr_error()/dump_stack() pair in struct class' glue_dirs' dummy class releaser.
- follow my own advice in the docstring of kset_register() and use kset_put() instead of kset_unregister() on error of kset_register() in ext4's ext4_init_sysfs() and ocfs2's mlog_sys_init().

[2/3] drivers/base/class: __class_register(): make error behaviour consistent This one is new and quite unrelated to the original patch. Following the sidenote made in my last mail, it makes __class_register() properly clean up after itself on error.

[3/3] drivers/base/class: __class_register(): invoke class' releaser on failure This one is new.
It addresses Greg K-H's comment on the initial version about the messiness of avoiding to call class->class_release() from __class_register() on error. Given the fact that I had to introduce an explicit cls->class_release() in the early part when there is no kset object available yet, I'm by no means sure that it is much less messy now and that this patch is worth the trouble.
-> It is up to you to judge.
As there are enough people bothered already, I did not CC the people suggested for this one by get_maintainer.pl: all of them are maintainers of external subsystems and probably not particularly interested in __class_register() and friends. I will do this as soon as
a decision for this patch has been made, perhaps in a separate thread.