In trying to understand the LDM, I came across a few places where
I believe it's possible to leak memory. The first patch appears to
just be a copy-paste bug, and 2-6 are all fixing the same fundamental
problem, an explanation of which is given in 2/6.
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(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 12eec3f..eb1b7fa 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -945,8 +945,8 @@ bus_devices_fail:
bus_remove_file(bus, &bus_attr_uevent);
bus_uevent_fail:
kset_unregister(&bus->p->subsys);
- kfree(bus->p);
out:
+ kfree(bus->p);
bus->p = NULL;
return retval;
}
--
1.7.2
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(-)
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);
goto out;
+ }
retval = bus_create_file(bus, &bus_attr_uevent);
if (retval)
--
1.7.2
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(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index f07c572..713b235 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -843,6 +843,7 @@ struct kset *kset_create_and_add(const char *name,
return NULL;
error = kset_register(kset);
if (error) {
+ kfree(kset->kobj.name);
kfree(kset);
return NULL;
}
--
1.7.2
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(-)
diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index c7fba39..de715d4 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -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;
@@ -163,7 +164,11 @@ int mlog_sys_init(struct kset *o2cb_kset)
kobject_set_name(&mlog_kset.kobj, "logmask");
mlog_kset.kobj.kset = o2cb_kset;
- return kset_register(&mlog_kset);
+
+ ret = kset_register(&mlog_kset);
+ if (ret)
+ kfree(mlog_kset.kobj.name);
+ return ret;
}
void mlog_sys_shutdown(void)
--
1.7.2
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
Signed-off-by: Patrick Pannuto <[email protected]>
---
drivers/base/sys.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/base/sys.c b/drivers/base/sys.c
index 9354dc1..e5deab1 100644
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -22,6 +22,7 @@
#include <linux/device.h>
#include <linux/mutex.h>
#include <linux/interrupt.h>
+#include <linux/slab.h>
#include "base.h"
@@ -148,6 +149,8 @@ int sysdev_class_register(struct sysdev_class *cls)
if (!retval && cls->attrs)
retval = sysfs_create_files(&cls->kset.kobj,
(const struct attribute **)cls->attrs);
+ else
+ kfree(cls->kset.kobj.name);
return retval;
}
--
1.7.2
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
Signed-off-by: Patrick Pannuto <[email protected]>
---
drivers/base/class.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/base/class.c b/drivers/base/class.c
index 8e231d0..045acaf 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -197,6 +197,7 @@ int __class_register(struct class *cls, struct lock_class_key *key)
error = kset_register(&cp->class_subsys);
if (error) {
+ kfree(cp->class_subsys.kobj.name);
kfree(cp);
return error;
}
--
1.7.2
On Thu, Jul 22, 2010 at 03:09:01PM -0700, Patrick Pannuto wrote:
> 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(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 12eec3f..eb1b7fa 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -945,8 +945,8 @@ bus_devices_fail:
> bus_remove_file(bus, &bus_attr_uevent);
> bus_uevent_fail:
> kset_unregister(&bus->p->subsys);
> - kfree(bus->p);
> out:
> + kfree(bus->p);
This fix is already in my tree.
thanks,
greg k-h
On Thu, Jul 22, 2010 at 03:09:04PM -0700, Patrick Pannuto wrote:
> 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
Same objection.
On Thu, Jul 22, 2010 at 03:09:06PM -0700, Patrick Pannuto wrote:
> 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(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index f07c572..713b235 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -843,6 +843,7 @@ struct kset *kset_create_and_add(const char *name,
> return NULL;
> error = kset_register(kset);
> if (error) {
> + kfree(kset->kobj.name);
Again, we should be able to clean this up automatically better than
forcing this code to "know" that it has to handle this.
Any other ideas?
And are these error paths something that you have seen in a working
system?
thanks,
greg k-h
On Thu, Jul 22, 2010 at 03:09:03PM -0700, Patrick Pannuto wrote:
> 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
Same objection here, I don't like how you are having to "know" about how
kobject names are handled here.
thanks,
greg k-h
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 <[email protected]>
> ---
> 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
Patrick Pannuto wrote:
> 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(-)
>
> diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
> index c7fba39..de715d4 100644
> --- a/fs/ocfs2/cluster/masklog.c
> +++ b/fs/ocfs2/cluster/masklog.c
> @@ -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;
> @@ -163,7 +164,11 @@ int mlog_sys_init(struct kset *o2cb_kset)
>
> kobject_set_name(&mlog_kset.kobj, "logmask");
> mlog_kset.kobj.kset = o2cb_kset;
> - return kset_register(&mlog_kset);
> +
> + ret = kset_register(&mlog_kset);
> + if (ret)
> + kfree(mlog_kset.kobj.name);
Aren't we supposed to use kobject_cleanup() here?
> + return ret;
> }
>
> void mlog_sys_shutdown(void)
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
>>>
>>> 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?
On Fri, Jul 23, 2010 at 06:19:23PM -0700, Patrick Pannuto wrote:
> 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.
I agree, within reason. If the memory leak is not going to happen on a
working system, then digging into the kobject internals like this isn't
worth it, don't you agree?
> >> 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)
But yet it doesn't :)
You might want to dig further to figure out why if you are curious.
> 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)
Leave it as-is, right?
> 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 :)
I agree, they aren't simple, any areas you can find to make them simpler
to understand is always appreciated.
thanks,
greg k-h
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