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(-)
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();
+}
+
+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);
return error;
}
error = add_class_attrs(class_get(cls));
diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 1b57c72..03703eb 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -339,9 +339,7 @@ static struct kobj_type ext4_ktype = {
.sysfs_ops = &ext4_attr_ops,
};
-static struct kset ext4_kset = {
- .kobj = {.ktype = &ext4_ktype},
-};
+static struct kset ext4_kset;
static struct kobj_type ext4_feat_ktype = {
.default_attrs = ext4_feat_attrs,
@@ -423,11 +421,12 @@ int __init ext4_init_sysfs(void)
{
int ret;
- kobject_set_name(&ext4_kset.kobj, "ext4");
- ext4_kset.kobj.parent = fs_kobj;
- ret = kset_register(&ext4_kset);
- if (ret)
+ kset_init(&ext4_kset, &ext4_ktype, NULL);
+ ret = kset_register(&ext4_kset, "ext4", fs_kobj);
+ if (ret) {
+ kset_unregister(&ext4_kset);
return ret;
+ }
ret = kobject_init_and_add(&ext4_feat, &ext4_feat_ktype,
NULL, "features");
diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index dfe162f..70c34ec 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -164,13 +164,12 @@ static struct kobj_type mlog_ktype = {
.sysfs_ops = &mlog_attr_ops,
};
-static struct kset mlog_kset = {
- .kobj = {.ktype = &mlog_ktype},
-};
+static struct kset mlog_kset;
int mlog_sys_init(struct kset *o2cb_kset)
{
int i = 0;
+ int ret;
while (mlog_attrs[i].attr.mode) {
mlog_attr_ptrs[i] = &mlog_attrs[i].attr;
@@ -178,9 +177,13 @@ int mlog_sys_init(struct kset *o2cb_kset)
}
mlog_attr_ptrs[i] = NULL;
- kobject_set_name(&mlog_kset.kobj, "logmask");
+ kset_init(&mlog_kset, &mlog_ktype, NULL);
mlog_kset.kobj.kset = o2cb_kset;
- return kset_register(&mlog_kset);
+ ret = kset_register(&mlog_kset, "logmask", NULL);
+ if (ret)
+ kset_unregister(&mlog_kset);
+
+ return ret;
}
void mlog_sys_shutdown(void)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index e628459..d081817 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -172,8 +172,10 @@ struct kset {
const struct kset_uevent_ops *uevent_ops;
};
-extern void kset_init(struct kset *kset);
-extern int __must_check kset_register(struct kset *kset);
+extern void kset_init(struct kset *kset, struct kobj_type *ktype,
+ const struct kset_uevent_ops *uevent_ops);
+extern int __must_check kset_register(struct kset *kset, const char *name,
+ struct kobject *parent_kobj);
extern void kset_unregister(struct kset *kset);
extern struct kset * __must_check kset_create_and_add(const char *name,
const struct kset_uevent_ops *u,
diff --git a/lib/kobject.c b/lib/kobject.c
index 7cbccd2..0327157 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -761,15 +761,35 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
EXPORT_SYMBOL_GPL(kobject_create_and_add);
/**
- * kset_init - initialize a kset for use
- * @k: kset
+ * kset_init - initialize a kset structure
+ * @kset: pointer to the kset to initialize
+ * @ktype: pointer to the ktype for this kset's contained kobject.
+ * @uevent_ops: an optional struct kset_uevent_ops for the kset.
+ *
+ * This function will properly initialize a kset such that it can then
+ * be passed to the kset_register() call.
+ *
+ * Note that there are only very few circumstances where you would
+ * initialize a kset by yourself, i.e. by calling kset_init()! The
+ * normal way to get a working kset object is through
+ * kset_create_and_add().
+ *
+ * Repeating the warning from kobject_init():
+ * after this function has been called, the kset MUST be cleaned up by
+ * a call to either kset_put() or, if it has been registered through
+ * kset_register(), to kset_unregister(). You shall not free it by a
+ * call to kfree() directly in order to ensure that all of the memory
+ * is cleaned up properly.
*/
-void kset_init(struct kset *k)
+void kset_init(struct kset *kset, struct kobj_type *ktype,
+ const struct kset_uevent_ops *uevent_ops)
{
- kobject_init_internal(&k->kobj);
- INIT_LIST_HEAD(&k->list);
- spin_lock_init(&k->list_lock);
+ kobject_init(&kset->kobj, ktype);
+ INIT_LIST_HEAD(&kset->list);
+ spin_lock_init(&kset->list_lock);
+ kset->uevent_ops = uevent_ops;
}
+EXPORT_SYMBOL_GPL(kset_init);
/* default kobject attribute operations */
static ssize_t kobj_attr_show(struct kobject *kobj, struct attribute *attr,
@@ -803,17 +823,37 @@ const struct sysfs_ops kobj_sysfs_ops = {
EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
- * kset_register - initialize and add a kset.
- * @k: kset.
+ * kset_register - add a struct kset to sysfs.
+ * @k: the kset to add
+ * @name: the name for the kset
+ * @parent_kobj: the parent kobject of this kset, if any.
+ *
+ * The kset @name is set and added to the kobject hierarchy in this
+ * function. This function is for ksets what kobject_add() is for kobjects.
+ *
+ * The rules to determine the parent kobject are the same as for
+ * kobject_add().
+ *
+ * If this function returns an error, either kset_put() (preferred) or
+ * kset_unregister() must be called to properly clean up the memory
+ * associated with the object. Under no instance should the kset
+ * that is passed to this function be directly freed with a call to
+ * kfree(), that will leak memory.
+ *
+ * Note, that an "add" uevent will be created with this call.
*/
-int kset_register(struct kset *k)
+int kset_register(struct kset *k, const char *name, struct kobject *parent_kobj)
{
int err;
if (!k)
return -EINVAL;
- kset_init(k);
+ err = kobject_set_name(&k->kobj, "%s", name);
+ if (err)
+ return err;
+
+ k->kobj.parent = parent_kobj;
err = kobject_add_internal(&k->kobj);
if (err)
return err;
@@ -823,7 +863,7 @@ int kset_register(struct kset *k)
EXPORT_SYMBOL(kset_register);
/**
- * kset_unregister - remove a kset.
+ * kset_unregister - unlink a kset from hierarchy and decrement its refcount.
* @k: kset.
*/
void kset_unregister(struct kset *k)
@@ -878,9 +918,7 @@ static struct kobj_type kset_ktype = {
/**
* kset_create - create a struct kset dynamically
*
- * @name: the name for the kset
- * @uevent_ops: a struct kset_uevent_ops for the kset
- * @parent_kobj: the parent kobject of this kset, if any.
+ * @uevent_ops: an optional struct kset_uevent_ops for the kset
*
* This function creates a kset structure dynamically. This structure can
* then be registered with the system and show up in sysfs with a call to
@@ -890,32 +928,15 @@ static struct kobj_type kset_ktype = {
*
* If the kset was not able to be created, NULL will be returned.
*/
-static struct kset *kset_create(const char *name,
- const struct kset_uevent_ops *uevent_ops,
- struct kobject *parent_kobj)
+static struct kset *kset_create(const struct kset_uevent_ops *uevent_ops)
{
struct kset *kset;
- int retval;
kset = kzalloc(sizeof(*kset), GFP_KERNEL);
if (!kset)
return NULL;
- retval = kobject_set_name(&kset->kobj, "%s", name);
- if (retval) {
- kfree(kset);
- return NULL;
- }
- kset->uevent_ops = uevent_ops;
- kset->kobj.parent = parent_kobj;
-
- /*
- * The kobject of this kset will have a type of kset_ktype and belong to
- * no kset itself. That way we can properly free it when it is
- * finished being used.
- */
- kset->kobj.ktype = &kset_ktype;
- kset->kobj.kset = NULL;
+ kset_init(kset, &kset_ktype, uevent_ops);
return kset;
}
@@ -940,12 +961,13 @@ struct kset *kset_create_and_add(const char *name,
struct kset *kset;
int error;
- kset = kset_create(name, uevent_ops, parent_kobj);
+ kset = kset_create(uevent_ops);
if (!kset)
return NULL;
- error = kset_register(kset);
+
+ error = kset_register(kset, name, parent_kobj);
if (error) {
- kfree(kset);
+ kset_put(kset);
return NULL;
}
return kset;
--
2.6.3
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 <[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(-)
>
> 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
Greg Kroah-Hartman <[email protected]> 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 <[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(-)
>>
>> 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?
>
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
ktype's, just as kobject_init() doesn't. Thus, I need some dummy
kobj_ktype for the glue_dirs().
3. From the usually harsh reactions against BUG(), I conclude that
crashing the kernel due to logic errors is not a good thing to do.
-> If all of the above is true, it certainly would be better to
provide some sort of release() for the kobj_ktype.
I do agree, that a simple WARN() would have been better than the above
dummy's implementation.
On the other hand, I'm not exactly sure whether requiring a non-NULL
ktype at kset_init() is a good idea at all. Given that there is
currently only one single use of kset_init() not providing a non-trivial
kobj_ktype and that kobject_init() does require them all to be non-NULL,
I let kset_init() require this, too.
Summarizing, the options are either of
- do not use kset_init() for the glue_dirs initialization, but revert to
the open coding.
- make kset_init() accept NULL ktype's.
- introduce another initialization function, maybe
"kset_init_internal()" which accepts NULL for the ktype. I had to
export this internal function though.
- keep the above dummy release function, but make it even more clear
that it is a dummy. Furthermore, use WARN() instead of
pr_err()/dump_stack().
Just tell me which of the options to pick and I will do that.
>> +}
>> +
>> +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.
However, for bisectability, I suggest to send this as a separate patch,
if any: this would change class_register()'s behaviour and is quite
unrelated to the kset stuff.
If you do not agree, please tell me.
> But overall I like the patch, nice job. Any way to fix this last bit
> up?
Thank you very much for your feedback. I will wait for your input on the
two questions above regarding
- the dummy implementation of the glue_dirs' ktype and
- the unmessing of class_register() in a separate patch
and resend this patch accordingly then.
Thank you,
Nicolai Stange
Nicolai Stange <[email protected]> writes:
> Greg Kroah-Hartman <[email protected]> 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.
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.
Greg Kroah-Hartman <[email protected]> writes:
> 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].
>> +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?
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.
>> 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 :(
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.
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.
However, since class registration usually does not fail, there is virtually
no real world impact.
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]>
---
drivers/base/bus.c | 14 ++-----
drivers/base/class.c | 38 ++++++++++++++-----
fs/ext4/sysfs.c | 13 +++----
fs/ocfs2/cluster/masklog.c | 13 ++++---
include/linux/kobject.h | 6 ++-
lib/kobject.c | 94 ++++++++++++++++++++++++++++------------------
6 files changed, 109 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..c9683cf 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/bug.h>
#include "base.h"
#define to_class_attr(_attr) container_of(_attr, struct class_attribute, attr)
@@ -82,6 +83,23 @@ 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.
+ */
+ WARN(1,
+ "class (%p): unexpected kset_put() on glue_dirs, something is broken.",
+ container_of(kobj, struct subsys_private,
+ glue_dirs.kobj)->class);
+}
+
+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 +193,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 +208,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);
return error;
}
error = add_class_attrs(class_get(cls));
diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 1b57c72..4011dd4 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -339,9 +339,7 @@ static struct kobj_type ext4_ktype = {
.sysfs_ops = &ext4_attr_ops,
};
-static struct kset ext4_kset = {
- .kobj = {.ktype = &ext4_ktype},
-};
+static struct kset ext4_kset;
static struct kobj_type ext4_feat_ktype = {
.default_attrs = ext4_feat_attrs,
@@ -423,11 +421,12 @@ int __init ext4_init_sysfs(void)
{
int ret;
- kobject_set_name(&ext4_kset.kobj, "ext4");
- ext4_kset.kobj.parent = fs_kobj;
- ret = kset_register(&ext4_kset);
- if (ret)
+ kset_init(&ext4_kset, &ext4_ktype, NULL);
+ ret = kset_register(&ext4_kset, "ext4", fs_kobj);
+ if (ret) {
+ kset_put(&ext4_kset);
return ret;
+ }
ret = kobject_init_and_add(&ext4_feat, &ext4_feat_ktype,
NULL, "features");
diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index dfe162f..5763407 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -164,13 +164,12 @@ static struct kobj_type mlog_ktype = {
.sysfs_ops = &mlog_attr_ops,
};
-static struct kset mlog_kset = {
- .kobj = {.ktype = &mlog_ktype},
-};
+static struct kset mlog_kset;
int mlog_sys_init(struct kset *o2cb_kset)
{
int i = 0;
+ int ret;
while (mlog_attrs[i].attr.mode) {
mlog_attr_ptrs[i] = &mlog_attrs[i].attr;
@@ -178,9 +177,13 @@ int mlog_sys_init(struct kset *o2cb_kset)
}
mlog_attr_ptrs[i] = NULL;
- kobject_set_name(&mlog_kset.kobj, "logmask");
+ kset_init(&mlog_kset, &mlog_ktype, NULL);
mlog_kset.kobj.kset = o2cb_kset;
- return kset_register(&mlog_kset);
+ ret = kset_register(&mlog_kset, "logmask", NULL);
+ if (ret)
+ kset_put(&mlog_kset);
+
+ return ret;
}
void mlog_sys_shutdown(void)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index e628459..d081817 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -172,8 +172,10 @@ struct kset {
const struct kset_uevent_ops *uevent_ops;
};
-extern void kset_init(struct kset *kset);
-extern int __must_check kset_register(struct kset *kset);
+extern void kset_init(struct kset *kset, struct kobj_type *ktype,
+ const struct kset_uevent_ops *uevent_ops);
+extern int __must_check kset_register(struct kset *kset, const char *name,
+ struct kobject *parent_kobj);
extern void kset_unregister(struct kset *kset);
extern struct kset * __must_check kset_create_and_add(const char *name,
const struct kset_uevent_ops *u,
diff --git a/lib/kobject.c b/lib/kobject.c
index 7cbccd2..0327157 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -761,15 +761,35 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
EXPORT_SYMBOL_GPL(kobject_create_and_add);
/**
- * kset_init - initialize a kset for use
- * @k: kset
+ * kset_init - initialize a kset structure
+ * @kset: pointer to the kset to initialize
+ * @ktype: pointer to the ktype for this kset's contained kobject.
+ * @uevent_ops: an optional struct kset_uevent_ops for the kset.
+ *
+ * This function will properly initialize a kset such that it can then
+ * be passed to the kset_register() call.
+ *
+ * Note that there are only very few circumstances where you would
+ * initialize a kset by yourself, i.e. by calling kset_init()! The
+ * normal way to get a working kset object is through
+ * kset_create_and_add().
+ *
+ * Repeating the warning from kobject_init():
+ * after this function has been called, the kset MUST be cleaned up by
+ * a call to either kset_put() or, if it has been registered through
+ * kset_register(), to kset_unregister(). You shall not free it by a
+ * call to kfree() directly in order to ensure that all of the memory
+ * is cleaned up properly.
*/
-void kset_init(struct kset *k)
+void kset_init(struct kset *kset, struct kobj_type *ktype,
+ const struct kset_uevent_ops *uevent_ops)
{
- kobject_init_internal(&k->kobj);
- INIT_LIST_HEAD(&k->list);
- spin_lock_init(&k->list_lock);
+ kobject_init(&kset->kobj, ktype);
+ INIT_LIST_HEAD(&kset->list);
+ spin_lock_init(&kset->list_lock);
+ kset->uevent_ops = uevent_ops;
}
+EXPORT_SYMBOL_GPL(kset_init);
/* default kobject attribute operations */
static ssize_t kobj_attr_show(struct kobject *kobj, struct attribute *attr,
@@ -803,17 +823,37 @@ const struct sysfs_ops kobj_sysfs_ops = {
EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
- * kset_register - initialize and add a kset.
- * @k: kset.
+ * kset_register - add a struct kset to sysfs.
+ * @k: the kset to add
+ * @name: the name for the kset
+ * @parent_kobj: the parent kobject of this kset, if any.
+ *
+ * The kset @name is set and added to the kobject hierarchy in this
+ * function. This function is for ksets what kobject_add() is for kobjects.
+ *
+ * The rules to determine the parent kobject are the same as for
+ * kobject_add().
+ *
+ * If this function returns an error, either kset_put() (preferred) or
+ * kset_unregister() must be called to properly clean up the memory
+ * associated with the object. Under no instance should the kset
+ * that is passed to this function be directly freed with a call to
+ * kfree(), that will leak memory.
+ *
+ * Note, that an "add" uevent will be created with this call.
*/
-int kset_register(struct kset *k)
+int kset_register(struct kset *k, const char *name, struct kobject *parent_kobj)
{
int err;
if (!k)
return -EINVAL;
- kset_init(k);
+ err = kobject_set_name(&k->kobj, "%s", name);
+ if (err)
+ return err;
+
+ k->kobj.parent = parent_kobj;
err = kobject_add_internal(&k->kobj);
if (err)
return err;
@@ -823,7 +863,7 @@ int kset_register(struct kset *k)
EXPORT_SYMBOL(kset_register);
/**
- * kset_unregister - remove a kset.
+ * kset_unregister - unlink a kset from hierarchy and decrement its refcount.
* @k: kset.
*/
void kset_unregister(struct kset *k)
@@ -878,9 +918,7 @@ static struct kobj_type kset_ktype = {
/**
* kset_create - create a struct kset dynamically
*
- * @name: the name for the kset
- * @uevent_ops: a struct kset_uevent_ops for the kset
- * @parent_kobj: the parent kobject of this kset, if any.
+ * @uevent_ops: an optional struct kset_uevent_ops for the kset
*
* This function creates a kset structure dynamically. This structure can
* then be registered with the system and show up in sysfs with a call to
@@ -890,32 +928,15 @@ static struct kobj_type kset_ktype = {
*
* If the kset was not able to be created, NULL will be returned.
*/
-static struct kset *kset_create(const char *name,
- const struct kset_uevent_ops *uevent_ops,
- struct kobject *parent_kobj)
+static struct kset *kset_create(const struct kset_uevent_ops *uevent_ops)
{
struct kset *kset;
- int retval;
kset = kzalloc(sizeof(*kset), GFP_KERNEL);
if (!kset)
return NULL;
- retval = kobject_set_name(&kset->kobj, "%s", name);
- if (retval) {
- kfree(kset);
- return NULL;
- }
- kset->uevent_ops = uevent_ops;
- kset->kobj.parent = parent_kobj;
-
- /*
- * The kobject of this kset will have a type of kset_ktype and belong to
- * no kset itself. That way we can properly free it when it is
- * finished being used.
- */
- kset->kobj.ktype = &kset_ktype;
- kset->kobj.kset = NULL;
+ kset_init(kset, &kset_ktype, uevent_ops);
return kset;
}
@@ -940,12 +961,13 @@ struct kset *kset_create_and_add(const char *name,
struct kset *kset;
int error;
- kset = kset_create(name, uevent_ops, parent_kobj);
+ kset = kset_create(uevent_ops);
if (!kset)
return NULL;
- error = kset_register(kset);
+
+ error = kset_register(kset, name, parent_kobj);
if (error) {
- kfree(kset);
+ kset_put(kset);
return NULL;
}
return kset;
--
2.6.3
If a class attribute's sysfs creation fails at the very end of
__class_register(), the kset object already registered for the class itself
is not unregistered again.
This is not consistent with the other possible failures of
__class_register() for which nothing is left over to be manually released
by the caller.
Since class attribute creation does not fail in the real world, the
impact is minimal -- this patch is a cosmetic one.
Make __class_register() follow all-or-nothing semantics, i.e. unregister
the already registered cp->subsys kset object on attribute creation
failure.
Signed-off-by: Nicolai Stange <[email protected]>
---
drivers/base/class.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/base/class.c b/drivers/base/class.c
index c9683cf..fc663d0 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -225,6 +225,12 @@ int __class_register(struct class *cls, struct lock_class_key *key)
}
error = add_class_attrs(class_get(cls));
class_put(cls);
+ if (error) {
+ /* as above, clear cp->class on error */
+ cp->class = NULL;
+ cls->p = NULL;
+ kset_put(&cp->subsys);
+ }
return error;
}
EXPORT_SYMBOL_GPL(__class_register);
--
2.6.3
Currently, upon failure, __class_register() does not call the class_release
member of the struct class object handed over to it. This leads to
potentially duplicated cleanup code at the call sites: once for the error
path handling a failed __class_register() and once for an orderly class
deregistration.
Note however, that the impact is _very_ low: currently there are only five
clients of __class_register() passing class objects with a non-trivial
class_release member.
This patch is more about consolidating the __class_register() interface as
well as slightly cleaning up the latter's implementation, i.e. cosmetic in
nature.
The call sites of __class_register handing in a non-NULL class_release
member are:
- drivers/base/class.c
- drivers/block/osdblk.c
- drivers/block/pktcdvd.c
- drivers/media/usb/pvrusb2/pvrusb2-sysfs.c
- drivers/pcmcia/cs.c
- drivers/isdn/mISDN/core.c
The first four just invoke a kfree() on the struct class object on failure
of __class_register() as well as in their class_release functions.
The next to last one from the PCMCIA subsystem is special as it completes
a condition variable in its class releaser pcmcia_release_socket_class().
This condition variable is waited on exclusively right after a call to
class_unregister() in the module_exit handler. Despite the fact that
the module_exit handler gets never executed if the __class_register()
invocation from the module init handler fails, there would be no harm if
it would.
Finally, the last candidate, the mISDN core, passes an empty class_release
implementation to __class_register().
Make __class_register() invoke the class_release member of the given
struct class object upon failure.
Adapt the callers to not do any cleanup as part of their error handling
if already handled by their respective class releaser.
Signed-off-by: Nicolai Stange <[email protected]>
---
drivers/base/class.c | 14 ++++----------
drivers/block/osdblk.c | 1 -
drivers/block/pktcdvd.c | 1 -
drivers/media/usb/pvrusb2/pvrusb2-sysfs.c | 1 -
4 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/base/class.c b/drivers/base/class.c
index fc663d0..6f89ee5 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -189,8 +189,11 @@ int __class_register(struct class *cls, struct lock_class_key *key)
pr_debug("device class '%s': registering\n", cls->name);
cp = kzalloc(sizeof(*cp), GFP_KERNEL);
- if (!cp)
+ if (!cp) {
+ if (cls->class_release)
+ cls->class_release(cls);
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, &glue_dirs_ktype, NULL);
@@ -213,12 +216,6 @@ int __class_register(struct class *cls, struct lock_class_key *key)
error = kset_register(&cp->subsys, cls->name, NULL);
if (error) {
- /*
- * 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);
return error;
@@ -226,8 +223,6 @@ int __class_register(struct class *cls, struct lock_class_key *key)
error = add_class_attrs(class_get(cls));
class_put(cls);
if (error) {
- /* as above, clear cp->class on error */
- cp->class = NULL;
cls->p = NULL;
kset_put(&cp->subsys);
}
@@ -285,7 +280,6 @@ struct class *__class_create(struct module *owner, const char *name,
return cls;
error:
- kfree(cls);
return ERR_PTR(retval);
}
EXPORT_SYMBOL_GPL(__class_create);
diff --git a/drivers/block/osdblk.c b/drivers/block/osdblk.c
index 1b709a4..59d3fa3 100644
--- a/drivers/block/osdblk.c
+++ b/drivers/block/osdblk.c
@@ -662,7 +662,6 @@ static int osdblk_sysfs_init(void)
ret = class_register(class_osdblk);
if (ret) {
- kfree(class_osdblk);
class_osdblk = NULL;
printk(PFX "failed to create class osdblk\n");
return ret;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index d06c62e..34693ac 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -429,7 +429,6 @@ static int pkt_sysfs_init(void)
class_pktcdvd->class_attrs = class_pktcdvd_attrs;
ret = class_register(class_pktcdvd);
if (ret) {
- kfree(class_pktcdvd);
class_pktcdvd = NULL;
pr_err("failed to create class pktcdvd\n");
return ret;
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-sysfs.c b/drivers/media/usb/pvrusb2/pvrusb2-sysfs.c
index 06fe63c..de7aca2 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-sysfs.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-sysfs.c
@@ -797,7 +797,6 @@ struct pvr2_sysfs_class *pvr2_sysfs_class_create(void)
if (class_register(&clp->class)) {
pvr2_sysfs_trace(
"Registration failed for pvr2_sysfs_class id=%p",clp);
- kfree(clp);
clp = NULL;
}
return clp;
--
2.6.3