On Wed, 26 May 2010 05:30:58 pm Rusty Russell wrote:
See if you can spot it (I acked the patch, so I can't point fingers):
free_core:
module_free(mod, mod->module_core);
/* mod will be freed with core. Don't access it beyond this line! */
free_percpu:
percpu_modfree(mod);
Only a year after Masami fixed that and added the comment, too :(
I suspect that the increased parallelism enabled by this patch uncovered this
bug. Does this fix it?
(Side note: the locking should be simplified. No code before simplify_symbols
actually needs the lock, so we should grab it just for that, then again at the
end. We use kobjects to protect us from multiple loads as a side-effect, but
we should move that registration to the end).
Subject: module: fix reference to mod->percpu after freeing module.
The comment about the mod being freed is self-explanatory, but neither
Tejun nor I read it. This bug was introduced in 259354deaa, after it
had previously been fixed in 6e2b75740b. How embarrassing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Tejun Heo <tj@kernel.org>
Cc: Masami Hiramatsu <mhiramat@redhat.com>
diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2031,6 +2031,7 @@ static noinline struct module *load_modu
long err = 0;
void *ptr = NULL; /* Stops spurious gcc warning */
unsigned long symoffs, stroffs, *strmap;
+ void __percpu *percpu;
mm_segment_t old_fs;
@@ -2175,6 +2176,8 @@ static noinline struct module *load_modu
goto free_mod;
sechdrs[pcpuindex].sh_flags &= ~(unsigned long)SHF_ALLOC;
}
+ /* Keep this around for failure path. */
+ percpu = mod_percpu(mod);
/* Determine total sizes, and put offsets in sh_entsize. For now
this is done generically; there doesn't appear to be any
@@ -2480,7 +2483,7 @@ static noinline struct module *load_modu
module_free(mod, mod->module_core);
/* mod will be freed with core. Don't access it beyond this line! */
free_percpu:
- percpu_modfree(mod);
+ free_percpu(percpu);
free_mod:
kfree(args);
kfree(strmap);
--
