sysfs-related oops during module unload (2.6.16-rc2)

sysfs-related oops during module unload (2.6.16-rc2)

[Nathan Lynch]

If the refcnt attribute of a module is open when the module is
unloaded, we get an oops when the file is closed.  I used ide_cd for
this report but I don't think the oops is caused by the driver itself.
This bug seems to be restricted to the /sys/module hierarchy; it
doesn't happen with /sys/class etc.

I suspect it's an extra put or a missing get somewhere, but the fix
isn't obvious to me after looking at it for a little while, so I'm
punting.

I'm pretty sure this happens with 2.6.15; I can double-check if
needed.


Testcase:

#!/bin/sh

tail -f /sys/module/ide_cd/refcnt > /dev/null &

sleep 1

modprobe -r ide_cd

sleep 2

kill $!


Messages and oops with CONFIG_DEBUG_KOBJECT=y, starting from the
time of modprobe -r:

 kobject iosched: unregistering
 kobject_uevent
 kobject iosched: cleaning up
 kobject queue: unregistering
 kobject_uevent
 kobject queue: cleaning up
 kobject_uevent
 fill_kobj_path: path = '/block/hdc'
 fill_kobj_path: path = '/devices/pci0000:00/0000:00:1f.1/ide1/1.0'
 kobject hdc: cleaning up
 kobject ide-cdrom: unregistering
 kobject_uevent
 fill_kobj_path: path = '/bus/ide/drivers/ide-cdrom'
 kobject ide-cdrom: cleaning up
 kobject ide_cd: unregistering
 kobject_uevent
 fill_kobj_path: path = '/module/ide_cd'
 Uniform CD-ROM driver unloaded
 kobject cdrom: unregistering
 kobject_uevent
 fill_kobj_path: path = '/module/cdrom'
 kobject cdrom: cleaning up
 Unable to handle kernel paging request at virtual address c0f62b60
  printing eip:
 781af1f8
 *pde = 45214067
 Oops: 0002 [#1]
 SMP 
 Modules linked in: speedstep_lib cpufreq_userspace cpufreq_stats freq_table cpufreq_powersave cpufreq_ondemand cpufreq_conservative nfsd exportfs lockd sunrpc autofs4 video container button battery ac floppy rtc pcspkr tsdev 3c59x snd_cs46xx gameport snd_rawmidi snd_seq_device snd_ac97_codec snd_ac97_bus snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd soundcore snd_page_alloc 8139cp usbhid 8139too mii uhci_hcd usbcore hw_random intel_agp agpgart psmouse parport_pc lp parport jfs ext3 jbd unix thermal processor fan
 CPU:    1
 EIP:    0060:[<781af1f8>]    Not tainted VLI
 EFLAGS: 00010216   (2.6.16-rc2 #2) 
 EIP is at kref_put+0x37/0x57
 eax: c0f62b60   ebx: c0f62b60   ecx: b839fc18   edx: 781aeb38
 esi: 781aeb38   edi: b13c744c   ebp: aff82e0c   esp: aff82e04
 ds: 007b   es: 007b   ss: 0068
 Process tail (pid: 5536, threadinfo=aff82000 task=af6b8bd0)
 Stack: <0>00000000 c0f62b48 aff82e14 781aeb59 aff82e28 7818c41d 00000010 bb8bb840 
        af7a16e8 aff82e48 781575b1 bb8bb840 bff90e38 b839fc18 af7a16e8 bd8cea1c 
        00000000 aff82e50 781574fb aff82e64 7815616a 00000001 0000000c b13c757c 
 Call Trace:
  [<78103a68>] show_stack_log_lvl+0xa8/0xb0
  [<78103ba0>] show_registers+0x10a/0x174
  [<78103d7a>] die+0xfb/0x16f
  [<78295744>] do_page_fault+0x370/0x522
  [<78103713>] error_code+0x4f/0x54
  [<781aeb59>] kobject_put+0x14/0x16
  [<7818c41d>] sysfs_release+0x2c/0x76
  [<781575b1>] __fput+0xb4/0x151
  [<781574fb>] fput+0x17/0x19
  [<7815616a>] filp_close+0x4e/0x58
  [<7811fb88>] close_files+0x57/0x67
  [<7811fbde>] put_files_struct+0x18/0x3e
  [<7812059f>] do_exit+0x1bc/0x35b
  [<78120803>] sys_exit_group+0x0/0x11
  [<7812847c>] get_signal_to_deliver+0x253/0x27b
  [<78102a2c>] do_signal+0x5f/0x10a
  [<78102b04>] do_notify_resume+0x2d/0x3d
  [<78102ca2>] work_notifysig+0x13/0x19
 Code: 04 6a 34 eb 0a 81 fa ed 41 15 78 75 1e 6a 35 68 1c 17 2b 78 68 34 cd 29 78 68 53 47 2a 78 e8 6e f0 f6 ff e8 8a 48 f5 ff 83 c4 10 <f0> ff 0b 0f 94 c0 31 d2 84 c0 74 09 89 d8 ff d6 ba 01 00 00 00 
  <1>Fixing recursive fault but reboot is needed!

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Greg KH]

On Sat, Feb 11, 2006 at 04:03:53PM -0600, Nathan Lynch wrote:
> If the refcnt attribute of a module is open when the module is
> unloaded, we get an oops when the file is closed.  I used ide_cd for
> this report but I don't think the oops is caused by the driver itself.
> This bug seems to be restricted to the /sys/module hierarchy; it
> doesn't happen with /sys/class etc.
> 
> I suspect it's an extra put or a missing get somewhere, but the fix
> isn't obvious to me after looking at it for a little while, so I'm
> punting.
> 
> I'm pretty sure this happens with 2.6.15; I can double-check if
> needed.

Ugh, we aren't setting the owner of these fields properly, good catch.

Does the patch below (built tested only), solve this for you?

thanks,

greg k-h

-----------------

 kernel/module.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- gregkh-2.6.orig/kernel/module.c	2006-01-17 08:27:49.000000000 -0800
+++ gregkh-2.6/kernel/module.c	2006-02-11 14:44:19.000000000 -0800
@@ -1085,8 +1085,10 @@
 
 	for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
 		if (!attr->test ||
-		    (attr->test && attr->test(mod)))
+		    (attr->test && attr->test(mod))) {
+			attr->attr.owner = mod;
 			error = sysfs_create_file(&mod->mkobj.kobj,&attr->attr);
+		}
 	}
 	return error;
 }

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Nathan Lynch]

Greg KH wrote:
> On Sat, Feb 11, 2006 at 04:03:53PM -0600, Nathan Lynch wrote:
> > If the refcnt attribute of a module is open when the module is
> > unloaded, we get an oops when the file is closed.  I used ide_cd for
> > this report but I don't think the oops is caused by the driver itself.
> > This bug seems to be restricted to the /sys/module hierarchy; it
> > doesn't happen with /sys/class etc.
> > 
> > I suspect it's an extra put or a missing get somewhere, but the fix
> > isn't obvious to me after looking at it for a little while, so I'm
> > punting.
> > 
> > I'm pretty sure this happens with 2.6.15; I can double-check if
> > needed.
> 
> Ugh, we aren't setting the owner of these fields properly, good catch.
> 
> Does the patch below (built tested only), solve this for you?

Thanks, but no, I get the same oops.  The refcnt attribute isn't part
of the modinfo_attrs array.


>  kernel/module.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> --- gregkh-2.6.orig/kernel/module.c	2006-01-17 08:27:49.000000000 -0800
> +++ gregkh-2.6/kernel/module.c	2006-02-11 14:44:19.000000000 -0800
> @@ -1085,8 +1085,10 @@
>  
>  	for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
>  		if (!attr->test ||
> -		    (attr->test && attr->test(mod)))
> +		    (attr->test && attr->test(mod))) {
> +			attr->attr.owner = mod;
>  			error = sysfs_create_file(&mod->mkobj.kobj,&attr->attr);
> +		}
>  	}
>  	return error;
>  }

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Greg KH]

On Sat, Feb 11, 2006 at 11:27:52PM -0600, Nathan Lynch wrote:
> Greg KH wrote:
> > On Sat, Feb 11, 2006 at 04:03:53PM -0600, Nathan Lynch wrote:
> > > If the refcnt attribute of a module is open when the module is
> > > unloaded, we get an oops when the file is closed.  I used ide_cd for
> > > this report but I don't think the oops is caused by the driver itself.
> > > This bug seems to be restricted to the /sys/module hierarchy; it
> > > doesn't happen with /sys/class etc.
> > > 
> > > I suspect it's an extra put or a missing get somewhere, but the fix
> > > isn't obvious to me after looking at it for a little while, so I'm
> > > punting.
> > > 
> > > I'm pretty sure this happens with 2.6.15; I can double-check if
> > > needed.
> > 
> > Ugh, we aren't setting the owner of these fields properly, good catch.
> > 
> > Does the patch below (built tested only), solve this for you?
> 
> Thanks, but no, I get the same oops.  The refcnt attribute isn't part
> of the modinfo_attrs array.

Ah, crap, you're right.  We really need to dynamically create these
attributes for every module to get the owner right.  That will be a
bigger patch that I'll work on on Monday...

thanks for testing,

greg k-h

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Greg KH]

On Sat, Feb 11, 2006 at 09:38:49PM -0800, Greg KH wrote:
> On Sat, Feb 11, 2006 at 11:27:52PM -0600, Nathan Lynch wrote:
> > Greg KH wrote:
> > > On Sat, Feb 11, 2006 at 04:03:53PM -0600, Nathan Lynch wrote:
> > > > If the refcnt attribute of a module is open when the module is
> > > > unloaded, we get an oops when the file is closed.  I used ide_cd for
> > > > this report but I don't think the oops is caused by the driver itself.
> > > > This bug seems to be restricted to the /sys/module hierarchy; it
> > > > doesn't happen with /sys/class etc.
> > > > 
> > > > I suspect it's an extra put or a missing get somewhere, but the fix
> > > > isn't obvious to me after looking at it for a little while, so I'm
> > > > punting.
> > > > 
> > > > I'm pretty sure this happens with 2.6.15; I can double-check if
> > > > needed.
> > > 
> > > Ugh, we aren't setting the owner of these fields properly, good catch.
> > > 
> > > Does the patch below (built tested only), solve this for you?
> > 
> > Thanks, but no, I get the same oops.  The refcnt attribute isn't part
> > of the modinfo_attrs array.
> 
> Ah, crap, you're right.  We really need to dynamically create these
> attributes for every module to get the owner right.  That will be a
> bigger patch that I'll work on on Monday...

Ok, turns out the code was trying to increment the module reference
count correctly, but it wasn't working right at all.  And we were not
showing a few things in sysfs if module unload was not selected, which
isn't right.

So here's a patch that fixes all of this, and your original problem.
Bonus is that it actually removes more code than it adds :)

Can you test it out to verify that it works for you?

thanks,

greg k-h


---
 include/linux/module.h |    1 
 kernel/module.c        |   77 +++++++++++++++++++------------------------------
 kernel/params.c        |   10 ------
 3 files changed, 32 insertions(+), 56 deletions(-)

--- gregkh-2.6.orig/include/linux/module.h
+++ gregkh-2.6/include/linux/module.h
@@ -242,6 +242,7 @@ struct module
 	/* Sysfs stuff. */
 	struct module_kobject mkobj;
 	struct module_param_attrs *param_attrs;
+	struct module_attribute *modinfo_attrs;
 	const char *version;
 	const char *srcversion;
 
--- gregkh-2.6.orig/kernel/module.c
+++ gregkh-2.6/kernel/module.c
@@ -379,7 +379,6 @@ static inline void percpu_modcopy(void *
 }
 #endif /* CONFIG_SMP */
 
-#ifdef CONFIG_MODULE_UNLOAD
 #define MODINFO_ATTR(field)	\
 static void setup_modinfo_##field(struct module *mod, const char *s)  \
 {                                                                     \
@@ -411,12 +410,7 @@ static struct module_attribute modinfo_#
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
 
-static struct module_attribute *modinfo_attrs[] = {
-	&modinfo_version,
-	&modinfo_srcversion,
-	NULL,
-};
-
+#ifdef CONFIG_MODULE_UNLOAD
 /* Init the unload section of the module. */
 static void module_unload_init(struct module *mod)
 {
@@ -731,6 +725,15 @@ static inline void module_unload_init(st
 }
 #endif /* CONFIG_MODULE_UNLOAD */
 
+static struct module_attribute *modinfo_attrs[] = {
+	&modinfo_version,
+	&modinfo_srcversion,
+#ifdef CONFIG_MODULE_UNLOAD
+	&refcnt,
+#endif
+	NULL,
+};
+
 #ifdef CONFIG_OBSOLETE_MODPARM
 /* Bounds checking done below */
 static int obsparm_copy_string(const char *val, struct kernel_param *kp)
@@ -1056,37 +1059,28 @@ static inline void remove_sect_attrs(str
 }
 #endif /* CONFIG_KALLSYMS */
 
-
-#ifdef CONFIG_MODULE_UNLOAD
-static inline int module_add_refcnt_attr(struct module *mod)
-{
-	return sysfs_create_file(&mod->mkobj.kobj, &refcnt.attr);
-}
-static void module_remove_refcnt_attr(struct module *mod)
-{
-	return sysfs_remove_file(&mod->mkobj.kobj, &refcnt.attr);
-}
-#else
-static inline int module_add_refcnt_attr(struct module *mod)
-{
-	return 0;
-}
-static void module_remove_refcnt_attr(struct module *mod)
-{
-}
-#endif
-
-#ifdef CONFIG_MODULE_UNLOAD
 static int module_add_modinfo_attrs(struct module *mod)
 {
 	struct module_attribute *attr;
+	struct module_attribute *temp_attr;
 	int error = 0;
 	int i;
 
+	mod->modinfo_attrs = kzalloc((sizeof(struct module_attribute) *
+					(ARRAY_SIZE(modinfo_attrs) + 1)),
+					GFP_KERNEL);
+	if (!mod->modinfo_attrs)
+		return -ENOMEM;
+
+	temp_attr = mod->modinfo_attrs;
 	for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
 		if (!attr->test ||
-		    (attr->test && attr->test(mod)))
-			error = sysfs_create_file(&mod->mkobj.kobj,&attr->attr);
+		    (attr->test && attr->test(mod))) {
+			memcpy(temp_attr, attr, sizeof(*temp_attr));
+			temp_attr->attr.owner = mod;
+			error = sysfs_create_file(&mod->mkobj.kobj,&temp_attr->attr);
+			++temp_attr;
+		}
 	}
 	return error;
 }
@@ -1096,12 +1090,16 @@ static void module_remove_modinfo_attrs(
 	struct module_attribute *attr;
 	int i;
 
-	for (i = 0; (attr = modinfo_attrs[i]); i++) {
+	for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
+		/* pick a field to test for end of list */
+		if (!attr->attr.name)
+			break;
 		sysfs_remove_file(&mod->mkobj.kobj,&attr->attr);
-		attr->free(mod);
+		if (attr->free)
+			attr->free(mod);
 	}
+	kfree(mod->modinfo_attrs);
 }
-#endif
 
 static int mod_sysfs_setup(struct module *mod,
 			   struct kernel_param *kparam,
@@ -1119,19 +1117,13 @@ static int mod_sysfs_setup(struct module
 	if (err)
 		goto out;
 
-	err = module_add_refcnt_attr(mod);
-	if (err)
-		goto out_unreg;
-
 	err = module_param_sysfs_setup(mod, kparam, num_params);
 	if (err)
 		goto out_unreg;
 
-#ifdef CONFIG_MODULE_UNLOAD
 	err = module_add_modinfo_attrs(mod);
 	if (err)
 		goto out_unreg;
-#endif
 
 	return 0;
 
@@ -1143,10 +1135,7 @@ out:
 
 static void mod_kobject_remove(struct module *mod)
 {
-#ifdef CONFIG_MODULE_UNLOAD
 	module_remove_modinfo_attrs(mod);
-#endif
-	module_remove_refcnt_attr(mod);
 	module_param_sysfs_remove(mod);
 
 	kobject_unregister(&mod->mkobj.kobj);
@@ -1424,7 +1413,6 @@ static char *get_modinfo(Elf_Shdr *sechd
 	return NULL;
 }
 
-#ifdef CONFIG_MODULE_UNLOAD
 static void setup_modinfo(struct module *mod, Elf_Shdr *sechdrs,
 			  unsigned int infoindex)
 {
@@ -1439,7 +1427,6 @@ static void setup_modinfo(struct module 
 						attr->attr.name));
 	}
 }
-#endif
 
 #ifdef CONFIG_KALLSYMS
 int is_exported(const char *name, const struct module *mod)
@@ -1755,10 +1742,8 @@ static struct module *load_module(void _
 	if (strcmp(mod->name, "driverloader") == 0)
 		add_taint(TAINT_PROPRIETARY_MODULE);
 
-#ifdef CONFIG_MODULE_UNLOAD
 	/* Set up MODINFO_ATTR fields */
 	setup_modinfo(mod, sechdrs, infoindex);
-#endif
 
 	/* Fix up syms, so that st_value is a pointer to location. */
 	err = simplify_symbols(sechdrs, symindex, strtab, versindex, pcpuindex,
--- gregkh-2.6.orig/kernel/params.c
+++ gregkh-2.6/kernel/params.c
@@ -638,13 +638,8 @@ static ssize_t module_attr_show(struct k
 	if (!attribute->show)
 		return -EIO;
 
-	if (!try_module_get(mk->mod))
-		return -ENODEV;
-
 	ret = attribute->show(attribute, mk->mod, buf);
 
-	module_put(mk->mod);
-
 	return ret;
 }
 
@@ -662,13 +657,8 @@ static ssize_t module_attr_store(struct 
 	if (!attribute->store)
 		return -EIO;
 
-	if (!try_module_get(mk->mod))
-		return -ENODEV;
-
 	ret = attribute->store(attribute, mk->mod, buf, len);
 
-	module_put(mk->mod);
-
 	return ret;
 }
 

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Greg KH]

On Thu, Feb 16, 2006 at 10:53:45PM -0500, Dmitry Torokhov wrote:
> On Thursday 16 February 2006 16:50, Greg KH wrote:
> > +???????mod->modinfo_attrs = kzalloc((sizeof(struct module_attribute) *
> > +???????????????????????????????????????(ARRAY_SIZE(modinfo_attrs) + 1)),
> > +???????????????????????????????????????GFP_KERNEL);
> > 
> 
> kcalloc() perhaps? Here you explecitely create n elements of a given size...

Heh, sure, the one time I actually are creating n elements :)

I'll go change that.

thanks for pointing it out.

greg k-h

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Nathan Lynch]

Greg KH wrote:
> On Sat, Feb 11, 2006 at 09:38:49PM -0800, Greg KH wrote:
> > On Sat, Feb 11, 2006 at 11:27:52PM -0600, Nathan Lynch wrote:
> > > Greg KH wrote:
> > > > On Sat, Feb 11, 2006 at 04:03:53PM -0600, Nathan Lynch wrote:
> > > > > If the refcnt attribute of a module is open when the module is
> > > > > unloaded, we get an oops when the file is closed.  I used ide_cd for
> > > > > this report but I don't think the oops is caused by the driver itself.
> > > > > This bug seems to be restricted to the /sys/module hierarchy; it
> > > > > doesn't happen with /sys/class etc.

<snip>

> 
> Ok, turns out the code was trying to increment the module reference
> count correctly, but it wasn't working right at all.  And we were not
> showing a few things in sysfs if module unload was not selected, which
> isn't right.
> 
> So here's a patch that fixes all of this, and your original problem.
> Bonus is that it actually removes more code than it adds :)
> 
> Can you test it out to verify that it works for you?

Sorry for the delay.

Tested against 2.6.16-rc4-ish, and it seems to do the right thing --
modprobe -r says the module is busy while the refcnt attribute is
open.  The module is allowed to unload once the file is closed.

I didn't verify the other stuff your patch changes, though.

Thanks.


Nathan

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Greg KH]

On Sat, Feb 18, 2006 at 06:47:51PM -0600, Nathan Lynch wrote:
> Sorry for the delay.
> 
> Tested against 2.6.16-rc4-ish, and it seems to do the right thing --
> modprobe -r says the module is busy while the refcnt attribute is
> open.  The module is allowed to unload once the file is closed.

Great, thanks for trying it out and letting me know.

> I didn't verify the other stuff your patch changes, though.

Heh, well if it all still works, that's pretty much proof that the other
stuff was correct too :)

thanks,

greg k-h

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Nathan Lynch]

Greg KH wrote:
> On Sat, Feb 18, 2006 at 06:47:51PM -0600, Nathan Lynch wrote:
> > Sorry for the delay.
> > 
> > Tested against 2.6.16-rc4-ish, and it seems to do the right thing --
> > modprobe -r says the module is busy while the refcnt attribute is
> > open.  The module is allowed to unload once the file is closed.
> 
> Great, thanks for trying it out and letting me know.

Sure -- do you plan to push this for 2.6.16?

The reason I ask is that the refcnt attribute is world-readable, so a
malicious or silly user can keep the file open until an unwitting
superuser unloads a module...

Far-fetched, I suppose, but I just wanted to make this scenario clear.


Nathan

Re: sysfs-related oops during module unload (2.6.16-rc2)

[Greg KH]

On Mon, Feb 20, 2006 at 11:50:39PM -0600, Nathan Lynch wrote:
> Greg KH wrote:
> > On Sat, Feb 18, 2006 at 06:47:51PM -0600, Nathan Lynch wrote:
> > > Sorry for the delay.
> > > 
> > > Tested against 2.6.16-rc4-ish, and it seems to do the right thing --
> > > modprobe -r says the module is busy while the refcnt attribute is
> > > open.  The module is allowed to unload once the file is closed.
> > 
> > Great, thanks for trying it out and letting me know.
> 
> Sure -- do you plan to push this for 2.6.16?

No.

> The reason I ask is that the refcnt attribute is world-readable, so a
> malicious or silly user can keep the file open until an unwitting
> superuser unloads a module...
> 
> Far-fetched, I suppose, but I just wanted to make this scenario clear.

It's been like this for a number of kernel versions now, and module
unloading is a rare thing to happen (no tools do it automatically.)  So
I don't think it's worth 2.6.16 material.  Let it sit in -mm for a bit
to verify that I didn't break anything else, and I'll send it in for
2.6.17.

thanks,

greg k-h