--- linux/kernel/ksyms.c.orig Fri May 23 22:17:07 2003
+++ linux/kernel/ksyms.c Fri May 23 22:16:38 2003
@@ -65,6 +65,7 @@
extern int request_dma(unsigned int dmanr, char * deviceID);
extern void free_dma(unsigned int dmanr);
extern spinlock_t dma_spin_lock;
+extern int panic_timeout;
#ifdef CONFIG_MODVERSIONS
const struct module_symbol __export_Using_Versions
@@ -471,6 +472,8 @@
/* misc */
EXPORT_SYMBOL(panic);
+EXPORT_SYMBOL(panic_notifier_list);
+EXPORT_SYMBOL(panic_timeout);
EXPORT_SYMBOL(__out_of_line_bug);
EXPORT_SYMBOL(sprintf);
EXPORT_SYMBOL(snprintf);
On Friday 23 May 2003 15:38, Eyal Lebedinsky wrote:
Hi Eyal,
> The exports in ksyms are still necessary, and missing:
> depmod: *** Unresolved symbols in
> /lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_msghandler.o
> depmod: panic_notifier_list
> depmod: *** Unresolved symbols in
> /lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_watchdog.o
> depmod: panic_notifier_list
> depmod: panic_timeout
> The attached snippet was part of the earlier, larger patch.
I've send this fix 3 times and I gave up after silent ignores ;)
ciao, Marc
On Fri, 23 May 2003 23:38:53 +1000,
Eyal Lebedinsky <[email protected]> wrote:
>The exports in ksyms are still necessary, and missing:
>
>depmod: *** Unresolved symbols in
>/lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_msghandler.o
>depmod: panic_notifier_list
>depmod: *** Unresolved symbols in
>/lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_watchdog.o
>depmod: panic_notifier_list
>depmod: panic_timeout
Danger Will Robinson: panic notification to modules is racy.
Registering via panic_notifier_list does not bump the module use count,
a panic can occur while a module is being unloaded and you are dead.
No big deal for panic, you are already dying, but it is just a symptom
of a larger problem, yet another uncounted reference to module code.
_ANY_ notifier callback to a module is racy, think very carefully
before exporting any XXX_notifier_list.
I would go so far as to say that no XXX_notifier_list should be
exported, that includes notifier_chain_register() itself. If a module
needs to be notified then it should have glue code in the main kernel
that does try_inc_mod_count() on the module before calling any module
functions.
Marc-Christian Petersen wrote:
>On Friday 23 May 2003 15:38, Eyal Lebedinsky wrote:
>
>Hi Eyal,
>
>
>
>>The exports in ksyms are still necessary, and missing:
>>depmod: *** Unresolved symbols in
>>/lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_msghandler.o
>>depmod: panic_notifier_list
>>depmod: *** Unresolved symbols in
>>/lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_watchdog.o
>>depmod: panic_notifier_list
>>depmod: panic_timeout
>>The attached snippet was part of the earlier, larger patch.
>>
>>
>I've send this fix 3 times and I gave up after silent ignores ;)
>
I've resent my fixes, Marcelo said earlier he would take them, but they
didn't get into this release.
-Corey
Keith Owens wrote:
>On Fri, 23 May 2003 23:38:53 +1000,
>Eyal Lebedinsky <[email protected]> wrote:
>
>
>>The exports in ksyms are still necessary, and missing:
>>
>>depmod: *** Unresolved symbols in
>>/lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_msghandler.o
>>depmod: panic_notifier_list
>>depmod: *** Unresolved symbols in
>>/lib/modules/2.4.21-rc3/kernel/drivers/char/ipmi/ipmi_watchdog.o
>>depmod: panic_notifier_list
>>depmod: panic_timeout
>>
>>
>
>Danger Will Robinson: panic notification to modules is racy.
>
>Registering via panic_notifier_list does not bump the module use count,
>a panic can occur while a module is being unloaded and you are dead.
>No big deal for panic, you are already dying, but it is just a symptom
>of a larger problem, yet another uncounted reference to module code.
>_ANY_ notifier callback to a module is racy, think very carefully
>before exporting any XXX_notifier_list.
>
>I would go so far as to say that no XXX_notifier_list should be
>exported, that includes notifier_chain_register() itself. If a module
>needs to be notified then it should have glue code in the main kernel
>that does try_inc_mod_count() on the module before calling any module
>functions.
>
Although, as you noted, this one is not a problem, you are probably
right in general.
However, having every modules that uses a notifier list have its own
custom code
in the kernel is probably not a very good option, either. It makes
things messy and
adds unneeded bloat to the kernel.
Would it be possible to have a notifier_chain_register_module() that did
the job
generically? Or maybe if notifier_chain_unregister() did a
synchronize_kernel()
(the RCU call to wait until everything is clear) would that be good
enough? It would
only work if all the notifier chain calls where done while the kernel
was unpreemptable,
if I understand this correctly. I realize the RCU option is not
available in 2.4, though.
-Corey
On Sun, 25 May 2003 22:37:17 -0500,
Corey Minyard <[email protected]> wrote:
>Keith Owens wrote:
>>Danger Will Robinson: panic notification to modules is racy.
>>
>>Registering via panic_notifier_list does not bump the module use count,
>>a panic can occur while a module is being unloaded and you are dead.
>>No big deal for panic, you are already dying, but it is just a symptom
>>of a larger problem, yet another uncounted reference to module code.
>>_ANY_ notifier callback to a module is racy, think very carefully
>>before exporting any XXX_notifier_list.
>>
>>I would go so far as to say that no XXX_notifier_list should be
>>exported, that includes notifier_chain_register() itself. If a module
>>needs to be notified then it should have glue code in the main kernel
>>that does try_inc_mod_count() on the module before calling any module
>>functions.
>>
>Although, as you noted, this one is not a problem, you are probably
>right in general.
>
>However, having every modules that uses a notifier list have its own
>custom code in the kernel is probably not a very good option, either.
>It makes things messy and adds unneeded bloat to the kernel.
>
>Would it be possible to have a notifier_chain_register_module() that did
>the job generically?
notifier_chain_register_module() is possible, just pass __THIS_MODULE
and the code that runs the notifier chain does try_inc_mod_count()
before making the upcall. But that new function cannot be mixed with
notifier_chain_register(), it has to be a complete replacement. Not
going to happen in 2.4.
I considered making notifier_chain_register() a macro which called
notifier_chain_register_module() with __THIS_MODULE, but that assumes
that all calls to notifier_chain_register() are local, i.e. from the
top level functions. Alas there are any service routines that call
notifier_chain_register() on behalf of their caller, so the macro
approach will result in the wrong value for __THIS_MODULE.
>Or maybe if notifier_chain_unregister() did a
>synchronize_kernel()
>(the RCU call to wait until everything is clear) would that be good
>enough? It would
>only work if all the notifier chain calls where done while the kernel
>was unpreemptable,
>if I understand this correctly. I realize the RCU option is not
>available in 2.4, though.
notifier_chain_unregister() is not a problem, that is a downcall from
the module into the kernel when the module is going away, downcalls are
"always" safe. The race is a module that has started to unload, and
another cpu (or even the same cpu under some circumstances) runs the
notifier chain, doing an upcall from the kernel into a module without
locking or refcounts. Given the right timing, the notifier code could
even branch to a module that has been completely removed. Note that
notifier_call_chain() has no locking, so it is also racy against
notifier_chain_unregister().
On Sul, 2003-05-25 at 08:57, Keith Owens wrote:
> I would go so far as to say that no XXX_notifier_list should be
> exported, that includes notifier_chain_register() itself. If a module
> needs to be notified then it should have glue code in the main kernel
> that does try_inc_mod_count() on the module before calling any module
> functions.
That would be mindbogglingly ugly. Unfortunately Rusty has still only
half solved the module problem because modules are refcounted as an
"entity" not the module info and the module code/data split into two.
Ie I can't unload a module that has module object references because we
have no way to seperate "I'm talking about module xyz" and "I'm jumping
into module xyz". That IMHO is what is causing much of the remaining
mess.
Were they split then I could safely take a module object reference in
the notifiers and have
try_inc_mod_count()
do the right thing passed a module handle to a module that is unloaded
but has object references left.
Keith Owens wrote:
>On Sun, 25 May 2003 22:37:17 -0500,
>Corey Minyard <[email protected]> wrote:
>
>
>>Keith Owens wrote:
>>
>>
>>>Danger Will Robinson: panic notification to modules is racy.
>>>
>>>Registering via panic_notifier_list does not bump the module use count,
>>>a panic can occur while a module is being unloaded and you are dead.
>>>No big deal for panic, you are already dying, but it is just a symptom
>>>of a larger problem, yet another uncounted reference to module code.
>>>_ANY_ notifier callback to a module is racy, think very carefully
>>>before exporting any XXX_notifier_list.
>>>
>>>I would go so far as to say that no XXX_notifier_list should be
>>>exported, that includes notifier_chain_register() itself. If a module
>>>needs to be notified then it should have glue code in the main kernel
>>>that does try_inc_mod_count() on the module before calling any module
>>>functions.
>>>
>>>
>>>
>>Although, as you noted, this one is not a problem, you are probably
>>right in general.
>>
>>However, having every modules that uses a notifier list have its own
>>custom code in the kernel is probably not a very good option, either.
>>It makes things messy and adds unneeded bloat to the kernel.
>>
>>Would it be possible to have a notifier_chain_register_module() that did
>>the job generically?
>>
>>
>
>notifier_chain_register_module() is possible, just pass __THIS_MODULE
>and the code that runs the notifier chain does try_inc_mod_count()
>before making the upcall. But that new function cannot be mixed with
>notifier_chain_register(), it has to be a complete replacement. Not
>going to happen in 2.4.
>
>I considered making notifier_chain_register() a macro which called
>notifier_chain_register_module() with __THIS_MODULE, but that assumes
>that all calls to notifier_chain_register() are local, i.e. from the
>top level functions. Alas there are any service routines that call
>notifier_chain_register() on behalf of their caller, so the macro
>approach will result in the wrong value for __THIS_MODULE.
>
Why can't you have a module id in the notifier chain, and use a boolean
to tell if it is set, or something similar to that? That way you could
mix them, if the bool is set then do the try_in_module_count thing, if
not then just call the function. It does add some components to the
register structure, but that shouldn't hurt anything besides taking a
little more memory.
>
>
>
>>Or maybe if notifier_chain_unregister() did a
>>synchronize_kernel()
>>(the RCU call to wait until everything is clear) would that be good
>>enough? It would
>>only work if all the notifier chain calls where done while the kernel
>>was unpreemptable,
>>if I understand this correctly. I realize the RCU option is not
>>available in 2.4, though.
>>
>>
>
>notifier_chain_unregister() is not a problem, that is a downcall from
>the module into the kernel when the module is going away, downcalls are
>"always" safe. The race is a module that has started to unload, and
>another cpu (or even the same cpu under some circumstances) runs the
>notifier chain, doing an upcall from the kernel into a module without
>locking or refcounts. Given the right timing, the notifier code could
>even branch to a module that has been completely removed. Note that
>notifier_call_chain() has no locking, so it is also racy against
>notifier_chain_unregister().
>
>
You don't understand how the RCU code works. The race is as you
describe. If notifier_chain_unregister() removes the item from the list
and then calls synchronize_kernel(), and all the notifier calls are in
unpreemptable sections, you guarantee that no one can be left that can
call the notifier, they will all have left their preemptable sections
before synchronize_kernel() will return. It's a little wierd, but it
does work.
If the calls to the notifier chain are outside unpreemptable sections,
though, then there's no guaranteed of when they will run (with a
preemptable kernel) so this won't work.
-Corey
On Mon, 26 May 2003 19:30:04 -0500,
Corey Minyard <[email protected]> wrote:
>Keith Owens wrote:
>>I considered making notifier_chain_register() a macro which called
>>notifier_chain_register_module() with __THIS_MODULE, but that assumes
>>that all calls to notifier_chain_register() are local, i.e. from the
>>top level functions. Alas there are any service routines that call
>>notifier_chain_register() on behalf of their caller, so the macro
>>approach will result in the wrong value for __THIS_MODULE.
>>
>Why can't you have a module id in the notifier chain, and use a boolean
>to tell if it is set, or something similar to that? That way you could
>mix them, if the bool is set then do the try_in_module_count thing, if
>not then just call the function. It does add some components to the
>register structure, but that shouldn't hurt anything besides taking a
>little more memory.
It is a change of API in a 2.4 kernel. Not a good idea.
>>notifier_chain_unregister() is not a problem, that is a downcall from
>>the module into the kernel when the module is going away, downcalls are
>>"always" safe. The race is a module that has started to unload, and
>>another cpu (or even the same cpu under some circumstances) runs the
>>notifier chain, doing an upcall from the kernel into a module without
>>locking or refcounts. Given the right timing, the notifier code could
>>even branch to a module that has been completely removed. Note that
>>notifier_call_chain() has no locking, so it is also racy against
>>notifier_chain_unregister().
>>
>>
>You don't understand how the RCU code works.
(a) I understand how RCU works, I was working on lock free code for
years before RCU appeared in the kernel.
(b) This is 2.4, not 2.5, there is no RCU.
Keith Owens wrote:
>On Mon, 26 May 2003 19:30:04 -0500,
>Corey Minyard <[email protected]> wrote:
>
>
>>Keith Owens wrote:
>>
>>
>>>I considered making notifier_chain_register() a macro which called
>>>notifier_chain_register_module() with __THIS_MODULE, but that assumes
>>>that all calls to notifier_chain_register() are local, i.e. from the
>>>top level functions. Alas there are any service routines that call
>>>notifier_chain_register() on behalf of their caller, so the macro
>>>approach will result in the wrong value for __THIS_MODULE.
>>>
>>>
>>>
>>Why can't you have a module id in the notifier chain, and use a boolean
>>to tell if it is set, or something similar to that? That way you could
>>mix them, if the bool is set then do the try_in_module_count thing, if
>>not then just call the function. It does add some components to the
>>register structure, but that shouldn't hurt anything besides taking a
>>little more memory.
>>
>>
>
>It is a change of API in a 2.4 kernel. Not a good idea.
>
Does adding a field to a structure (where the user does not have to do
anything with the
field) change the API? That would be the only API change here.
>
>
>
>>>notifier_chain_unregister() is not a problem, that is a downcall from
>>>the module into the kernel when the module is going away, downcalls are
>>>"always" safe. The race is a module that has started to unload, and
>>>another cpu (or even the same cpu under some circumstances) runs the
>>>notifier chain, doing an upcall from the kernel into a module without
>>>locking or refcounts. Given the right timing, the notifier code could
>>>even branch to a module that has been completely removed. Note that
>>>notifier_call_chain() has no locking, so it is also racy against
>>>notifier_chain_unregister().
>>>
>>>
>>>
>>>
>>You don't understand how the RCU code works.
>>
>>
>
>(a) I understand how RCU works, I was working on lock free code for
> years before RCU appeared in the kernel.
>
Then maybe I don't understand it. The writers of the RCU code pointed
it out to me
for a very similar situation. Why doesn't calling synchronize_kernel() in
notifier_chain_unregister() fix the module unload and unregister races?
Or perhaps not all notifier chains get called when the kernel is
unpreemptable?
You could turn preempt off in notifier_call_chain(), though that might have
some bad effects. In the preemptable kernel case, I'm not sure if the RCU
code waits until all threads of execution leave the kernel. If it does,
then
preemption shouldn't even matter.
>
>(b) This is 2.4, not 2.5, there is no RCU.
>
>
Understood (I already said this). But I was thinking it might be a good
addition to 2.5,
if it solved the problem.
-Corey
On Mon, 26 May 2003 23:45:39 -0500,
Corey Minyard <[email protected]> wrote:
>Keith Owens wrote:
>>>Why can't you have a module id in the notifier chain, and use a boolean
>>>to tell if it is set, or something similar to that? That way you could
>>>mix them, if the bool is set then do the try_in_module_count thing, if
>>>not then just call the function. It does add some components to the
>>>register structure, but that shouldn't hurt anything besides taking a
>>>little more memory.
>>
>>It is a change of API in a 2.4 kernel. Not a good idea.
>>
>Does adding a field to a structure (where the user does not have to do
>anything with the
>field) change the API? That would be the only API change here.
The user does have to do something. Every piece of code that calls
notify_register has to set the new field to __THIS_MODULE. WIthout
that field being set, you are no better off, the race still exists.
Index: include/linux/notifier.h
===================================================================
RCS file: /home/cvs/linux-2.4/include/linux/notifier.h,v
retrieving revision 1.3
diff -u -r1.3 notifier.h
--- include/linux/notifier.h 4 Aug 2002 00:02:49 -0000 1.3
+++ include/linux/notifier.h 27 May 2003 13:39:02 -0000
@@ -10,18 +10,22 @@
#ifndef _LINUX_NOTIFIER_H
#define _LINUX_NOTIFIER_H
#include <linux/errno.h>
+#include <linux/module.h>
struct notifier_block
{
int (*notifier_call)(struct notifier_block *self, unsigned long, void *);
struct notifier_block *next;
int priority;
+
+ struct module *owner; /* NULL if not from a module */
};
#ifdef __KERNEL__
extern int notifier_chain_register(struct notifier_block **list, struct notifier_block *n);
+extern int notifier_chain_register_module(struct notifier_block **list, struct notifier_block *n, struct module *owner);
extern int notifier_chain_unregister(struct notifier_block **nl, struct notifier_block *n);
extern int notifier_call_chain(struct notifier_block **n, unsigned long val, void *v);
Index: kernel/sys.c
===================================================================
RCS file: /home/cvs/linux-2.4/kernel/sys.c,v
retrieving revision 1.14
diff -u -r1.14 sys.c
--- kernel/sys.c 9 May 2003 00:35:17 -0000 1.14
+++ kernel/sys.c 27 May 2003 13:39:03 -0000
@@ -71,16 +71,19 @@
rwlock_t notifier_lock = RW_LOCK_UNLOCKED;
/**
- * notifier_chain_register - Add notifier to a notifier chain
+ * notifier_chain_register_module - Add notifier to a notifier chain
+ * for a module.
+ *
* @list: Pointer to root list pointer
* @n: New entry in notifier chain
+ * @owner: The module that owns this notifier block
*
* Adds a notifier to a notifier chain.
*
* Currently always returns zero.
*/
-int notifier_chain_register(struct notifier_block **list, struct notifier_block *n)
+int notifier_chain_register_module(struct notifier_block **list, struct notifier_block *n, struct module *owner)
{
write_lock(¬ifier_lock);
while(*list)
@@ -90,12 +93,28 @@
list= &((*list)->next);
}
n->next = *list;
+ n->owner = owner;
*list=n;
write_unlock(¬ifier_lock);
return 0;
}
/**
+ * notifier_chain_register - Add notifier to a notifier chain
+ * @list: Pointer to root list pointer
+ * @n: New entry in notifier chain
+ *
+ * Adds a notifier to a notifier chain.
+ *
+ * Currently always returns zero.
+ */
+
+int notifier_chain_register(struct notifier_block **list, struct notifier_block *n)
+{
+ return notifier_chain_register_module(list, n, NULL);
+}
+
+/**
* notifier_chain_unregister - Remove notifier from a notifier chain
* @nl: Pointer to root list pointer
* @n: New entry in notifier chain
@@ -128,7 +147,8 @@
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
*
- * Calls each function in a notifier chain in turn.
+ * Calls each function in a notifier chain in turn. If it is owned
+ * by a module, increment that module's use count while in the call.
*
* If the return value of the notifier can be and'd
* with %NOTIFY_STOP_MASK, then notifier_call_chain
@@ -142,15 +162,24 @@
{
int ret=NOTIFY_DONE;
struct notifier_block *nb = *n;
+ struct notifier_block *next;
+ struct module *owner;
- while(nb)
+ for (; nb; nb=next)
{
+ owner = nb->owner;
+ next = nb->next;
+ if ((owner) && (!try_inc_mod_count(owner)))
+ /* The module that owns us has ceased to
+ exist, just go on. */
+ continue;
ret=nb->notifier_call(nb,val,v);
+ if (owner)
+ __MOD_DEC_USE_COUNT(owner);
if(ret&NOTIFY_STOP_MASK)
{
return ret;
}
- nb=nb->next;
}
return ret;
}
On Tue, May 27, 2003 at 09:48:53AM -0500, Corey Minyard wrote:
> >The user does have to do something. Every piece of code that calls
> >notify_register has to set the new field to __THIS_MODULE. WIthout
> >that field being set, you are no better off, the race still exists.
> >
> Why? The user doesn't have to set the field, you can let the
> registration code do that. I have attached a patch as an example.
How the devil would registration code figure out which module should
be used?
[email protected] wrote:
>On Tue, May 27, 2003 at 09:48:53AM -0500, Corey Minyard wrote:
>
>
>
>>>The user does have to do something. Every piece of code that calls
>>>notify_register has to set the new field to __THIS_MODULE. WIthout
>>>that field being set, you are no better off, the race still exists.
>>>
>>>
>>>
>>Why? The user doesn't have to set the field, you can let the
>>registration code do that. I have attached a patch as an example.
>>
>>
>
>How the devil would registration code figure out which module should
>be used?
>
>
You create a new call that also takes the module as a parameter, and
have the old call set the module owner to NULL. You export the new
call, and modules use it.
-Corey
On Tue, 27 May 2003 12:09:12 -0500,
Corey Minyard <[email protected]> wrote:
>[email protected] wrote:
viro>>How the devil would registration code figure out which module should
viro>>be used?
I am glad that somebody gets it.
minyard>You create a new call that also takes the module as a parameter, and
minyard>have the old call set the module owner to NULL. You export the new
minyard>call, and modules use it.
Which brings us right back to where we started.
* Find all users of notifier_chain_register() and change them to the
new API.
* If any of the calls to notifier_chain_register() are in service
routines then add a new parameter to the service routine to pass in
the module owner. There are several service routines that do this,
especially in the network code.
* Find all callers of all the modified service routines and change them
to use the new API for these service routines.
* Repeat until you have propagated the new API all the way out to the
end points, to the only code that knows the module owner.
Not in 2.4 thanks.