subsystem_unregister() breakage in -mm

subsystem_unregister() breakage in -mm

[Miklos Szeredi]

I get "BUG: failure at mm/slab.c:591/page_get_cache()!" in latest -mm
when removing the fuse module.

This patch titled "kobject: remove the static array for the name"
looks like it's responsible.  Reverting it cures the problem.

The root of the problem seems to be, that decl_subsys() initializes
k_name with a string constant.  Then subsystem_unregister() will put
the kobject, which will free the k_name.

So, what's the right way to deal with this?

Thanks,
Miklos

Re: subsystem_unregister() breakage in -mm

[Greg KH]

On Wed, Sep 26, 2007 at 12:27:14AM +0200, Miklos Szeredi wrote:
> I get "BUG: failure at mm/slab.c:591/page_get_cache()!" in latest -mm
> when removing the fuse module.
> 
> This patch titled "kobject: remove the static array for the name"
> looks like it's responsible.  Reverting it cures the problem.
> 
> The root of the problem seems to be, that decl_subsys() initializes
> k_name with a string constant.  Then subsystem_unregister() will put
> the kobject, which will free the k_name.
> 
> So, what's the right way to deal with this?

How is the static kobject created by decl_subsys() getting it's release
function called when it is never really "released" as it is a static
kobject?

The "correct" way to fix this is to dynamically create the subsystem
kobject, and is what I am working toward accomplishing.  But I didn't
think it was necessary just yet.

Let me go dig some more...

thanks,

greg k-h

Re: subsystem_unregister() breakage in -mm

[Miklos Szeredi]

> On Wed, Sep 26, 2007 at 12:27:14AM +0200, Miklos Szeredi wrote:
> > I get "BUG: failure at mm/slab.c:591/page_get_cache()!" in latest -mm
> > when removing the fuse module.
> > 
> > This patch titled "kobject: remove the static array for the name"
> > looks like it's responsible.  Reverting it cures the problem.
> > 
> > The root of the problem seems to be, that decl_subsys() initializes
> > k_name with a string constant.  Then subsystem_unregister() will put
> > the kobject, which will free the k_name.
> > 
> > So, what's the right way to deal with this?
> 
> How is the static kobject created by decl_subsys() getting it's release
> function called when it is never really "released" as it is a static
> kobject?

Don't know.

This is the stack trace I get:

BUG: failure at mm/slab.c:591/page_get_cache()!
Kernel panic - not syncing: BUG!

EIP: 0023:[<400e7f84>] CPU: 0 Not tainted ESP: 002b:bf77e018 EFLAGS: 00000246
    Not tainted
EAX: ffffffda EBX: bf77e040 ECX: 00000880 EDX: bf780884
ESI: 00000003 EDI: bf77e040 EBP: bf7808d8 DS: 002b ES: 002b
08eefd2c:  [<0806ec8c>] show_regs+0x104/0x106
08eefd48:  [<0805b96a>] panic_exit+0x2c/0x4b
08eefd58:  [<0808fcab>] notifier_call_chain+0x36/0x61
08eefd78:  [<0808fd83>] __atomic_notifier_call_chain+0x30/0x32
08eefd94:  [<0808fdb4>] atomic_notifier_call_chain+0x2f/0x31
08eefdb0:  [<08075cf0>] panic+0x75/0x126
08eefdd4:  [<080bc3f5>] kfree+0xb1/0xc6
08eefdf8:  [<08137de9>] kobject_cleanup+0x37/0x6a
08eefe10:  [<08137e30>] kobject_release+0x14/0x16
08eefe1c:  [<08138711>] kref_put+0x30/0xaf
08eefe3c:  [<08137e52>] kobject_put+0x20/0x22
08eefe4c:  [<08137d8b>] kobject_unregister+0x2e/0x34
08eefe60:  [<08137fc9>] kset_unregister+0x18/0x1a
08eefe6c:  [<08138066>] subsystem_unregister+0x11/0x13
08eefe78:  [<0a88579f>] fuse_sysfs_cleanup+0x1e/0x47 [fuse]
08eefe84:  [<0a885d6c>] fuse_exit+0x1c/0x2c [fuse]
08eefe90:  [<080986ba>] sys_delete_module+0x17f/0x21e
08eefef4:  [<0805bd71>] handle_syscall+0x8d/0xac
08eeff48:  [<0806adc2>] handle_trap+0x27/0x135
08eeff70:  [<0806b5a2>] userspace+0x2cb/0x31b
08eeffe4:  [<08058ec2>] fork_handler+0x76/0x88
08eefffc:  [<00000000>] _sinittext+0xf7fb7000/0x14

Re: subsystem_unregister() breakage in -mm

[Greg KH]

On Wed, Sep 26, 2007 at 01:02:37AM +0200, Miklos Szeredi wrote:
> > On Wed, Sep 26, 2007 at 12:27:14AM +0200, Miklos Szeredi wrote:
> > > I get "BUG: failure at mm/slab.c:591/page_get_cache()!" in latest -mm
> > > when removing the fuse module.
> > > 
> > > This patch titled "kobject: remove the static array for the name"
> > > looks like it's responsible.  Reverting it cures the problem.
> > > 
> > > The root of the problem seems to be, that decl_subsys() initializes
> > > k_name with a string constant.  Then subsystem_unregister() will put
> > > the kobject, which will free the k_name.
> > > 
> > > So, what's the right way to deal with this?
> > 
> > How is the static kobject created by decl_subsys() getting it's release
> > function called when it is never really "released" as it is a static
> > kobject?
> 
> Don't know.
> 
> This is the stack trace I get:
> 
> BUG: failure at mm/slab.c:591/page_get_cache()!
> Kernel panic - not syncing: BUG!
> 
> EIP: 0023:[<400e7f84>] CPU: 0 Not tainted ESP: 002b:bf77e018 EFLAGS: 00000246
>     Not tainted
> EAX: ffffffda EBX: bf77e040 ECX: 00000880 EDX: bf780884
> ESI: 00000003 EDI: bf77e040 EBP: bf7808d8 DS: 002b ES: 002b
> 08eefd2c:  [<0806ec8c>] show_regs+0x104/0x106
> 08eefd48:  [<0805b96a>] panic_exit+0x2c/0x4b
> 08eefd58:  [<0808fcab>] notifier_call_chain+0x36/0x61
> 08eefd78:  [<0808fd83>] __atomic_notifier_call_chain+0x30/0x32
> 08eefd94:  [<0808fdb4>] atomic_notifier_call_chain+0x2f/0x31
> 08eefdb0:  [<08075cf0>] panic+0x75/0x126
> 08eefdd4:  [<080bc3f5>] kfree+0xb1/0xc6
> 08eefdf8:  [<08137de9>] kobject_cleanup+0x37/0x6a
> 08eefe10:  [<08137e30>] kobject_release+0x14/0x16
> 08eefe1c:  [<08138711>] kref_put+0x30/0xaf
> 08eefe3c:  [<08137e52>] kobject_put+0x20/0x22
> 08eefe4c:  [<08137d8b>] kobject_unregister+0x2e/0x34

Hm, this make sense from an object lifecycle type of thing, and it makes
sense why it is dying.  I think I know how to fix it up, give me a few
hours (dinner time here...)

thanks,

greg k-h

Re: subsystem_unregister() breakage in -mm

[Greg KH]

On Wed, Sep 26, 2007 at 01:02:37AM +0200, Miklos Szeredi wrote:
> > On Wed, Sep 26, 2007 at 12:27:14AM +0200, Miklos Szeredi wrote:
> > > I get "BUG: failure at mm/slab.c:591/page_get_cache()!" in latest -mm
> > > when removing the fuse module.
> > > 
> > > This patch titled "kobject: remove the static array for the name"
> > > looks like it's responsible.  Reverting it cures the problem.
> > > 
> > > The root of the problem seems to be, that decl_subsys() initializes
> > > k_name with a string constant.  Then subsystem_unregister() will put
> > > the kobject, which will free the k_name.
> > > 
> > > So, what's the right way to deal with this?
> > 
> > How is the static kobject created by decl_subsys() getting it's release
> > function called when it is never really "released" as it is a static
> > kobject?
> 
> Don't know.
> 
> This is the stack trace I get:
> 
> BUG: failure at mm/slab.c:591/page_get_cache()!
> Kernel panic - not syncing: BUG!

Ok, here's a patch that fixes this for me.  It should go on top of the
-mm tree.  Let me know of this works or not.

In the end, we need to dynamically allocate these subsystems to properly
fix this.  In looking through the tree, there really isn't that many of
them, so I'll try to knock that out later this week.


thanks,

greg k-h

---
 lib/kobject.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -487,11 +487,11 @@ void kobject_cleanup(struct kobject * ko
 	struct kobject * parent = kobj->parent;
 
 	pr_debug("kobject %s: cleaning up\n",kobject_name(kobj));
-	kfree(kobj->k_name);
-	kobj->k_name = NULL;
-	if (t && t->release)
+	if (t && t->release) {
 		t->release(kobj);
-	else
+		kfree(kobj->k_name);
+		kobj->k_name = NULL;
+	} else
 		pr_debug("kobject '%s' does not have a release() function, "
 			"if this is not a directory kobject, it is broken "
 			"and must be fixed.\n",

Re: subsystem_unregister() breakage in -mm

[Cornelia Huck]

On Tue, 25 Sep 2007 19:41:21 -0700,
Greg KH <greg@kroah.com> wrote:

> Ok, here's a patch that fixes this for me.  It should go on top of the
> -mm tree.  Let me know of this works or not.

Looks sane to me.

> In the end, we need to dynamically allocate these subsystems to properly
> fix this.  In looking through the tree, there really isn't that many of
> them, so I'll try to knock that out later this week.

What they need at least is a dynamically allocated k_name. Same goes
for all those objects that use set_kset_name(), and there are quite a
few of those...

> 
> 
> thanks,
> 
> greg k-h
> 
> ---
>  lib/kobject.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -487,11 +487,11 @@ void kobject_cleanup(struct kobject * ko
>  	struct kobject * parent = kobj->parent;
> 
>  	pr_debug("kobject %s: cleaning up\n",kobject_name(kobj));
> -	kfree(kobj->k_name);
> -	kobj->k_name = NULL;
> -	if (t && t->release)
> +	if (t && t->release) {
>  		t->release(kobj);
> -	else
> +		kfree(kobj->k_name);
> +		kobj->k_name = NULL;
> +	} else
>  		pr_debug("kobject '%s' does not have a release() function, "
>  			"if this is not a directory kobject, it is broken "
>  			"and must be fixed.\n",

Re: subsystem_unregister() breakage in -mm

[Greg KH]

On Wed, Sep 26, 2007 at 09:21:56AM +0200, Cornelia Huck wrote:
> On Tue, 25 Sep 2007 19:41:21 -0700,
> Greg KH <greg@kroah.com> wrote:
> 
> > Ok, here's a patch that fixes this for me.  It should go on top of the
> > -mm tree.  Let me know of this works or not.
> 
> Looks sane to me.
> 
> > In the end, we need to dynamically allocate these subsystems to properly
> > fix this.  In looking through the tree, there really isn't that many of
> > them, so I'll try to knock that out later this week.
> 
> What they need at least is a dynamically allocated k_name.

Yes :(

> Same goes
> for all those objects that use set_kset_name(), and there are quite a
> few of those...

Almost all of those are the stupid sysdev_class and sys_device stuff.  I
_really_ want to get rid of those as they should be a "normal"
device/driver/class.  I think Adam Belay has some patches he is working
on to get rid of these, but I do not know the status of them.

Anyway, they are static structures, so my fix will keep them from
oopsing.

thanks,

greg k-h

Re: subsystem_unregister() breakage in -mm

[Miklos Szeredi]

> > BUG: failure at mm/slab.c:591/page_get_cache()!
> > Kernel panic - not syncing: BUG!
> 
> Ok, here's a patch that fixes this for me.  It should go on top of the
> -mm tree.  Let me know of this works or not.

Yes, it works for me too.  Thanks.

Miklos

Re: subsystem_unregister() breakage in -mm

[Greg KH]

On Wed, Sep 26, 2007 at 10:17:45AM +0200, Miklos Szeredi wrote:
> > > BUG: failure at mm/slab.c:591/page_get_cache()!
> > > Kernel panic - not syncing: BUG!
> > 
> > Ok, here's a patch that fixes this for me.  It should go on top of the
> > -mm tree.  Let me know of this works or not.
> 
> Yes, it works for me too.  Thanks.

Great, thanks for testing and letting me know.  If only all bug reports
were this easy :)

thanks,

greg k-h