Re: Notifier chains are unsafe

[Alan Stern <stern@rowland.harvard.edu>]

On Thu, 27 Oct 2005, Chandra Seetharaman wrote:

> > I agree that updating all the existing definitions would be a little
> > painful.  However, adding a new notifier_head will require doing those 
> > updates anyway.
> 
> That change would be minimal, one have only change the place from which
> the notify_register is called.
> 
> Whereas if we change it to become two types one has to go through the
> corresponding callouts (and the functions they call and so on) to see
> which (BLOCKING or ATOMIC) notifier mechanism to use.

True, the amount of study required would be greater -- but the actual
change is still minimal!  :-)

In many cases the type is obvious.  For instance, there are lots of
callouts that occur in ioctl handlers; they clearly have to be BLOCKING.

In fact there's another sort of problem I neglected to mention: Callouts
that unregister themselves will have to be changed.  I think that's going
to be unavoidable.  Fortunately there probably aren't very many, but
finding them might not be easy.

> Functionally looks good. But there are two problems w.r.t interface
> changes:
>     1. above mentioned problem of making sure of all the places to use
>        the proper one.

This at least is a one-time difficulty, and it doesn't require changing 
many sections of code.  Last time I checked, there were 28 notifier chain 
definitions in the kernel.

>     2. Requiring register and unregister to be able to sleep. (there are
>        few usages that are called with a lock held)

Are there?  I haven't looked at all of them closely.  According to Keith 
Owens, such usages are wrong.

> How does the following code look (only change w.r.t the existing usage
> model is that unregister can now return -EAGAIN, if the list is busy).
> 
> One assumption the following code makes is that the store of a pointer
> (next in the list) is atomic. If that assumption is unacceptable, we can
> do one of two things:
>     1. change notify_register to return -EAGAIN if list is busy.
>     2. move the chain list in call_chain under lock and use that
>        list instead of using the chain in the head, and restore it back
>        before returning.

I see a couple of problems (aside from the trivial one where you increment 
nh->readers before the early exit).

The biggest problem is allowing unregister to return an error.  None of 
its callers will expect that, and they all will have to be changed.  There 
are a lot more calls to unregister than there are chain definitions.

The other problem is that you violated Keith's statement that 
notifier_call_chain shouldn't take any locks.  On the other hand, if we 
put together all the requirements people have listed for notifier chains, 
the resulting set is inconsistent!  That's part of the reason why I 
suggested implementing two different kinds of chains.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/