On Mon, Apr 15, 2013 at 11:56:03AM +0930, Rusty Russell wrote:
Veaceslav Falico <vfalico@xxxxxxxxxx> writes:
On Wed, Apr 10, 2013 at 04:47:34PM +0930, Rusty Russell wrote:
That's a bug. We should be cleaning up sysfs before we unlike the
removed module from the list.
Because the same thing applies to ddebug info, which is also keyed by
module name.
Something like this (untested!):
Sorry for the late response - I wanted to test it for a longer time.
Your patch works flawlessly and fixes this race, with just a small
addition, cause otherwise we could BUG() in show_initstate().
Can you apply this patch or should I (re-)send it somehow?
Thank you!
diff --git a/kernel/module.c b/kernel/module.c
index d0afe23..8be6e97 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1063,6 +1063,9 @@ static ssize_t show_initstate(struct module_attribute *mattr,
case MODULE_STATE_GOING:
state = "going";
break;
+ case MODULE_STATE_UNFORMED:
+ state = "unformed";
+ break;
default:
BUG();
}
Prefer to remove from sysfs before marking it unformed, like so:
diff --git a/kernel/module.c b/kernel/module.c
index 2468fda..2e7189f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1862,12 +1862,12 @@ static void free_module(struct module *mod)
{
trace_module_free(mod);
- /* We leave it in list to prevent duplicate loads while we clean
- * up sysfs, ddebug and any other external exposure. */
- mod->state = MODULE_STATE_UNFORMED;
-
mod_sysfs_teardown(mod);
+ /* We leave it in list to prevent duplicate loads, but make sure
+ * that noone uses it while it's being deconstructed. */
+ mod->state = MODULE_STATE_UNFORMED;
+
/* Remove dynamic debug info */
ddebug_remove_module(mod->name);
Here's the updated total patch:
Tested for a day on two reproducers on the latest upstream kernel, with the
recent kobject fix a49b7e82 ("kobject: fix kset_find_obj() race with
concurrent last kobject_put()") - it fixes the issue, no regressions met.
Without the patch (but with that kobject fix) it gives the following, for
parallel modprobe/rmmod of pch_gbe module (it's easier to reproduce on less
powerful boxes, like this one):
[ 103.981925] ------------[ cut here ]------------
[ 103.986902] WARNING: at fs/sysfs/dir.c:536 sysfs_add_one+0xab/0xd0()
[ 103.993606] Hardware name: CrownBay Platform
[ 103.998075] sysfs: cannot create duplicate filename '/module/pch_gbe'
[ 104.004784] Modules linked in: pch_gbe(+) [last unloaded: pch_gbe]
[ 104.011362] Pid: 3021, comm: modprobe Tainted: G W 3.9.0-rc5+ #5
[ 104.018662] Call Trace:
[ 104.021286] [<c103599d>] warn_slowpath_common+0x6d/0xa0
[ 104.026933] [<c1168c8b>] ? sysfs_add_one+0xab/0xd0
[ 104.031986] [<c1168c8b>] ? sysfs_add_one+0xab/0xd0
[ 104.037000] [<c1035a4e>] warn_slowpath_fmt+0x2e/0x30
[ 104.042188] [<c1168c8b>] sysfs_add_one+0xab/0xd0
[ 104.046982] [<c1168dbe>] create_dir+0x5e/0xa0
[ 104.051633] [<c1168e78>] sysfs_create_dir+0x78/0xd0
[ 104.056774] [<c1262bc3>] kobject_add_internal+0x83/0x1f0
[ 104.062351] [<c126daf6>] ? kvasprintf+0x46/0x60
[ 104.067231] [<c1262ebd>] kobject_add_varg+0x2d/0x50
[ 104.072450] [<c1262f07>] kobject_init_and_add+0x27/0x30
[ 104.078075] [<c1089240>] mod_sysfs_setup+0x80/0x540
[ 104.083207] [<c1260851>] ? module_bug_finalize+0x51/0xc0
[ 104.088720] [<c108ab29>] load_module+0x1429/0x18b0
[ 104.093712] [<c1088510>] ? free_notes_attrs+0x40/0x40
[ 104.098973] [<c126c588>] ? _copy_from_user+0x38/0x130
[ 104.104225] [<c108b0c3>] sys_init_module+0x93/0xb0
[ 104.109211] [<c16f22fa>] sysenter_do_call+0x12/0x22
[ 104.114272] ---[ end trace aa239ae9897b5680 ]---
[ 104.119029] ------------[ cut here ]------------
[ 104.123901] WARNING: at lib/kobject.c:196 kobject_add_internal+0x1da/0x1f0()
[ 104.131254] Hardware name: CrownBay Platform
[ 104.135792] kobject_add_internal failed for pch_gbe with -EEXIST, don't try to register things with the same name in the same directory.
[ 104.148539] Modules linked in: pch_gbe(+) [last unloaded: pch_gbe]
[ 104.155256] Pid: 3021, comm: modprobe Tainted: G W 3.9.0-rc5+ #5
[ 104.162437] Call Trace:
[ 104.165057] [<c103599d>] warn_slowpath_common+0x6d/0xa0
[ 104.170715] [<c1262d1a>] ? kobject_add_internal+0x1da/0x1f0
[ 104.176683] [<c1262d1a>] ? kobject_add_internal+0x1da/0x1f0
[ 104.182644] [<c1035a4e>] warn_slowpath_fmt+0x2e/0x30
[ 104.187937] [<c1262d1a>] kobject_add_internal+0x1da/0x1f0
[ 104.193750] [<c1262ebd>] kobject_add_varg+0x2d/0x50
[ 104.198891] [<c1262f07>] kobject_init_and_add+0x27/0x30
[ 104.204489] [<c1089240>] mod_sysfs_setup+0x80/0x540
[ 104.209706] [<c1260851>] ? module_bug_finalize+0x51/0xc0
[ 104.215362] [<c108ab29>] load_module+0x1429/0x18b0
[ 104.220530] [<c1088510>] ? free_notes_attrs+0x40/0x40
[ 104.226001] [<c126c588>] ? _copy_from_user+0x38/0x130
[ 104.231442] [<c108b0c3>] sys_init_module+0x93/0xb0
[ 104.236629] [<c16f22fa>] sysenter_do_call+0x12/0x22
[ 104.241855] ---[ end trace aa239ae9897b5681 ]---
With the patch it worked flawlessly for a day.
diff --git a/kernel/module.c b/kernel/module.c
index 69d2600..2e7189f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1862,12 +1862,12 @@ static void free_module(struct module *mod)
{
trace_module_free(mod);
- /* Delete from various lists */
- mutex_lock(&module_mutex);
- stop_machine(__unlink_module, mod, NULL);
- mutex_unlock(&module_mutex);
mod_sysfs_teardown(mod);
+ /* We leave it in list to prevent duplicate loads, but make sure
+ * that noone uses it while it's being deconstructed. */
+ mod->state = MODULE_STATE_UNFORMED;
+
/* Remove dynamic debug info */
ddebug_remove_module(mod->name);
@@ -1880,6 +1880,11 @@ static void free_module(struct module *mod)
/* Free any allocated parameters. */
destroy_params(mod->kp, mod->num_kp);
+ /* Now we can delete it from the lists */
+ mutex_lock(&module_mutex);
+ stop_machine(__unlink_module, mod, NULL);
+ mutex_unlock(&module_mutex);
+
/* This may be NULL, but that's OK */
unset_module_init_ro_nx(mod);
module_free(mod, mod->module_init);
Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
