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