Re: Notifier chains are unsafe

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

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/