OpenVZ Forum - RDF feed
type: channel
items: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
items: Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races

[PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: [cc'ing folks whose proc files are affected] kallsyms_lookup() can call module_address_lookup() which iterates over modules list without module_mutex taken. Comment at the top of module_address_lookup() says it's for oops resolution so races are irrelevant, but in some cases it's reachable from regular code: BUG: unable to handle kernel paging request at virtual address f94fb19c printing eip: c0138097 *pde = 33576067 Oops: 0000 [#1] PREEMPT Modules linked in: ohci_hcd af_packet e1000 ehci_hcd uhci_hcd usbcore CPU: 0 EIP: 0060:[<c0138097>] Not tainted VLI EFLAGS: 00010246 (2.6.21-rc3-8b9909ded6922c33c221b105b26917780cfa497d #25) EIP is at module_address_lookup+0x43/0x24c eax: 00000000 ebx: 00000000 ecx: f94fb080 edx: f882e704 esi: 00000000 edi: 00000000 ebp: 00000000 esp: d30b8e78 ds: 007b es: 007b fs: 00d8 gs: 0033 ss: 0068 Process cat (pid: 13274, ti=d30b8000 task=d2b50af0 task.ti=d30b8000) Stack: d30b8f44 d30b8f48 00008001 00000000 c039a23c d30b8ec4 00000000 d30b8f44 d30b8f48 c013876c d30b8f4c 00000000 cf544000 fffffff4 d38ebb7c c01880ff d30b8f4c d30b8ec4 00000246 c0399600 00000001 00000001 00000000 c0399654 Call Trace: [<c013876c>] kallsyms_lookup+0x5e/0x7c [<c01880ff>] proc_pid_wchan+0x38/0x76 [<c01404fe>] __alloc_pages+0x4f/0x2f9 [<c01891c6>] proc_info_read+0x6f/0xa8 [<c015bbfd>] sys_fstat64+0x1e/0x23 [<c01592d4>] vfs_read+0x7d/0xb5 [<c0189157>] proc_info_read+0x0/0xa8 [<c0159621>] sys_read+0x41/0x6a [<c0103e72>] sysenter_past_esp+0x5f/0x99 ======================= Code: 04 8b 51 04 0f 18 02 90 81 3d 4c 8d 39 c0 4c 8d 39 c0 74 3f 8b b9 18 01 00 00 8b 99 10 01 00 00 39 eb 77 07 8d 04 3b 39 c5 72 32 <8b> b1 1c 01 00 00 8b 81 14 01 00 00 39 c5 72 06 01 f0 39 c5 72 EIP: [<c0138097>] module_address_lookup+0x43/0x24c SS:ESP 0068:d30b8e78 Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru> --- fs/proc/base.c | 7 +++++++ kernel/time/timer_list.c | 6 ++++++ kernel/time/timer_stats.c | 6 ++++++ mm/slab.c | 3 +++ 4 files changed, 22 insertions(+) --- a/fs/proc/base.c +++ b/fs…
creator: Alexey Dobriyan
date: 2007-03-16T11:39:01-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: * Alexey Dobriyan <adobriyan@sw.ru> wrote: > [cc'ing folks whose proc files are affected] > > kallsyms_lookup() can call module_address_lookup() which iterates over > modules list without module_mutex taken. Comment at the top of > module_address_lookup() says it's for oops resolution so races are > irrelevant, but in some cases it's reachable from regular code: looking at the problem from another angle: wouldnt this be something that would benefit from freeze_processes()/unfreeze_processes(), and hence no locking would be required? Ingo
creator: Ingo Molnar
date: 2007-03-16T11:51:54-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: Ingo Molnar wrote: > * Alexey Dobriyan <adobriyan@sw.ru> wrote: > >> [cc'ing folks whose proc files are affected] >> >> kallsyms_lookup() can call module_address_lookup() which iterates over >> modules list without module_mutex taken. Comment at the top of >> module_address_lookup() says it's for oops resolution so races are >> irrelevant, but in some cases it's reachable from regular code: > > looking at the problem from another angle: wouldnt this be something > that would benefit from freeze_processes()/unfreeze_processes(), and > hence no locking would be required? I also considered this, but it seemed a little too "blunt" to stop everything (including completely unrelated processes and kernel threads) just to remove a module. How about something like this instead? (just for review) It's a little more intrusive, since the interface for symbol lookup is changed (and it affects the callers), but it makes the caller aware that it should set "safe_to_lock" if possible. This way we avoid exposing module_mutex outside of module.c and avoid returning any data pointer to some data structure that might disappear before the caller tries to use it. Some of these changes are actually cleanups, because the callers where creating a dummy modname variables that they didn't use afterwards. The thing I like the less about this patch is the increase of stack usage on some code paths by about 60 bytes. Anyway, I don't have very strong feelings about this, so if you think it would be better to use freeze_processes()/unfreeze_processes(), I can cook up a patch for that too... -- Paulo Marques - www.grupopie.com "Very funny Scotty. Now beam up my clothes." diffstat: > arch/parisc/kernel/unwind.c | 3 -- > arch/powerpc/xmon/xmon.c | 11 ++++----- > arch/ppc/xmon/xmon.c | 8 +++--- > arch/sh64/kernel/unwind.c | 4 +-- > arch/x86_64/kernel/traps.c | 10 ++++---- > fs/proc/base.c | 3 -- > include/linux/kallsyms.h | 6 +++- > include/linux/module.h | 44 +++++++++++++++++++----------------- > ke…
creator: Paulo Marques
date: 2007-03-16T16:16:10-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: * Paulo Marques <pmarques@grupopie.com> wrote: > >looking at the problem from another angle: wouldnt this be something > >that would benefit from freeze_processes()/unfreeze_processes(), and > >hence no locking would be required? > > I also considered this, but it seemed a little too "blunt" to stop > everything (including completely unrelated processes and kernel > threads) just to remove a module. 'just to remove a module' is very, very rare, on the timescale of most kernel ops. Almost no distro does it. Furthermore, because we want to do CPU-hotplug that way, we really want to make freeze_processes()/unfreeze_processes() 'instantaneous' to the human - and it is that already. (if it isnt in some case we can make it so) Ingo
creator: Ingo Molnar
date: 2007-03-16T16:18:53-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: Ingo Molnar wrote: > * Paulo Marques <pmarques@grupopie.com> wrote: > >>> looking at the problem from another angle: wouldnt this be something >>> that would benefit from freeze_processes()/unfreeze_processes(), and >>> hence no locking would be required? >> I also considered this, but it seemed a little too "blunt" to stop >> everything (including completely unrelated processes and kernel >> threads) just to remove a module. > > 'just to remove a module' is very, very rare, on the timescale of most > kernel ops. Almost no distro does it. Furthermore, because we want to do > CPU-hotplug that way, we really want to make > freeze_processes()/unfreeze_processes() 'instantaneous' to the human - > and it is that already. (if it isnt in some case we can make it so) Ok. I started to look at this approach and realized that module.c already does this: > .... > static int __unlink_module(void *_mod) > { > struct module *mod = _mod; > list_del(&mod->list); > return 0; > } > > /* Free a module, remove from lists, etc (must hold module mutex). */ > static void free_module(struct module *mod) > { > /* Delete from various lists */ > stop_machine_run(__unlink_module, mod, NR_CPUS); > .... However stop_machine_run doesn't seem like the right thing to do, because users of the "modules" list don't seem to do anything to prevent preemption. Am I missing something? Does freeze_processes() / unfreeze_processes() solve this by only freezing processes that have voluntarily scheduled (opposed to just being preempted)? -- Paulo Marques - www.grupopie.com "The Computer made me do it."
creator: Paulo Marques
date: 2007-03-16T17:16:39-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Fri, 16 Mar 2007 17:16:39 +0000 Paulo Marques <pmarques@grupopie.com> wrote: > Does freeze_processes() / unfreeze_processes() solve this by only > freezing processes that have voluntarily scheduled (opposed to just > being preempted)? It goes much much further than that. Those processes need to actually perform an explicit call to try_to_freeze().
creator: Andrew Morton
date: 2007-03-16T18:15:14-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: Andrew Morton wrote: > On Fri, 16 Mar 2007 17:16:39 +0000 Paulo Marques <pmarques@grupopie.com> wrote: > >> Does freeze_processes() / unfreeze_processes() solve this by only >> freezing processes that have voluntarily scheduled (opposed to just >> being preempted)? > > It goes much much further than that. Those processes need to actually > perform an explicit call to try_to_freeze(). Ok, I've just done a few tests with the attached patch. It basically creates a freeze_machine_run function that is equivalent in interface to stop_machine_run, but uses freeze_processes / thaw_processes to stop the machine. This is more of a proof of concept than an actual patch. At the very least "freeze_machine_run" should be moved to kernel/power/process.c and declared at include/linux/freezer.h so that it could be treated as a more general purpose function and not something that is module specific. Anyway, I then tested it by running a modprobe/rmmod loop while running a "cat /proc/kallsyms" loop. On the first run I forgot to remove the mutex_lock(module_mutex) from the /proc/kallsyms read path and the freezer was unable to freeze the "cat" process that was waiting for the same mutex that the freezer process was holding :P After removing the module_mutex locking from "module_get_kallsym" everything was going fine (at least I got no oopses) until I got this: kernel: Stopping user space processes timed out after 20 seconds (1 tasks refusing to freeze): kernel: kbluetoothd kernel: Restarting tasks ... <4> Strange, kseriod not stopped kernel: Strange, pdflush not stopped kernel: Strange, pdflush not stopped kernel: Strange, kswapd0 not stopped kernel: Strange, cifsoplockd not stopped kernel: Strange, cifsdnotifyd not stopped kernel: Strange, jfsIO not stopped kernel: Strange, jfsCommit not stopped kernel: Strange, jfsCommit not stopped kernel: Strange, jfsSync not stopped kernel: Strange, xfslogd/0 not stopped kernel: Strange, xfslogd/1 not stopped kernel: Strange, xfsdatad/0 not stoppe…
creator: Paulo Marques
date: 2007-03-16T20:27:29-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Fri, 16 Mar 2007 20:27:29 +0000 Paulo Marques <pmarques@grupopie.com> wrote: > Andrew Morton wrote: > > On Fri, 16 Mar 2007 17:16:39 +0000 Paulo Marques <pmarques@grupopie.com> wrote: > > > >> Does freeze_processes() / unfreeze_processes() solve this by only > >> freezing processes that have voluntarily scheduled (opposed to just > >> being preempted)? > > > > It goes much much further than that. Those processes need to actually > > perform an explicit call to try_to_freeze(). > > Ok, I've just done a few tests with the attached patch. It basically > creates a freeze_machine_run function that is equivalent in interface to > stop_machine_run, but uses freeze_processes / thaw_processes to stop the > machine. > > This is more of a proof of concept than an actual patch. At the very > least "freeze_machine_run" should be moved to kernel/power/process.c and > declared at include/linux/freezer.h so that it could be treated as a > more general purpose function and not something that is module specific. OK. > Anyway, I then tested it by running a modprobe/rmmod loop while running > a "cat /proc/kallsyms" loop. > > On the first run I forgot to remove the mutex_lock(module_mutex) from > the /proc/kallsyms read path and the freezer was unable to freeze the > "cat" process that was waiting for the same mutex that the freezer > process was holding :P > > After removing the module_mutex locking from "module_get_kallsym" > everything was going fine (at least I got no oopses) until I got this: > > kernel: Stopping user space processes timed out after 20 seconds (1 > tasks refusing to freeze): > kernel: kbluetoothd > kernel: Restarting tasks ... <4> Strange, kseriod not stopped > kernel: Strange, pdflush not stopped > kernel: Strange, pdflush not stopped > kernel: Strange, kswapd0 not stopped > kernel: Strange, cifsoplockd not stopped > kernel: Strange, cifsdnotifyd not stopped > kernel: Strange, jfsIO not stopped > kernel: Strange, jfsCommit not stopped > kernel: Strange, jfsComm…
creator: Andrew Morton
date: 2007-03-16T20:49:49-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Fri, 2007-03-16 at 14:44 +0300, Alexey Dobriyan wrote: > [cc'ing folks whose proc files are affected] > > kallsyms_lookup() can call module_address_lookup() which iterates over > modules list without module_mutex taken. Comment at the top of > module_address_lookup() says it's for oops resolution so races are > irrelevant, but in some cases it's reachable from regular code: Yes, this changed somewhere along the way. I prefer keeping the lock internal as much as possible, and have the crash code use an __ variant of the function. Note also that it might be an idea to have less-powerful accessors than kallsyms_lookup... Thanks! Rusty.
creator: Rusty Russell
date: 2007-03-17T09:32:51-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Fri, 2007-03-16 at 12:51 +0100, Ingo Molnar wrote: > * Alexey Dobriyan <adobriyan@sw.ru> wrote: > > > [cc'ing folks whose proc files are affected] > > > > kallsyms_lookup() can call module_address_lookup() which iterates over > > modules list without module_mutex taken. Comment at the top of > > module_address_lookup() says it's for oops resolution so races are > > irrelevant, but in some cases it's reachable from regular code: > > looking at the problem from another angle: wouldnt this be something > that would benefit from freeze_processes()/unfreeze_processes(), and > hence no locking would be required? Actually, the list manipulation is done with stop_machine for this reason. Alexey, is preempt enabled in your kernel? Rusty.
creator: Rusty Russell
date: 2007-03-17T09:37:18-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Fri, 2007-03-16 at 12:49 -0800, Andrew Morton wrote: > > Ok, I've just done a few tests with the attached patch. It basically > > creates a freeze_machine_run function that is equivalent in interface to > > stop_machine_run, but uses freeze_processes / thaw_processes to stop the > > machine. I've never been convinced that the freezer was a good idea. stop_machine is a damn big hammer, but it works. Rusty.
creator: Rusty Russell
date: 2007-03-17T10:36:33-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Fri, Mar 16, 2007 at 08:27:29PM +0000, Paulo Marques wrote: > Andrew Morton wrote: > >On Fri, 16 Mar 2007 17:16:39 +0000 Paulo Marques <pmarques@grupopie.com> > >wrote: > > > >>Does freeze_processes() / unfreeze_processes() solve this by only > >>freezing processes that have voluntarily scheduled (opposed to just > >>being preempted)? > > > >It goes much much further than that. Those processes need to actually > >perform an explicit call to try_to_freeze(). > > Ok, I've just done a few tests with the attached patch. It basically > creates a freeze_machine_run function that is equivalent in interface to > stop_machine_run, but uses freeze_processes / thaw_processes to stop the > machine. > > This is more of a proof of concept than an actual patch. At the very > least "freeze_machine_run" should be moved to kernel/power/process.c and > declared at include/linux/freezer.h so that it could be treated as a > more general purpose function and not something that is module specific. > > Anyway, I then tested it by running a modprobe/rmmod loop while running > a "cat /proc/kallsyms" loop. > > On the first run I forgot to remove the mutex_lock(module_mutex) from > the /proc/kallsyms read path and the freezer was unable to freeze the > "cat" process that was waiting for the same mutex that the freezer > process was holding :P > > After removing the module_mutex locking from "module_get_kallsym" > everything was going fine (at least I got no oopses) until I got this: > > kernel: Stopping user space processes timed out after 20 seconds (1 > tasks refusing to freeze): > kernel: kbluetoothd > kernel: Restarting tasks ... <4> Strange, kseriod not stopped > kernel: Strange, pdflush not stopped > kernel: Strange, pdflush not stopped > kernel: Strange, kswapd0 not stopped > kernel: Strange, cifsoplockd not stopped > kernel: Strange, cifsdnotifyd not stopped > kernel: Strange, jfsIO not stopped > kernel: Strange, jfsCommit not stopped > kernel: Strange, jfsCommit not stopped > kerne…
creator: Alexey Dobriyan
date: 2007-03-19T09:50:26-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Sat, Mar 17, 2007 at 08:37:18PM +1100, Rusty Russell wrote: > On Fri, 2007-03-16 at 12:51 +0100, Ingo Molnar wrote: > > * Alexey Dobriyan <adobriyan@sw.ru> wrote: > > > > > [cc'ing folks whose proc files are affected] > > > > > > kallsyms_lookup() can call module_address_lookup() which iterates over > > > modules list without module_mutex taken. Comment at the top of > > > module_address_lookup() says it's for oops resolution so races are > > > irrelevant, but in some cases it's reachable from regular code: > > > > looking at the problem from another angle: wouldnt this be something > > that would benefit from freeze_processes()/unfreeze_processes(), and > > hence no locking would be required? > > Actually, the list manipulation is done with stop_machine for this > reason. mmm, my changelog is slightly narrow than it should be. Non-emergency code is traversing modules list. It finds "struct module *". module is removed. "struct module *" is now meaningless, but still dereferenced. How would all this refrigerator stuff would help? It wouldn't, Non-emergency code is traversing modules list. It finds "struct module *". Everything is freezed. Module is removed. Everything is unfreezed. "struct module *" is now meaningless, but still dereferenced. > Alexey, is preempt enabled in your kernel? Yes. FWIW, CONFIG_PREEMPT=y CONFIG_PREEMPT_BKL=y CONFIG_DEBUG_PREEMPT=y I very much agree with proto-patch which _copies_ all relevant information into caller-supplied structure, keeping module_mutex private. Time to split it sanely.
creator: Alexey Dobriyan
date: 2007-03-19T10:14:25-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: Alexey Dobriyan wrote: > On Sat, Mar 17, 2007 at 08:37:18PM +1100, Rusty Russell wrote: >> On Fri, 2007-03-16 at 12:51 +0100, Ingo Molnar wrote: >>> [...] >>> looking at the problem from another angle: wouldnt this be something >>> that would benefit from freeze_processes()/unfreeze_processes(), and >>> hence no locking would be required? >> Actually, the list manipulation is done with stop_machine for this >> reason. > > mmm, my changelog is slightly narrow than it should be. > > Non-emergency code is traversing modules list. > It finds "struct module *". > module is removed. > "struct module *" is now meaningless, but still dereferenced. > > How would all this refrigerator stuff would help? It wouldn't, > > Non-emergency code is traversing modules list. > It finds "struct module *". > Everything is freezed. > Module is removed. > Everything is unfreezed. > "struct module *" is now meaningless, but still dereferenced. That is why I asked if the refrigerator would preempt processes or not. AFAICS there is no path where the "struct module *" that is returned is used after a voluntary preemption point, so it should be safe. I might be missing something, though. However, this isn't very robust. Since the functions are still returning pointers to module data, some changes in the future might keep the pointer, and use it after a valid freezing point -> oops. >> Alexey, is preempt enabled in your kernel? > > Yes. FWIW, > > CONFIG_PREEMPT=y > CONFIG_PREEMPT_BKL=y > CONFIG_DEBUG_PREEMPT=y > > I very much agree with proto-patch which _copies_ all relevant > information into caller-supplied structure, keeping module_mutex private. > Time to split it sanely. That depends on the roadmap: if we think the freezer approach is the best in the long run, maybe your less intrusive (in the sense that it changes less stuff) patch should go in now (as a quick fix to mainline) so that after we've sorted out the bugs from the freezer in -mm, it will be easier to revert. However, if we thin…
creator: Paulo Marques
date: 2007-03-19T15:17:17-00:00

Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
type: item
description: On Mon, 2007-03-19 at 13:21 +0300, Alexey Dobriyan wrote: > I very much agree with proto-patch which _copies_ all relevant > information into caller-supplied structure, keeping module_mutex private. > Time to split it sanely. Indeed. The current interface needs to be ripped apart and put together sanely. Thanks! Rusty.
creator: Rusty Russell
date: 2007-03-19T23:23:07-00:00