lkml.org 
[lkml]   [2010]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [Regression] Crash in load_module() while freeing args
Date
On Wednesday 26 May 2010, Rusty Russell wrote:
> On Wed, 26 May 2010 05:30:58 pm Rusty Russell wrote:
> > On Wed, 26 May 2010 09:17:32 am Linus Torvalds wrote:
> > >
> > > On Wed, 26 May 2010, Rafael J. Wysocki wrote:
> > > >
> > > > I'm not able to reproduce the issue with the following commit reverted:
> > > >
> > > > commit 480b02df3aa9f07d1c7df0cd8be7a5ca73893455
> > > > Author: Rusty Russell <rusty@rustcorp.com.au>
> > > > Date: Wed May 19 17:33:39 2010 -0600
> > > >
> > > > module: drop the lock while waiting for module to complete initialization.
> > >
> > > Hmm. That does seem to be buggy. We can't just drop and re-take the lock:
> > > that may make sense _internally_ as far as resolve_symbol() itself is
> > > concerned, but the caller will its own local variables, and some of those
> > > will no longer be valid if the lock was dropped.
> >
> > Well, yes, obviously I missed something :( I'll look at it tonight after
> > Arabella is asleep.
>
> 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?

Since the commit has been reverted, do you still want me to test this patch?
Quite frankly I'd prefer to test a complete replacement for that commit on top
of current -git.

Rafael


> (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);
>
>



\
 
 \ /
  Last update: 2010-05-27 00:57    [W:0.150 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site