[PATCH] Mark kobjects as unitialized

[PATCH] Mark kobjects as unitialized

[Balaji Rao]

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

[PATCH] Mark kobjects as unitialized

[Balaji Rao]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Thu, Mar 06, 2008 at 10:23:18PM +0530, Balaji Rao wrote:
> 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.

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?

Ugh, is this the sys_device stuff?  I hate that code...

> This patch fixes kobject_cleanup by marking the kobject as
> uninitialized when we cleanup to allow kobjects to be reused.

The patch seems correct, but I would like to verify which kobject is
being reused here first.

thanks,

greg k-h

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Thu, Mar 06, 2008 at 10:23:18PM +0530, Balaji Rao wrote:
> 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.

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?

Ugh, is this the sys_device stuff?  I hate that code...

> This patch fixes kobject_cleanup by marking the kobject as
> uninitialized when we cleanup to allow kobjects to be reused.

The patch seems correct, but I would like to verify which kobject is
being reused here first.

thanks,

greg k-h

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
<snip>
> 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?
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 
loaded/unloaded.
>
> Ugh, is this the sys_device stuff?  I hate that code...
>
Yes it is! But, why do you hate it ?

-- 
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
<snip>
> 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?
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 
loaded/unloaded.
>
> Ugh, is this the sys_device stuff?  I hate that code...
>
Yes it is! But, why do you hate it ?

-- 
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> <snip>
> > 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?
> 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 
> loaded/unloaded.

Ah, ok.  I'll add this patch then.

> > Ugh, is this the sys_device stuff?  I hate that code...
> >
> Yes it is! But, why do you hate it ?

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

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> <snip>
> > 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?
> 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 
> loaded/unloaded.

Ah, ok.  I'll add this patch then.

> > Ugh, is this the sys_device stuff?  I hate that code...
> >
> Yes it is! But, why do you hate it ?

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-devel] [PATCH] Mark kobjects as unitialized

[Avi Kivity]

Greg KH wrote:
> and is on my TODO list, slowly getting
> closer to the top...
>
>   

Strange.  On my TODO list, things slowly get pushed to the bottom.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.

Re: [PATCH] Mark kobjects as unitialized

[Avi Kivity]

Greg KH wrote:
> and is on my TODO list, slowly getting
> closer to the top...
>
>   

Strange.  On my TODO list, things slowly get pushed to the bottom.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > <snip>
> > > 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?
> > 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 
> > loaded/unloaded.
> 
> Ah, ok.  I'll add this patch then.
> 
> > > Ugh, is this the sys_device stuff?  I hate that code...
> > >
> > Yes it is! But, why do you hate it ?
> 
> 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...
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

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > <snip>
> > > > 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?
> > > 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 
> > > loaded/unloaded.
> > 
> > Ah, ok.  I'll add this patch then.
> > 
> > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > >
> > > Yes it is! But, why do you hate it ?
> > 
> > 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...
> 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.

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
fix all of this, right?

> 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.

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);
 }
 

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > <snip>
> > > > 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?
> > > 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 
> > > loaded/unloaded.
> > 
> > Ah, ok.  I'll add this patch then.
> > 
> > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > >
> > > Yes it is! But, why do you hate it ?
> > 
> > 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...
> 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.

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
fix all of this, right?

> 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.

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);
 }
 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sat, Mar 08, 2008 at 09:06:49PM -0800, Greg KH wrote:
> On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > <snip>
> > > > > 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?
> > > > 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 
> > > > loaded/unloaded.
> > > 
> > > Ah, ok.  I'll add this patch then.
> > > 
> > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > >
> > > > Yes it is! But, why do you hate it ?
> > > 
> > > 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...
> > 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.
> 
> 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
> fix all of this, right?

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

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sat, Mar 08, 2008 at 09:06:49PM -0800, Greg KH wrote:
> On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > <snip>
> > > > > 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?
> > > > 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 
> > > > loaded/unloaded.
> > > 
> > > Ah, ok.  I'll add this patch then.
> > > 
> > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > >
> > > > Yes it is! But, why do you hate it ?
> > > 
> > > 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...
> > 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.
> 
> 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
> fix all of this, right?

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Sunday 09 March 2008 10:36:49 am Greg KH wrote:
> On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > <snip>
> > > > > 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?
> > > > 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 
> > > > loaded/unloaded.
> > > 
> > > Ah, ok.  I'll add this patch then.
> > > 
> > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > >
> > > > Yes it is! But, why do you hate it ?
> > > 
> > > 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...
> > 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.
> 
> 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.
> 
No, that doesn't happen. We free the name only if it is allocated..
> A simple 'strdup' of the class name in sysdev_class_register() should
> fix all of this, right?
> 
> > 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.
> 
> 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);
>  }
>  
> 



-- 
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Sunday 09 March 2008 10:36:49 am Greg KH wrote:
> On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > <snip>
> > > > > 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?
> > > > 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 
> > > > loaded/unloaded.
> > > 
> > > Ah, ok.  I'll add this patch then.
> > > 
> > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > >
> > > > Yes it is! But, why do you hate it ?
> > > 
> > > 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...
> > 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.
> 
> 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.
> 
No, that doesn't happen. We free the name only if it is allocated..
> A simple 'strdup' of the class name in sysdev_class_register() should
> fix all of this, right?
> 
> > 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.
> 
> 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);
>  }
>  
> 



-- 
regards,
balaji rao
3rd year,
Dept. of Mechanical Engineering,
National Institute of Technology, Karnataka

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > <snip>
> > > > 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?
> > > 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 
> > > loaded/unloaded.
> > 
> > Ah, ok.  I'll add this patch then.
> > 
> > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > >
> > > Yes it is! But, why do you hate it ?
> > 
> > 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...
> 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.

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;

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > <snip>
> > > > 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?
> > > 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 
> > > loaded/unloaded.
> > 
> > Ah, ok.  I'll add this patch then.
> > 
> > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > >
> > > Yes it is! But, why do you hate it ?
> > 
> > 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...
> 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.

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 SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Sunday 09 March 2008 12:03:08 pm Greg KH wrote:
> On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > <snip>
> > > > > 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?
> > > > 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 
> > > > loaded/unloaded.
> > > 
> > > Ah, ok.  I'll add this patch then.
> > > 
> > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > >
> > > > Yes it is! But, why do you hate it ?
> > > 
> > > 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...
> > 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.
> 
> 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

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Sunday 09 March 2008 12:03:08 pm Greg KH wrote:
> On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > <snip>
> > > > > 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?
> > > > 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 
> > > > loaded/unloaded.
> > > 
> > > Ah, ok.  I'll add this patch then.
> > > 
> > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > >
> > > > Yes it is! But, why do you hate it ?
> > > 
> > > 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...
> > 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.
> 
> 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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 12:06:02PM +0530, Balaji Rao wrote:
> On Sunday 09 March 2008 12:03:08 pm Greg KH wrote:
> > On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > > <snip>
> > > > > > 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?
> > > > > 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 
> > > > > loaded/unloaded.
> > > > 
> > > > Ah, ok.  I'll add this patch then.
> > > > 
> > > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > > >
> > > > > Yes it is! But, why do you hate it ?
> > > > 
> > > > 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...
> > > 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.
> > 
> > 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.

What "genuine cases"?  kobjects should always be initialized to zero
before they are registered.

> 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.

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

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 12:06:02PM +0530, Balaji Rao wrote:
> On Sunday 09 March 2008 12:03:08 pm Greg KH wrote:
> > On Sun, Mar 09, 2008 at 03:37:16AM +0530, Balaji Rao wrote:
> > > On Thursday 06 March 2008 11:35:59 pm Greg KH wrote:
> > > > On Thu, Mar 06, 2008 at 11:20:50PM +0530, Balaji Rao wrote:
> > > > > On Thursday 06 March 2008 10:35:14 pm Greg KH wrote:
> > > > > <snip>
> > > > > > 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?
> > > > > 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 
> > > > > loaded/unloaded.
> > > > 
> > > > Ah, ok.  I'll add this patch then.
> > > > 
> > > > > > Ugh, is this the sys_device stuff?  I hate that code...
> > > > > >
> > > > > Yes it is! But, why do you hate it ?
> > > > 
> > > > 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...
> > > 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.
> > 
> > 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.

What "genuine cases"?  kobjects should always be initialized to zero
before they are registered.

> 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.

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
<snip>
> > > > 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.
> > > 
> > > 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.
> 
> What "genuine cases"?  kobjects should always be initialized to zero
> before they are registered.
> 
The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > 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.
> 
> Are you sure you know all of the sysdev_class objects that can be
> re-registered?
> 
Hmm.. Right. No, I only know that KVM and Oprofile use it.
> Can you test this patch out?
> 
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

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
<snip>
> > > > 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.
> > > 
> > > 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.
> 
> What "genuine cases"?  kobjects should always be initialized to zero
> before they are registered.
> 
The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > 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.
> 
> Are you sure you know all of the sysdev_class objects that can be
> re-registered?
> 
Hmm.. Right. No, I only know that KVM and Oprofile use it.
> Can you test this patch out?
> 
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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Mikael Pettersson]

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

Re: [PATCH] Mark kobjects as unitialized

[Mikael Pettersson]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 11:49:26AM +0100, Mikael Pettersson wrote:
> 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>

Thanks for testing.

greg k-h

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 11:49:26AM +0100, Mikael Pettersson wrote:
> 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>

Thanks for testing.

greg k-h

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 12:51:15PM +0530, Balaji Rao wrote:
> On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
> <snip>
> > > > > 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.
> > > > 
> > > > 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.
> > 
> > What "genuine cases"?  kobjects should always be initialized to zero
> > before they are registered.
> > 
> The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > > 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.
> > 
> > Are you sure you know all of the sysdev_class objects that can be
> > re-registered?
> > 
> Hmm.. Right. No, I only know that KVM and Oprofile use it.
> > Can you test this patch out?
> > 
> 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;

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

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Sun, Mar 09, 2008 at 12:51:15PM +0530, Balaji Rao wrote:
> On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
> <snip>
> > > > > 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.
> > > > 
> > > > 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.
> > 
> > What "genuine cases"?  kobjects should always be initialized to zero
> > before they are registered.
> > 
> The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > > 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.
> > 
> > Are you sure you know all of the sysdev_class objects that can be
> > re-registered?
> > 
> Hmm.. Right. No, I only know that KVM and Oprofile use it.
> > Can you test this patch out?
> > 
> 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;

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Monday 10 March 2008 09:22:00 pm Greg KH wrote:
> On Sun, Mar 09, 2008 at 12:51:15PM +0530, Balaji Rao wrote:
> > On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
> > <snip>
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > 
> > > What "genuine cases"?  kobjects should always be initialized to zero
> > > before they are registered.
> > > 
> > The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > > > 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.
> > > 
> > > Are you sure you know all of the sysdev_class objects that can be
> > > re-registered?
> > > 
> > Hmm.. Right. No, I only know that KVM and Oprofile use it.
> > > Can you test this patch out?
> > > 
> > 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;
> 
> 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?
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

Re: [PATCH] Mark kobjects as unitialized

[Balaji Rao]

On Monday 10 March 2008 09:22:00 pm Greg KH wrote:
> On Sun, Mar 09, 2008 at 12:51:15PM +0530, Balaji Rao wrote:
> > On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
> > <snip>
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > 
> > > What "genuine cases"?  kobjects should always be initialized to zero
> > > before they are registered.
> > > 
> > The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > > > 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.
> > > 
> > > Are you sure you know all of the sysdev_class objects that can be
> > > re-registered?
> > > 
> > Hmm.. Right. No, I only know that KVM and Oprofile use it.
> > > Can you test this patch out?
> > > 
> > 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;
> 
> 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?
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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Mon, Mar 10, 2008 at 09:35:56PM +0530, Balaji Rao wrote:
> On Monday 10 March 2008 09:22:00 pm Greg KH wrote:
> > On Sun, Mar 09, 2008 at 12:51:15PM +0530, Balaji Rao wrote:
> > > On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
> > > <snip>
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > 
> > > > What "genuine cases"?  kobjects should always be initialized to zero
> > > > before they are registered.
> > > > 
> > > The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > > > > 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.
> > > > 
> > > > Are you sure you know all of the sysdev_class objects that can be
> > > > re-registered?
> > > > 
> > > Hmm.. Right. No, I only know that KVM and Oprofile use it.
> > > > Can you test this patch out?
> > > > 
> > > 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;
> > 
> > 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?
> Yes, That's no longer needed. 
> 
> I am resending the patch, as the previous one had some whitespace issues.

I think that gmail did that, my original version was fine :)

thanks,

greg k-h

Re: [PATCH] Mark kobjects as unitialized

[Greg KH]

On Mon, Mar 10, 2008 at 09:35:56PM +0530, Balaji Rao wrote:
> On Monday 10 March 2008 09:22:00 pm Greg KH wrote:
> > On Sun, Mar 09, 2008 at 12:51:15PM +0530, Balaji Rao wrote:
> > > On Sunday 09 March 2008 12:33:07 pm Greg KH wrote:
> > > <snip>
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > 
> > > > What "genuine cases"?  kobjects should always be initialized to zero
> > > > before they are registered.
> > > > 
> > > The cases in which a freed kobject is passed to the register function ? Probably they will be caught when do a memset..
> > > > > 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.
> > > > 
> > > > Are you sure you know all of the sysdev_class objects that can be
> > > > re-registered?
> > > > 
> > > Hmm.. Right. No, I only know that KVM and Oprofile use it.
> > > > Can you test this patch out?
> > > > 
> > > 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;
> > 
> > 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?
> Yes, That's no longer needed. 
> 
> I am resending the patch, as the previous one had some whitespace issues.

I think that gmail did that, my original version was fine :)

thanks,

greg k-h

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

patch kobjects-mark-cleaned-up-kobjects-as-unitialized.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: kobjects: mark cleaned up kobjects as unitialized

to my gregkh-2.6 tree.  Its filename is

     kobjects-mark-cleaned-up-kobjects-as-unitialized.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


>From balajirrao@gmail.com  Thu Mar  6 10:19:32 2008
From: Balaji Rao <balajirrao@gmail.com>
Date: Thu, 6 Mar 2008 22:23:18 +0530
Subject: kobjects: mark cleaned up kobjects as unitialized
To: gregkh@suse.de
Cc: <kvm-devel@lists.sourceforge.net>, linux-kernel@vger.kernel.org
Message-ID: <200803062223.18857.balajirrao@gmail.com>
Content-Disposition: inline


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>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 lib/kobject.c |    3 +++
 1 file changed, 3 insertions(+)

--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -629,6 +629,9 @@ static void kobject_cleanup(struct kobje
 		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)


Patches currently in gregkh-2.6 which might be from balajirrao@gmail.com are

driver-core/kobjects-mark-cleaned-up-kobjects-as-unitialized.patch

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

patch patches/driver-core/kobjects-mark-cleaned-up-kobjects-as-unitialized.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: kobjects: mark cleaned up kobjects as unitialized

to my gregkh-2.6 tree.  Its filename is

     patches/driver-core/kobjects-mark-cleaned-up-kobjects-as-unitialized.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


>From balajirrao@gmail.com  Thu Mar  6 10:19:32 2008
From: Balaji Rao <balajirrao@gmail.com>
Date: Thu, 6 Mar 2008 22:23:18 +0530
Subject: kobjects: mark cleaned up kobjects as unitialized
To: gregkh@suse.de
Cc: <kvm-devel@lists.sourceforge.net>, linux-kernel@vger.kernel.org
Message-ID: <200803062223.18857.balajirrao@gmail.com>
Content-Disposition: inline


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>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 lib/kobject.c |    7 +++++++
 1 file changed, 7 insertions(+)

--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -566,6 +566,13 @@ static void kobject_cleanup(struct kobje
 		kobject_del(kobj);
 	}
 
+	/* set the states incase someone tries to use this object again */
+	kobj->state_initialized = 0;
+	kobj->state_in_sysfs = 0;
+	kobj->state_add_uevent_sent = 0;
+	kobj->state_remove_uevent_sent = 0;
+	kobj->state_initialized = 0;
+
 	if (t && t->release) {
 		pr_debug("kobject: '%s' (%p): calling ktype release\n",
 			 kobject_name(kobj), kobj);


Patches currently in gregkh-2.6 which might be from balajirrao@gmail.com are

driver-core/kobjects-mark-cleaned-up-kobjects-as-unitialized.patch

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/