Driver core: Fix memory leak on bus_register error path
in [Kernel]
|
Prev: Driver core: Fix memory leak on class_register error path
Next: Driver core: Fix memory leak on sysdev_class_register error path
From: Greg KH on 22 Jul 2010 19:50 On Thu, Jul 22, 2010 at 03:09:02PM -0700, Patrick Pannuto wrote: > 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? > (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 <ppannuto(a)codeaurora.org> > --- > drivers/base/bus.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > index eb1b7fa..fea1774 100644 > --- a/drivers/base/bus.c > +++ b/drivers/base/bus.c > @@ -900,8 +900,10 @@ int bus_register(struct bus_type *bus) > priv->drivers_autoprobe = 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? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Patrick Pannuto on 23 Jul 2010 21:20 On 07/22/2010 04:41 PM, Greg KH wrote: > On Thu, Jul 22, 2010 at 03:09:02PM -0700, Patrick Pannuto wrote: >> 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. >> >> 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 :) -pat -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Patrick Pannuto on 23 Jul 2010 21:50 >>> >>> 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) > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on 26 Jul 2010 20:10
On Fri, Jul 23, 2010 at 06:48:49PM -0700, Patrick Pannuto wrote: > >>> > >>> 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) > > > > 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? You are, busses are wierd :) I can't recall off the top of my head, but it all works out ok in the end. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |