Re: Notifier chains are unsafe

From: Alan Stern
Date: Sat Oct 29 2005 - 09:51:57 EST


On Fri, 28 Oct 2005, Chandra Seetharaman wrote:

> On Fri, 2005-10-28 at 10:23 -0400, Alan Stern wrote:
> > On Thu, 27 Oct 2005, Chandra Seetharaman wrote:
> >
> > > So, requirements to fix the bug are:
> > > - no sleeping in register/unregister(if we want to keep the
> > > current way of use. We can change it and make the relevant
> > > changes in the kernel code, if it is agreeable)
> >
> > I think we will have to make these changes. In principal it shouldn't be
> > hard to add a simple "enabled" flag to each callout which currently is
> > registered/unregistered atomically or while running. We could even put
> > such a flag into the notifier_block structure and add routines to set or
> > clear it, using appropriate barriers.
>
> I do not understand the purpose of enabled flag. Can you clarify

Something like this:

struct notifier_block {
int (*notifier_call)(struct notifier_block *self, unsigned long,
void *);
struct list_head node;
int priority;
int enabled;
};

int notifier_call_chain(struct notifier_head *nh, unsigned long val,
void *v)
{
int ret = 0;
notifier_block *b;

if (list_empty(&nh->chain)) /* Optimize for common case */
return ret;

smp_rmb();
list_for_each_entry(b, &nh->chain, node) {
if (b->enabled) {
ret = b->notifier_call(b, val, v);
if (ret & NOTIFY_STOP_MASK)
break;
}
}

return ret;
}

#define notifier_block_enable(b) set_wmb((b)->enabled, 1)
#define notifier_block_disable(b) set_wmb((b)->enabled, 0)


It occurred to me that there _is_ a way to do unregister for atomic chains
without blocking. Add to struct notifier_head

atomic_t num_callers;

Then in notifier_call_chain, do atomic_inc(&nh->num_callers) at the start
and atomic_dec(&nh->num_callers) at the end. Finally, make unregister do
this:

int notifier_chain_unregister(struct notifier_head *nh,
struct notifier_block *n)
{
if (nh->type == ATOMIC_NOTIFIER) {
spin_lock(nh->lock);
list_del(&n->node);
smp_mb();
while (atomic_read(&nh->num_callers) > 0)
cpu_relax();
spin_unlock(nh->lock);
} else {
...
}
return 0;
}

I don't mean to suggest that this is better than using RCU, and with
notifier_block_disable it probably isn't needed. However it is worth
thinking about.

Alan Stern

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