Possible memory leaks on driver core error paths

In both of the 'goto out' cases, priv has already
been allocated and assigned to bus->p. We reset
bus->p to NULL, but neglect to free bus->p

Signed-off-by Patrick Pannuto <[email protected]>

---
 drivers/base/bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/bus.c +0 -0

index 12eec3f..eb1b7fa 100644

There is a subtle memory leak in driver core error path. Consider the simplified view of bus_register (drivers/base/bus.c):

priv = kzalloc...
kobject_set_name(&priv->subsys.kobj,...) <== allocate in priv->subsys.kobj.name if kset_register(&priv->subsys) FAILS:

(1) -- need to free name string here!
goto out
else
keep going, assume we fail later...

err_path_after_kset_register:
kset_unregister(&priv->subsys) <== calls kobject_put(&priv->subsys->kobj),
which is the last reference, which leads to kobject_cleanup, which frees the name field BUT DOES NOT SET IT TO NULL
out:

	(2) Other place we could free name
	kfree(bus->p) // bus->p = priv
	...

Thus, we cannot insert the free'ing of name on the error path (2) since it will be indeterminite if we need to free the name string or not. kset_register cannot (and should not) free the name string if it fails since the caller may have other ideas, leaving only (1) as a viable location to free the name string.

A walk-through of the failure path that will trigger this leak:

bus_register(struct bus_type *bus) {
	...
	priv = kzalloc...
	kobject_set_name(&priv->subsys.kobj,... {
		kobject_set_name_vargs(kobj,... {
			kobj->name = kvasprintf... <== ALLOCATE INTO &priv->subsys.kobj.name

	priv->subsys.kobj.kset = bus_kset
	priv->subsys.kobj.ktype = &bus_ktype

	retval = kset_register(&priv->subsys)...
	if (retval) YES
		goto out;

	...
out:
	kfree(bus->p);
	bus->p = NULL;
	return retval;
}

kset_register(struct kset *k) { // k = &priv->subsys

	kset_init(k) {
		kobject_init_internal(&k->kobj) {
			kref_init(&kobj->kref) {
				kref->refcount = 1

	err = kobject_add_internal(&k->kobj)...
	if (err) YES
		return err;
	...
}

kobject_add_internal(struct kobject *kobj) { // kobj = &priv->subsys.kobj

	...
	if (kobj->kset) YES {
		kobj_kset_join(kobj) {
			kset_get(kobj->kset) <== bus_kset
	...
	error = create_dir(kobj);
	if (error) YES {
		kobj_kset_leave(kobj) {
			kset_put(kobj->kset) <== bus_kset
		... (release parent and make KERN_ERR noise)
	}
	return error
}

Signed-off-by Patrick Pannuto <[email protected]>

---
 drivers/base/bus.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

drivers/base/bus.c +0 -0

index eb1b7fa..fea1774 100644

Quoted 6 lines · Patrick Pannuto

There is a subtle memory leak in driver core error path. Consider the simplified view of bus_register (drivers/base/bus.c):

priv = kzalloc...
kobject_set_name(&priv->subsys.kobj,...) <== allocate in priv->subsys.kobj.name if kset_register(&priv->subsys) FAILS:

Why would this fail?

drivers/base/bus.c int bus_register(struct bus_type *bus) · line 904

Quoted 5 lines

drivers/base/bus.c +2 -1

 	retval = kset_register(&priv->subsys);
-	if (retval)
+	if (retval) {
+		kfree(priv->subsys.kobj.name);

I don't think we want to bury the logic of how kobject names are handled up here in the bus code, right? Shouldn't the subsys kobject name be able to be cleaned up on its own somehow instead?

Quoted 10 lines

drivers/base/bus.c int bus_register(struct bus_type *bus) · line 904

Quoted 5 lines

drivers/base/bus.c +2 -1

 	retval = kset_register(&priv->subsys);
-	if (retval)
+	if (retval) {
+		kfree(priv->subsys.kobj.name);

I don't think we want to bury the logic of how kobject names are handled up here in the bus code, right? Shouldn't the subsys kobject name be able to be cleaned up on its own somehow instead?

So, my first instinct was to use kobject_cleanup, but a few lines above:

	priv->subsys.kobj.ktype = &bus_ktype;

and bus_ktype's definition, with the notable absence of a release method:

static struct kobj_type bus_ktype = {
	.sysfs_ops	= &bus_sysfs_ops,
};

which in kobject_cleanup would yield:

	struct kobj_type *t = get_ktype(kobj);

	if (t && !t->release)
		pr_debug("kobject: '%s' (%p): does not have a release() "
			 "function, it is broken and must be fixed.\n",
			 kobject_name(kobj), kobj);

(if I understand everything correctly)

Quoted 23 lines

Quoted 10 lines

drivers/base/bus.c int bus_register(struct bus_type *bus) · line 904

Quoted 5 lines

drivers/base/bus.c +2 -1

 	retval = kset_register(&priv->subsys);
-	if (retval)
+	if (retval) {
+		kfree(priv->subsys.kobj.name);

I don't think we want to bury the logic of how kobject names are handled up here in the bus code, right? Shouldn't the subsys kobject name be able to be cleaned up on its own somehow instead?

So, my first instinct was to use kobject_cleanup, but a few lines above:

	priv->subsys.kobj.ktype = &bus_ktype;

and bus_ktype's definition, with the notable absence of a release method:

static struct kobj_type bus_ktype = {
	.sysfs_ops	= &bus_sysfs_ops,
};

which in kobject_cleanup would yield:

Quoted 9 lines

	struct kobj_type *t = get_ktype(kobj);

	if (t && !t->release)
		pr_debug("kobject: '%s' (%p): does not have a release() "
			 "function, it is broken and must be fixed.\n",
			 kobject_name(kobj), kobj);

(if I understand everything correctly)

Thinking about this more, how does this ever work "correctly"? If we chase the 'normal' path from bus_unregister...

given bus->p->subsys.kobj.ktype = &bus_ktype

bus_unregister(struct bus_type *bus) {
   kset_unregister(&bus->p->subsys) {
      kobject_put(&k->kobj) {
         kref_put(&kobj->kref, kobject_release) {
            (assuming last ref)
            kobject_cleanup(container_of(kref, struct kobject, kref)) {
               	struct kobj_type *t = get_ktype(kobj);

		if (t && !t->release)
			pr_debug("kobject: '%s' (%p): does not have a release() "
				 "function, it is broken and must be fixed.\n",
				 kobject_name(kobj), kobj);

Wouldn't this get hit every time a bus unregisters? I feel like I'm missing something here?

Quoted 10 lines · Greg KH

Quoted 6 lines · Patrick Pannuto

There is a subtle memory leak in driver core error path. Consider the simplified view of bus_register (drivers/base/bus.c):

priv = kzalloc...
kobject_set_name(&priv->subsys.kobj,...) <== allocate in priv->subsys.kobj.name if kset_register(&priv->subsys) FAILS:

Why would this fail?

This is not a likely failure path at all, but (from my understanding), it is possible:

kset_register {
   kobject_add_internal {
      create_dir()

is the most likely candidate to fail, mostly likely for EEXIST due to something else screwy going on. Regardless of how likely it is to fail, it *is* possible, otherwise, what is the point of checking the return code and having an error path? If the error path exists (and a panic is not eminent), we shouldn't leak memory on it IMHO.

Quoted 9 lines

drivers/base/bus.c int bus_register(struct bus_type *bus) · line 904

Quoted 4 lines

drivers/base/bus.c +2 -1

 	retval = kset_register(&priv->subsys);
-	if (retval)
+	if (retval) {
+		kfree(priv->subsys.kobj.name);

I don't think we want to bury the logic of how kobject names are handled up here in the bus code, right? Shouldn't the subsys kobject name be able to be cleaned up on its own somehow instead?

So, my first instinct was to use kobject_cleanup, but a few lines above:

	priv->subsys.kobj.ktype = &bus_ktype;

and bus_ktype's definition, with the notable absence of a release method:

static struct kobj_type bus_ktype = {
	.sysfs_ops	= &bus_sysfs_ops,
};

which in kobject_cleanup would yield:

	struct kobj_type *t = get_ktype(kobj);

	if (t && !t->release)
		pr_debug("kobject: '%s' (%p): does not have a release() "
			 "function, it is broken and must be fixed.\n",
			 kobject_name(kobj), kobj);

(if I understand everything correctly)

I have no idea what would constitute a proper 'release' method in this context, thus I did not write one (and am hoping this patchset would motivate those who know more than me to write one, or indicate to me how to write one, if that would be the correct course of action)

Sorry if any of this is trivial / obvious / incorrect; it's my first time in this code at all, and kobject and friends aren't the easiest to comprehend on first glance :)

This leak is the same as the bus path; cp's kobj name is set, but kset_register fails, free'ing cp, but not cp->class_subsys.kobj.name

This leak is the same as the bus path; cls->kset.kobj.name is set, but kset_register fails, thus nothing will ever call kset_unregister and name will be leaked

This leak is the same as the bus path; mlog_kset.kobj.name is set, but kset_register fails, thus nothing will ever call kset_unregister and name will be leaked

Signed-off-by Patrick Pannuto <[email protected]>

---
 fs/ocfs2/cluster/masklog.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

fs/ocfs2/cluster/masklog.c +1 -0

index c7fba39..de715d4 100644
@@ -154,6 +154,7 @@ 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;

fs/ocfs2/cluster/masklog.c +1 -0

+	return ret;
 }
 
 void mlog_sys_shutdown(void)

This leak is the same as the bus path; kset->kobj.name is set, but kset_register fails, thus nothing will ever call kset_unregister and name will be leaked

Signed-off-by Patrick Pannuto <[email protected]>

---
 lib/kobject.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

lib/kobject.c +0 -0

index f07c572..713b235 100644