[Ocfs2-devel] [PATCH] lib/kobject: fix memory leak in error path of kset_create_and_add()

[Ocfs2-devel] [PATCH] lib/kobject: fix memory leak in error path of kset_create_and_add()

[Greg Kroah-Hartman]

On Tue, Nov 17, 2015 at 01:04:19AM +0100, Nicolai Stange wrote:
> In kset_create_and_add(), the name duped into the kset's kobject by
> kset_create() gets leaked if the call to kset_register() fails.
> 
> Indeed, triggering failure by invoking kset_create_and_add() with a
> duplicate name makes kmemleak reporting
> 
>   unreferenced object 0xffff8800b4a1f428 (size 16):
>     comm "insmod", pid 682, jiffies 4294745489 (age 50.477s)
>     hex dump (first 16 bytes):
>       64 75 70 6c 69 63 61 74 65 00 6b 6b 6b 6b 6b a5  duplicate.kkkkk.
>     backtrace:
>       [<ffffffff814380ee>] kmemleak_alloc+0x4e/0xb0
>       [<ffffffff811aab01>] __kmalloc_track_caller+0x2b1/0x390
>       [<ffffffff811797f1>] kstrdup+0x31/0x60
>       [<ffffffff81179843>] kstrdup_const+0x23/0x30
>       [<ffffffff81290254>] kvasprintf_const+0x54/0x90
>       [<ffffffff81284d21>] kobject_set_name_vargs+0x21/0xa0
>       [<ffffffff81284dee>] kobject_set_name+0x4e/0x70
>       [<ffffffff812854f5>] kset_create_and_add+0x45/0xa0
>       [...]
> 
> Similar problems exist at all call sites of kset_register(), that is
> in drivers/base, fs/ext4 and in fs/ocfs2.

Yes, but those calls all succeed, so this isn't a problem in the "real"
world :)

> The deeper cause behind this are the current semantics of the kset
> initialization, creation and registration functions which differ from the
> ones of their corresponding kobject counterparts.
> Namely,
> - there is no _exported_ kset initialization function,
> - the (not exported) kset_create() creates a halfway initialized kset
>   object,
> - and the kset_register() finishes a kset object's initialization but
>   expects its name to already have been set at its entry.
> 
> To fix this:
> - Export kset_init() and let it mimic the semantics of kobject_init().
>   Especially it takes and sets a kobj_type which makes the kset object
>   kset_put()able upon return.
> - Let the internal kset_create() do the complete initialization by means
>   of kset_init().
> - Remove any kset initialization from kset_register(): it expects a
>   readily initialized kset object now. Furthermore, let kset_register()
>   take and set the kset object's name. This resembles the behaviour of
>   kobject_add(). In analogy to the latter, require the caller to call
>   kset_put() or kset_unregister() unconditionally, even on failure.
> 
> At the call sites of kset_register():
> - Replace the open coded kset object initialization by a call to
>   kset_init().
> - Remove the explicit name setting and let the kset_register() call do
>   that work.
> - Replace any kfree()s by kset_put()s where appropriate.
> 
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
> To the maintainers of ext4 and ocfs2: although several subsystems are
> touched, the changes come in one single patch in order to keep them bisectable.
> 
> 
>  drivers/base/bus.c         | 14 ++-----
>  drivers/base/class.c       | 39 ++++++++++++++-----
>  fs/ext4/sysfs.c            | 13 +++----
>  fs/ocfs2/cluster/masklog.c | 13 ++++---
>  include/linux/kobject.h    |  6 ++-
>  lib/kobject.c              | 94 ++++++++++++++++++++++++++++------------------
>  6 files changed, 110 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 5005924..a81227c 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -900,15 +900,11 @@ int bus_register(struct bus_type *bus)
>  
>  	BLOCKING_INIT_NOTIFIER_HEAD(&priv->bus_notifier);
>  
> -	retval = kobject_set_name(&priv->subsys.kobj, "%s", bus->name);
> -	if (retval)
> -		goto out;
> -
> -	priv->subsys.kobj.kset = bus_kset;
> -	priv->subsys.kobj.ktype = &bus_ktype;
>  	priv->drivers_autoprobe = 1;
>  
> -	retval = kset_register(&priv->subsys);
> +	kset_init(&priv->subsys, &bus_ktype, NULL);
> +	priv->subsys.kobj.kset = bus_kset;
> +	retval = kset_register(&priv->subsys, bus->name, NULL);
>  	if (retval)
>  		goto out;
>  
> @@ -955,10 +951,8 @@ bus_drivers_fail:
>  bus_devices_fail:
>  	bus_remove_file(bus, &bus_attr_uevent);
>  bus_uevent_fail:
> -	kset_unregister(&bus->p->subsys);
>  out:
> -	kfree(bus->p);
> -	bus->p = NULL;
> +	kset_unregister(&bus->p->subsys);
>  	return retval;
>  }
>  EXPORT_SYMBOL_GPL(bus_register);
> diff --git a/drivers/base/class.c b/drivers/base/class.c
> index 71059e3..94a5b040 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -19,6 +19,7 @@
>  #include <linux/slab.h>
>  #include <linux/genhd.h>
>  #include <linux/mutex.h>
> +#include <linux/printk.h>
>  #include "base.h"
>  
>  #define to_class_attr(_attr) container_of(_attr, struct class_attribute, attr)
> @@ -82,6 +83,24 @@ static struct kobj_type class_ktype = {
>  	.child_ns_type	= class_child_ns_type,
>  };
>  
> +static void glue_dirs_release_dummy(struct kobject *kobj)
> +{
> +	/*
> +	 * The glue_dirs kset member of struct subsys_private is never
> +	 * registered and thus, never unregistered.
> +	 * This release function is a dummy to make kset_init() happy.
> +	 */
> +	pr_err(
> +	"class (%p): unexpected kset_put() on glue_dirs, something is broken.",
> +		container_of(kobj, struct subsys_private,
> +				glue_dirs.kobj)->class);
> +	dump_stack();

How can this ever happen?

> +}
> +
> +static struct kobj_type glue_dirs_ktype = {
> +	.release = glue_dirs_release_dummy,
> +};
> +
>  /* Hotplug events for classes go to the class subsys */
>  static struct kset *class_kset;
>  
> @@ -175,18 +194,14 @@ int __class_register(struct class *cls, struct lock_class_key *key)
>  		return -ENOMEM;
>  	klist_init(&cp->klist_devices, klist_class_dev_get, klist_class_dev_put);
>  	INIT_LIST_HEAD(&cp->interfaces);
> -	kset_init(&cp->glue_dirs);
> +	kset_init(&cp->glue_dirs, &glue_dirs_ktype, NULL);
>  	__mutex_init(&cp->mutex, "subsys mutex", key);
> -	error = kobject_set_name(&cp->subsys.kobj, "%s", cls->name);
> -	if (error) {
> -		kfree(cp);
> -		return error;
> -	}
>  
>  	/* set the default /sys/dev directory for devices of this class */
>  	if (!cls->dev_kobj)
>  		cls->dev_kobj = sysfs_dev_char_kobj;
>  
> +	kset_init(&cp->subsys, &class_ktype, NULL);
>  #if defined(CONFIG_BLOCK)
>  	/* let the block class directory show up in the root of sysfs */
>  	if (!sysfs_deprecated || cls != &block_class)
> @@ -194,13 +209,19 @@ int __class_register(struct class *cls, struct lock_class_key *key)
>  #else
>  	cp->subsys.kobj.kset = class_kset;
>  #endif
> -	cp->subsys.kobj.ktype = &class_ktype;
>  	cp->class = cls;
>  	cls->p = cp;
>  
> -	error = kset_register(&cp->subsys);
> +	error = kset_register(&cp->subsys, cls->name, NULL);
>  	if (error) {
> -		kfree(cp);
> +		/*
> +		 * class->release() would be called by cp->subsys'
> +		 * release function. Prevent this from happening in
> +		 * the error case by zeroing cp->class out.
> +		 */
> +		cp->class = NULL;
> +		cls->p = NULL;
> +		kset_put(&cp->subsys);

That seems pretty messy, why can't we allow the release to be called?  I
don't think this is really correct :(

But overall I like the patch, nice job.  Any way to fix this last bit
up?

thanks,

greg k-h

[Ocfs2-devel] [PATCH v2] kset- and class-registration cleanups

[Nicolai Stange]

Nicolai Stange <nicstange@gmail.com> writes:

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

Hi Greg,

I'd like to ask whether this is still in your review queue or did it
slip through?

Should I resend?

Thank you,

Nicolai

> 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 <gregkh@linuxfoundation.org> 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.