Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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 think the most reliable solution would be to not return internal module information through pointers and keep all that logic internal to module.c, then the "proto-patch" with some improvements might be the way to go... -- Paulo Marques - www.grupopie.com "God is love. Love is blind. Ray Charles is blind. Ray Charles is God." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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 <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, Mar 16, 2007 at 08:27:29PM +, Paulo Marques wrote: > Andrew Morton wrote: > >On Fri, 16 Mar 2007 17:16:39 + Paulo Marques <[EMAIL PROTECTED]> > >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 stopped > kernel: Strange, xfsdatad/1 not stopped > kernel: Strange, kjournald not stopped > kernel: Strange, khubd not stopped > kernel: Strange, khelper not stopped > kernel: Strange, kbluetoothd not stopped > kernel: done. > > I repeated the test and did a Alt+SysRq+T to try to find out what > kbluetoothd was doing and got this: > > kernel: kbluetoothd D 79A11860 0 19156 1 19142 > (NOTLB) > kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030 > 0003 > kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001 > 0001 > kernel: 9a2d7570 79a11860 c7018140 1832 42430d03 > 00ab > kernel: Call Trace: > kernel: [<7845dba3>] wait_for_completion+0x7d/0xb7 > kernel: [<781190ba>] default_wake_function+0x0/0xc > kernel: [<781190ba>] default_wake_function+0x0/0xc > kernel: [<7812c759>] call_usermodehelper_keys+0xd1/0xf1 > kernel: [<7812c41e>] request_module+0x96/0xd9 > kernel: [<783e30fe>] sock_alloc_inode+0x20/0x4e > kernel: [<78172559>] alloc_inode+0x15/0x115 > kernel: [<78172d87>] new_inode+0x24/0x81 > kernel: [<783e4003>] __sock_create+0x111/0x199 > kernel: [<783e40a3>] sock_create+0x18/0x1d > kernel: [<783e40e1>] sys_socket+0x1c/0x43 > kernel: [<783e51da>] sys_socketcall+0x247/0x24c > kernel: [<78121b2d>] sys_gettimeofday+0x2c/0x65 > kernel: [<78103f10>] sysenter_past_esp+0x5d/0x81 > > And this was as far as I got... > > This actually seems like a better approach than to hold module_mutex > everywhere to account for an operation that should be "rare" (module > loading/unloading). If something like this goes in, there are probably a > few more places inside module.c where we can drop the locking completely. > > However, it still has a few gotchas. Apart from the problem above (which > may still be me doing something wrong) it makes module loading / > unloading depend on CONFIG_PM which is somewhat unexpected for the user. It'd be a bug. cat /proc/kallsyms should work reliably regardless of CONFIG_PM, CONFIG_MODULES, etc. > Would it make sense to separate the process freezing / thawing API from > actual power management and create a new config option (CONFIG_FREEZER?) > that was automatically selected by the systems that used it (CONFIG_PM, > CONFIG_MODULES, etc.)? or is that overkill? I tried you patch on top of 2.6.21-rc4-5851fadce8824d5d4b8fd02c22ae098401f6489e *shrug* Let's say that is doesn't work here. :) On boot I got Stopping user space processes timed out after 20 seconds (1 tasks refusing to freeze): mount Strange, kseriod not stopped Strange, pdflush not stopped Strange, pdflush not stopped Strange, kswapd0 not stopped Strange, kjournald not stopped Strange, khelper not stopped Strange, mount not stopped
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, Mar 16, 2007 at 08:27:29PM +, Paulo Marques wrote: Andrew Morton wrote: On Fri, 16 Mar 2007 17:16:39 + Paulo Marques [EMAIL PROTECTED] 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 stopped kernel: Strange, xfsdatad/1 not stopped kernel: Strange, kjournald not stopped kernel: Strange, khubd not stopped kernel: Strange, khelper not stopped kernel: Strange, kbluetoothd not stopped kernel: done. I repeated the test and did a Alt+SysRq+T to try to find out what kbluetoothd was doing and got this: kernel: kbluetoothd D 79A11860 0 19156 1 19142 (NOTLB) kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030 0003 kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001 0001 kernel: 9a2d7570 79a11860 c7018140 1832 42430d03 00ab kernel: Call Trace: kernel: [7845dba3] wait_for_completion+0x7d/0xb7 kernel: [781190ba] default_wake_function+0x0/0xc kernel: [781190ba] default_wake_function+0x0/0xc kernel: [7812c759] call_usermodehelper_keys+0xd1/0xf1 kernel: [7812c41e] request_module+0x96/0xd9 kernel: [783e30fe] sock_alloc_inode+0x20/0x4e kernel: [78172559] alloc_inode+0x15/0x115 kernel: [78172d87] new_inode+0x24/0x81 kernel: [783e4003] __sock_create+0x111/0x199 kernel: [783e40a3] sock_create+0x18/0x1d kernel: [783e40e1] sys_socket+0x1c/0x43 kernel: [783e51da] sys_socketcall+0x247/0x24c kernel: [78121b2d] sys_gettimeofday+0x2c/0x65 kernel: [78103f10] sysenter_past_esp+0x5d/0x81 And this was as far as I got... This actually seems like a better approach than to hold module_mutex everywhere to account for an operation that should be rare (module loading/unloading). If something like this goes in, there are probably a few more places inside module.c where we can drop the locking completely. However, it still has a few gotchas. Apart from the problem above (which may still be me doing something wrong) it makes module loading / unloading depend on CONFIG_PM which is somewhat unexpected for the user. It'd be a bug. cat /proc/kallsyms should work reliably regardless of CONFIG_PM, CONFIG_MODULES, etc. Would it make sense to separate the process freezing / thawing API from actual power management and create a new config option (CONFIG_FREEZER?) that was automatically selected by the systems that used it (CONFIG_PM, CONFIG_MODULES, etc.)? or is that overkill? I tried you patch on top of 2.6.21-rc4-5851fadce8824d5d4b8fd02c22ae098401f6489e *shrug* Let's say that is doesn't work here. :) On boot I got Stopping user space processes timed out after 20 seconds (1 tasks refusing to freeze): mount Strange, kseriod not stopped Strange, pdflush not stopped Strange, pdflush not stopped Strange, kswapd0 not stopped Strange, kjournald not stopped Strange, khelper not stopped Strange, mount not stopped Filesystem loop0: Disabling barriers, not supported by the underlying device XFS mounting filesystem loop0 Ending clean XFS mount for filesystem: loop0
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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 [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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 think the most reliable solution would be to not return internal module information through pointers and keep all that logic internal to module.c, then the proto-patch with some improvements might be the way to go... -- Paulo Marques - www.grupopie.com God is love. Love is blind. Ray Charles is blind. Ray Charles is God. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, 2007-03-16 at 12:51 +0100, Ingo Molnar wrote: > * Alexey Dobriyan <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, 2007-03-16 at 12:51 +0100, Ingo Molnar wrote: * Alexey Dobriyan [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, 16 Mar 2007 20:27:29 + Paulo Marques <[EMAIL PROTECTED]> wrote: > Andrew Morton wrote: > > On Fri, 16 Mar 2007 17:16:39 + Paulo Marques <[EMAIL PROTECTED]> 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, 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 stopped > kernel: Strange, xfsdatad/1 not stopped > kernel: Strange, kjournald not stopped > kernel: Strange, khubd not stopped > kernel: Strange, khelper not stopped > kernel: Strange, kbluetoothd not stopped > kernel: done. There are a bunch of freezer fixes in -mm. But problems might still remain - I don't think freezer has had a lot of load put on it yet, but it will soon and it needs to become reliable. > I repeated the test and did a Alt+SysRq+T to try to find out what > kbluetoothd was doing and got this: > > kernel: kbluetoothd D 79A11860 0 19156 1 19142 > (NOTLB) > kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030 > 0003 > kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001 > 0001 > kernel: 9a2d7570 79a11860 c7018140 1832 42430d03 > 00ab > kernel: Call Trace: > kernel: [<7845dba3>] wait_for_completion+0x7d/0xb7 > kernel: [<781190ba>] default_wake_function+0x0/0xc > kernel: [<781190ba>] default_wake_function+0x0/0xc > kernel: [<7812c759>] call_usermodehelper_keys+0xd1/0xf1 > kernel: [<7812c41e>] request_module+0x96/0xd9 > kernel: [<783e30fe>] sock_alloc_inode+0x20/0x4e > kernel: [<78172559>] alloc_inode+0x15/0x115 > kernel: [<78172d87>] new_inode+0x24/0x81 > kernel: [<783e4003>] __sock_create+0x111/0x199 > kernel: [<783e40a3>] sock_create+0x18/0x1d > kernel: [<783e40e1>] sys_socket+0x1c/0x43 > kernel: [<783e51da>] sys_socketcall+0x247/0x24c > kernel: [<78121b2d>] sys_gettimeofday+0x2c/0x65 > kernel: [<78103f10>] sysenter_past_esp+0x5d/0x81 > > And this was as far as I got... > > This actually seems like a better approach than to hold module_mutex > everywhere to account for an operation that should be "rare" (module > loading/unloading). If something like this goes in, there are probably a > few more places inside module.c where we can drop the locking completely. Yes, using the freezer and module load/unload time seems like a good idea. > However, it still has a few gotchas. Apart from the problem above (which > may still be me doing something wrong) it makes module loading / > unloading depend on CONFIG_PM which is somewhat unexpected for the user. yup. > Would it make sense to separate the process freezing / thawing API from > actual power management and create a new config option (CONFIG_FREEZER?) > that was automatically selected by the systems that used it (CONFIG_PM, > CONFIG_MODULES, etc.)? or is that overkill? Yes, freezer needs to be decoupled from swsusp and from power management and it should become a first-class core kernel component. Whether we would need a CONFIG_FREEZER isn't clear - I suspect we'd end up just compiling it unconditionally. I cc'ed Rafael, who is doing the freezer
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Andrew Morton wrote:
On Fri, 16 Mar 2007 17:16:39 + Paulo Marques <[EMAIL PROTECTED]> 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 stopped
kernel: Strange, xfsdatad/1 not stopped
kernel: Strange, kjournald not stopped
kernel: Strange, khubd not stopped
kernel: Strange, khelper not stopped
kernel: Strange, kbluetoothd not stopped
kernel: done.
I repeated the test and did a Alt+SysRq+T to try to find out what
kbluetoothd was doing and got this:
kernel: kbluetoothd D 79A11860 0 19156 1 19142
(NOTLB)
kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030
0003
kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001
0001
kernel: 9a2d7570 79a11860 c7018140 1832 42430d03
00ab
kernel: Call Trace:
kernel: [<7845dba3>] wait_for_completion+0x7d/0xb7
kernel: [<781190ba>] default_wake_function+0x0/0xc
kernel: [<781190ba>] default_wake_function+0x0/0xc
kernel: [<7812c759>] call_usermodehelper_keys+0xd1/0xf1
kernel: [<7812c41e>] request_module+0x96/0xd9
kernel: [<783e30fe>] sock_alloc_inode+0x20/0x4e
kernel: [<78172559>] alloc_inode+0x15/0x115
kernel: [<78172d87>] new_inode+0x24/0x81
kernel: [<783e4003>] __sock_create+0x111/0x199
kernel: [<783e40a3>] sock_create+0x18/0x1d
kernel: [<783e40e1>] sys_socket+0x1c/0x43
kernel: [<783e51da>] sys_socketcall+0x247/0x24c
kernel: [<78121b2d>] sys_gettimeofday+0x2c/0x65
kernel: [<78103f10>] sysenter_past_esp+0x5d/0x81
And this was as far as I got...
This actually seems like a better approach than to hold module_mutex
everywhere to account for an operation that should be "rare" (module
loading/unloading). If something like this goes in, there are probably a
few more places inside module.c where we can drop the locking completely.
However, it still has a few gotchas. Apart from the problem above (which
may still be me doing something wrong) it makes module loading /
unloading depend on CONFIG_PM which is somewhat unexpected for the user.
Would it make sense to separate the process freezing / thawing API from
actual power management and create a new config option (CONFIG_FREEZER?)
that was automatically selected by the systems that used it (CONFIG_PM,
CONFIG_MODULES, etc.)? or is that overkill?
--
Paulo Marques - www.grupopie.com
"Nostalgia isn't what it used to be."
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -35,7 +35,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -618,13 +618,23 @@ static int __try_stop_module(void *_sref
return 0;
}
+static int freeze_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
+{
+ int ret;
+ freeze_processes();
+ ret = fn(data);
+ thaw_processes();
+ return ret;
+}
+
static int try_stop_module(struct module *mod, int flags, int *forced)
{
struct stopref sref = { mod, flags, forced };
- return stop_machine_run(__try_stop_module, , NR_CPUS);
+ return freeze_machine_run(__try_stop_module, , NR_CPUS);
}
+
unsigned int module_refcount(struct module *mod)
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, 16 Mar 2007 17:16:39 + Paulo Marques <[EMAIL PROTECTED]> 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(). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Paulo Marques <[EMAIL PROTECTED]> 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(>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."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
* Paulo Marques <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Alexey Dobriyan <[EMAIL PROTECTED]> 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 +++-
kernel/kallsyms.c | 53 +---
kernel/kprobes.c|6 ++--
kernel/lockdep.c|3 --
kernel/module.c | 44 +++-
kernel/time/timer_list.c|3 --
kernel/time/timer_stats.c |3 --
mm/slab.c |6 ++--
15 files changed, 114 insertions(+), 93 deletions(-)
diff --git a/arch/parisc/kernel/unwind.c b/arch/parisc/kernel/unwind.c
--- a/arch/parisc/kernel/unwind.c
+++ b/arch/parisc/kernel/unwind.c
@@ -216,11 +216,10 @@ static void unwind_frame_regs(struct unw
/* Handle some frequent special cases */
{
char symname[KSYM_NAME_LEN+1];
- char *modname;
unsigned long symsize, offset;
kallsyms_lookup(info->ip, , ,
- , symname);
+ NULL, symname, 0);
dbg("info->ip = 0x%lx, name = %s\n", info->ip, symname);
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1218,7 +1218,6 @@ static void get_function_bounds(unsigned
{
unsigned long size, offset;
const char *name;
- char *modname;
*startp = *endp = 0;
if (pc == 0)
@@ -1226,7 +1225,7 @@ static void get_function_bounds(unsigned
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(pc, , , , tmpstr);
+ name = kallsyms_lookup(pc, , , NULL, tmpstr, 0);
if (name != NULL) {
*startp = pc - offset;
*endp = pc - offset + size;
@@ -2496,7 +2495,7 @@ symbol_lookup(void)
static void xmon_print_symbol(unsigned long address, const char *mid,
const char *after)
{
- char *modname;
+ char modname[MODULE_NAME_LEN + 1];
const char *name = NULL;
unsigned long offset, size;
@@ -2504,8 +2503,8 @@ static void xmon_print_symbol(unsigned l
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(address, , , ,
- tmpstr);
+ name = kallsyms_lookup(address, , , modname,
+ tmpstr, 0);
sync();
/* wait a little while to see if we get a machine check */
__delay(200);
@@ -2515,7 +2514,7 @@ static void xmon_print_symbol(unsigned l
if (name) {
printf("%s%s+%#lx/%#lx", mid, name, offset, size);
- if (modname)
+ if (modname[0])
printf(" [%s]", modname);
}
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
* Alexey Dobriyan <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
* Alexey Dobriyan [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Alexey Dobriyan [EMAIL PROTECTED] 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 +++-
kernel/kallsyms.c | 53 +---
kernel/kprobes.c|6 ++--
kernel/lockdep.c|3 --
kernel/module.c | 44 +++-
kernel/time/timer_list.c|3 --
kernel/time/timer_stats.c |3 --
mm/slab.c |6 ++--
15 files changed, 114 insertions(+), 93 deletions(-)
diff --git a/arch/parisc/kernel/unwind.c b/arch/parisc/kernel/unwind.c
--- a/arch/parisc/kernel/unwind.c
+++ b/arch/parisc/kernel/unwind.c
@@ -216,11 +216,10 @@ static void unwind_frame_regs(struct unw
/* Handle some frequent special cases */
{
char symname[KSYM_NAME_LEN+1];
- char *modname;
unsigned long symsize, offset;
kallsyms_lookup(info-ip, symsize, offset,
- modname, symname);
+ NULL, symname, 0);
dbg(info-ip = 0x%lx, name = %s\n, info-ip, symname);
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1218,7 +1218,6 @@ static void get_function_bounds(unsigned
{
unsigned long size, offset;
const char *name;
- char *modname;
*startp = *endp = 0;
if (pc == 0)
@@ -1226,7 +1225,7 @@ static void get_function_bounds(unsigned
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(pc, size, offset, modname, tmpstr);
+ name = kallsyms_lookup(pc, size, offset, NULL, tmpstr, 0);
if (name != NULL) {
*startp = pc - offset;
*endp = pc - offset + size;
@@ -2496,7 +2495,7 @@ symbol_lookup(void)
static void xmon_print_symbol(unsigned long address, const char *mid,
const char *after)
{
- char *modname;
+ char modname[MODULE_NAME_LEN + 1];
const char *name = NULL;
unsigned long offset, size;
@@ -2504,8 +2503,8 @@ static void xmon_print_symbol(unsigned l
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(address, size, offset, modname,
- tmpstr);
+ name = kallsyms_lookup(address, size, offset, modname,
+ tmpstr, 0);
sync();
/* wait a little while to see if we get a machine check */
__delay(200);
@@ -2515,7 +2514,7 @@ static void xmon_print_symbol(unsigned l
if (name) {
printf(%s%s+%#lx/%#lx, mid, name, offset, size);
- if (modname)
+ if (modname[0])
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
* Paulo Marques [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Ingo Molnar wrote:
* Paulo Marques [EMAIL PROTECTED] 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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, 16 Mar 2007 17:16:39 + Paulo Marques [EMAIL PROTECTED] 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(). - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
Andrew Morton wrote:
On Fri, 16 Mar 2007 17:16:39 + Paulo Marques [EMAIL PROTECTED] 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 stopped
kernel: Strange, xfsdatad/1 not stopped
kernel: Strange, kjournald not stopped
kernel: Strange, khubd not stopped
kernel: Strange, khelper not stopped
kernel: Strange, kbluetoothd not stopped
kernel: done.
I repeated the test and did a Alt+SysRq+T to try to find out what
kbluetoothd was doing and got this:
kernel: kbluetoothd D 79A11860 0 19156 1 19142
(NOTLB)
kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030
0003
kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001
0001
kernel: 9a2d7570 79a11860 c7018140 1832 42430d03
00ab
kernel: Call Trace:
kernel: [7845dba3] wait_for_completion+0x7d/0xb7
kernel: [781190ba] default_wake_function+0x0/0xc
kernel: [781190ba] default_wake_function+0x0/0xc
kernel: [7812c759] call_usermodehelper_keys+0xd1/0xf1
kernel: [7812c41e] request_module+0x96/0xd9
kernel: [783e30fe] sock_alloc_inode+0x20/0x4e
kernel: [78172559] alloc_inode+0x15/0x115
kernel: [78172d87] new_inode+0x24/0x81
kernel: [783e4003] __sock_create+0x111/0x199
kernel: [783e40a3] sock_create+0x18/0x1d
kernel: [783e40e1] sys_socket+0x1c/0x43
kernel: [783e51da] sys_socketcall+0x247/0x24c
kernel: [78121b2d] sys_gettimeofday+0x2c/0x65
kernel: [78103f10] sysenter_past_esp+0x5d/0x81
And this was as far as I got...
This actually seems like a better approach than to hold module_mutex
everywhere to account for an operation that should be rare (module
loading/unloading). If something like this goes in, there are probably a
few more places inside module.c where we can drop the locking completely.
However, it still has a few gotchas. Apart from the problem above (which
may still be me doing something wrong) it makes module loading /
unloading depend on CONFIG_PM which is somewhat unexpected for the user.
Would it make sense to separate the process freezing / thawing API from
actual power management and create a new config option (CONFIG_FREEZER?)
that was automatically selected by the systems that used it (CONFIG_PM,
CONFIG_MODULES, etc.)? or is that overkill?
--
Paulo Marques - www.grupopie.com
Nostalgia isn't what it used to be.
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -35,7 +35,7 @@
#include linux/vermagic.h
#include linux/notifier.h
#include linux/sched.h
-#include linux/stop_machine.h
+#include linux/freezer.h
#include linux/device.h
#include linux/string.h
#include linux/mutex.h
@@ -618,13 +618,23 @@ static int __try_stop_module(void *_sref
return 0;
}
+static int freeze_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
+{
+ int ret;
+ freeze_processes();
+ ret = fn(data);
+ thaw_processes();
+ return ret;
+}
+
static int try_stop_module(struct module *mod, int flags, int *forced)
{
struct stopref sref = { mod, flags, forced };
- return stop_machine_run(__try_stop_module, sref, NR_CPUS);
+ return
Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races
On Fri, 16 Mar 2007 20:27:29 + Paulo Marques [EMAIL PROTECTED] wrote: Andrew Morton wrote: On Fri, 16 Mar 2007 17:16:39 + Paulo Marques [EMAIL PROTECTED] 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, 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 stopped kernel: Strange, xfsdatad/1 not stopped kernel: Strange, kjournald not stopped kernel: Strange, khubd not stopped kernel: Strange, khelper not stopped kernel: Strange, kbluetoothd not stopped kernel: done. There are a bunch of freezer fixes in -mm. But problems might still remain - I don't think freezer has had a lot of load put on it yet, but it will soon and it needs to become reliable. I repeated the test and did a Alt+SysRq+T to try to find out what kbluetoothd was doing and got this: kernel: kbluetoothd D 79A11860 0 19156 1 19142 (NOTLB) kernel: 9a269e4c 0082 0001 79a11860 79a09860 c7018030 0003 kernel: 9a269e71 78475100 c7ebe000 c6730e40 0001 0001 0001 kernel: 9a2d7570 79a11860 c7018140 1832 42430d03 00ab kernel: Call Trace: kernel: [7845dba3] wait_for_completion+0x7d/0xb7 kernel: [781190ba] default_wake_function+0x0/0xc kernel: [781190ba] default_wake_function+0x0/0xc kernel: [7812c759] call_usermodehelper_keys+0xd1/0xf1 kernel: [7812c41e] request_module+0x96/0xd9 kernel: [783e30fe] sock_alloc_inode+0x20/0x4e kernel: [78172559] alloc_inode+0x15/0x115 kernel: [78172d87] new_inode+0x24/0x81 kernel: [783e4003] __sock_create+0x111/0x199 kernel: [783e40a3] sock_create+0x18/0x1d kernel: [783e40e1] sys_socket+0x1c/0x43 kernel: [783e51da] sys_socketcall+0x247/0x24c kernel: [78121b2d] sys_gettimeofday+0x2c/0x65 kernel: [78103f10] sysenter_past_esp+0x5d/0x81 And this was as far as I got... This actually seems like a better approach than to hold module_mutex everywhere to account for an operation that should be rare (module loading/unloading). If something like this goes in, there are probably a few more places inside module.c where we can drop the locking completely. Yes, using the freezer and module load/unload time seems like a good idea. However, it still has a few gotchas. Apart from the problem above (which may still be me doing something wrong) it makes module loading / unloading depend on CONFIG_PM which is somewhat unexpected for the user. yup. Would it make sense to separate the process freezing / thawing API from actual power management and create a new config option (CONFIG_FREEZER?) that was automatically selected by the systems that used it (CONFIG_PM, CONFIG_MODULES, etc.)? or is that overkill? Yes, freezer needs to be decoupled from swsusp and from power management and it should become a first-class core kernel component. Whether we would need a CONFIG_FREEZER isn't clear - I suspect we'd end up just compiling it unconditionally. I cc'ed Rafael, who is doing the freezer revamp work. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo
