login
Search
Hi greg, When I remove only the kvm-intel module without removing the kvm module itself, I get an error saying that a kobject is trying to be reinitialized. Its because of the fact that kvm reuses a kobject in kvm_init when calling sysdev_register. This patch fixes kobject_cleanup by marking the kobject as uninitialized when we cleanup to allow kobjects to be reused. Signed-off-by: Balaji Rao <balajirrao@gmail.com> diff --git a/lib/kobject.c b/lib/kobject.c index 0d03252..fbdefb8 100644 --- a/lib/kobject.c +++ b/lib/kobject.c @@ -577,6 +577,9 @@ static void kobject_cleanup(struct kobject *kobj) pr_debug("kobject: '%s': free name\n", name); kfree(name); } + + /* Set the state to uninitialized */ + kobj->state_initialized = 0; } static void kobject_release(struct kref *kref) -- regards, balaji rao 3rd year, Dept. of Mechanical Engineering, National Institute of Technology, Karnataka --
Where exactly in the code does that happen? kobjects should not be "reused" as that implies that they are static, and not dynamically allocated, right? Which kobject is this? The patch seems correct, but I would like to verify which kobject is being reused here first. thanks, greg k-h --
On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
Yes, its static. Here's the code from virt/kvm_main.c:1269
static struct sys_device kvm_sysdev = {
.id = 0,
.cls = &kvm_sysdev_class,
};
this sys_device is being registered/unregistered when kvm-intel is
Yes it is! But, why do you hate it ?
--
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka
--
For reasons like this :) kobjects should not be static. the sysdevice stuff was a hack when it was originally created and never touched since the mid 2.5 days. It needs to be fixed up a lot, and is on my TODO list, slowly getting closer to the top... thanks, greg k-h --
Strange. On my TODO list, things slowly get pushed to the bottom. -- Any sufficiently difficult bug is indistinguishable from a feature. --
Hi, This patch does not fix it all! The problem is in fact more involved. I also get these BUG reports when I reload kvm-intel. BUG kmalloc-8: Object already free [ 74.696570] ----------------------------------------------------------------------------- [ 74.696596] [ 74.697310] INFO: Allocated in strndup_user+0x30/0x62 age=587 cpu=2 pid=1439 [ 74.697845] INFO: Freed in kobject_set_name_vargs+0x29/0x32 age=559 cpu=3 pid=1439 [ 74.698008] INFO: Slab 0xc16f93a0 used=10 fp=0xf7c9d2d8 flags=0x10000c3 [ 74.698008] INFO: Object 0xf7c9d1f8 @offset=504 fp=0xf7c9d578 This happens because, sysdev_class_register assigns a name to the kobject, and kfrees the old name if any. The poisoned 'name' object persists in case of statically allocated kobjects and as its passed to kfree again when re registered, we get the above warning. So, AFAICS the best way to solve this is by fixing the kobject users (kvm, oprofile etc.) to use dynamic kobjects instead of static ones or memset the kobject to zero before passing it to sysdev_register. Greg ? -- regards, balaji rao 3rd year, Dept. of Mechanical Engineering, National Institute of Technology, Karnataka --
Ugh, that's horrible. And people wonder why I hate the sysdev code :)
Does it also mean that when we do cleanup sysdev devices, we are freeing
a name that might not have been dynamically allocated? If so, we need
to fix that as well.
A simple 'strdup' of the class name in sysdev_class_register() should
Yes, that would be great to fix up, but probably unlikly so late in the
release cycle.
How about the patch below, does it work for you? (build tested only)
thanks,
greg k-h
---
drivers/base/sys.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -130,13 +130,16 @@ static struct kset *system_kset;
int sysdev_class_register(struct sysdev_class * cls)
{
+ char *name;
+
pr_debug("Registering sysdev class '%s'\n",
kobject_name(&cls->kset.kobj));
INIT_LIST_HEAD(&cls->drivers);
cls->kset.kobj.parent = &system_kset->kobj;
cls->kset.kobj.ktype = &ktype_sysdev_class;
cls->kset.kobj.kset = system_kset;
- kobject_set_name(&cls->kset.kobj, cls->name);
+ name = kstrdup(cls->name, GFP_KERNEL);
+ kobject_set_name(&cls->kset.kobj, name);
return kset_register(&cls->kset);
}
--
Wait, no, that's not going to work, the name is already kstrduped. That patch shouldn't do anything except leak memory. Let me try something else... thanks, greg k-h --
-- regards, balaji rao 3rd year, Dept. of Mechanical Engineering, National Institute of Technology, Karnataka --
I like the memset idea, how about this patch instead? thanks, greg k-h --- a/drivers/base/sys.c +++ b/drivers/base/sys.c @@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_ pr_debug("Registering sysdev class '%s'\n", kobject_name(&cls->kset.kobj)); INIT_LIST_HEAD(&cls->drivers); + memset(&cls->kset.kobj, 0x00, sizeof(struct kobject)); cls->kset.kobj.parent = &system_kset->kobj; cls->kset.kobj.ktype = &ktype_sysdev_class; cls->kset.kobj.kset = system_kset; --
This should work.But I am afraid if we zeroed it, the kfree poison test wouldn't catch even the genuine cases. Isn't it ? CMIIW. A better fix according to me would be to zero the kobject in the places where we know it is being re-registered (kvm, oprofile) etc. This should do for now. But we should fix the sys_device users later, for the next cycle. -- regards, balaji rao 3rd year, Dept. of Mechanical Engineering, National Institute of Technology, Karnataka --
What "genuine cases"? kobjects should always be initialized to zero Are you sure you know all of the sysdev_class objects that can be re-registered? Can you test this patch out? thanks, greg k-h --
On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
Yes the idea works. One more memset is needed in sysdev_register. Here's the final patch.
diff --git a/drivers/base/sys.c b/drivers/base/sys.c
index 2f79c55..7c839d9 100644
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_class * cls)
pr_debug("Registering sysdev class '%s'\n",
kobject_name(&cls->kset.kobj));
INIT_LIST_HEAD(&cls->drivers);
+ memset(&cls->kset.kobj, 0x00, sizeof(struct kobject));
cls->kset.kobj.parent = &system_kset->kobj;
cls->kset.kobj.ktype = &ktype_sysdev_class;
cls->kset.kobj.kset = system_kset;
@@ -227,6 +228,7 @@ int sysdev_register(struct sys_device * sysdev)
pr_debug("Registering sys device '%s'\n", kobject_name(&sysdev->kobj));
+ memset(&sysdev->kobj, 0x00, sizeof(struct kobject));
/* Make sure the kset is set */
sysdev->kobj.kset = &cls->kset;
--
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka
--
Balaji Rao writes: > Yes the idea works. One more memset is needed in sysdev_register. Here's the final patch. > > diff --git a/drivers/base/sys.c b/drivers/base/sys.c > index 2f79c55..7c839d9 100644 > --- a/drivers/base/sys.c > +++ b/drivers/base/sys.c > @@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_class * cls) > pr_debug("Registering sysdev class '%s'\n", > kobject_name(&cls->kset.kobj)); > INIT_LIST_HEAD(&cls->drivers); > + memset(&cls->kset.kobj, 0x00, sizeof(struct kobject)); > cls->kset.kobj.parent = &system_kset->kobj; > cls->kset.kobj.ktype = &ktype_sysdev_class; > cls->kset.kobj.kset = system_kset; > @@ -227,6 +228,7 @@ int sysdev_register(struct sys_device * sysdev) > > pr_debug("Registering sys device '%s'\n", kobject_name(&sysdev->kobj)); > > + memset(&sysdev->kobj, 0x00, sizeof(struct kobject)); > /* Make sure the kset is set */ > sysdev->kobj.kset = &cls->kset; > Thanks, 2.6.25-rc4 + these two memset()s is finally stable for me with no warnings, BUG()s, or panics. (However, the patch is whitespace damaged with initial tabs converted to spaces.) If you want to pass this on to Linus, you can add a Tested-by: Mikael Pettersson <mikpe@it.uu.se> /Mikael --
Yes, you're right. This also means I can drop the kobject_cleanup() patch from you as well, as that should no longer be needed, right? thanks, greg k-h --
Yes, That's no longer needed. I am resending the patch, as the previous one had some whitespace issues. Signed-off-by: Balaji Rao <balajirrao@gmail.com> Tested-by: Mikael Pettersson <mikpe@it.uu.se> diff --git a/drivers/base/sys.c b/drivers/base/sys.c index 2f79c55..7c839d9 100644 --- a/drivers/base/sys.c +++ b/drivers/base/sys.c @@ -133,6 +133,7 @@ int sysdev_class_register(struct sysdev_class * cls) pr_debug("Registering sysdev class '%s'\n", kobject_name(&cls->kset.kobj)); INIT_LIST_HEAD(&cls->drivers); + memset(&cls->kset.kobj, 0x00, sizeof(struct kobject)); cls->kset.kobj.parent = &system_kset->kobj; cls->kset.kobj.ktype = &ktype_sysdev_class; cls->kset.kobj.kset = system_kset; @@ -227,6 +228,7 @@ int sysdev_register(struct sys_device * sysdev) pr_debug("Registering sys device '%s'\n", kobject_name(&sysdev->kobj)); + memset(&sysdev->kobj, 0x00, sizeof(struct kobject)); /* Make sure the kset is set */ sysdev->kobj.kset = &cls->kset; -- regards, balaji rao 3rd year, Dept. of Mechanical Engineering, National Institute of Technology, Karnataka --
I think that gmail did that, my original version was fine :) thanks, greg k-h --
