Re: Notifier chains are unsafe

[Chandra Seetharaman <sekharan@us.ibm.com>]

On Mon, 2005-10-24 at 16:48 -0400, Alan Stern wrote:

Hi Alan,

I agree with your approach of having a notifier_head to have both the
head of the notifier_list and the corresponding lock (instead of having
a single global rwlock to protect all notifier lists).

But, I am confused about the need for three data structures and two next
pointers. I think we can achieve the same by 2 data structures:

	notifier_head {
		spinlock_t lock;
		list_head  head;
	};
	notifier_block {
		int (*notifier_call)(struct notifier_block *self,
			unsigned long, void *);
		list_head lists;
		int priority;
	};

I think that having multiple data structures make the code hard to
follow.

No. of register/unregister would be a lot lesser than a
notifier_call_chain() calls, so IMHO, rwlock would be a better option.	

<snip>
>  /**
>   *	notifier_call_chain - Call functions in a notifier chain
> - *	@n: Pointer to root pointer of notifier chain
> + *	@nh: Pointer to head of the notifier chain
>   *	@val: Value passed unmodified to notifier function
>   *	@v: Pointer passed unmodified to notifier function
>   *
> @@ -167,20 +194,28 @@ EXPORT_SYMBOL(notifier_chain_unregister)
>   *	of the last notifier function called.
>   */
>   
> -int notifier_call_chain(struct notifier_block **n, unsigned long val, void *v)
> +int notifier_call_chain(struct notifier_head *nh, unsigned long val, void *v)
>  {
> -	int ret=NOTIFY_DONE;
> -	struct notifier_block *nb = *n;
> +	int ret = NOTIFY_DONE;
> +	struct notifier_caller caller;
> +	struct notifier_block *n;
>  
> -	while(nb)
> -	{
> -		ret=nb->notifier_call(nb,val,v);
> -		if(ret&NOTIFY_STOP_MASK)
> -		{
> -			return ret;
> -		}
> -		nb=nb->next;
> +	spin_lock(&nh->lock);
> +	caller.next = nh->first;
> +	list_add(&caller.node, &nh->callers);
> +
> +	while (caller.next) {
> +		n = caller.next;
> +		caller.next = n->next;
> +		spin_unlock(&nh->lock);
> +		ret = n->notifier_call(n, val, v);
> +		spin_lock(&nh->lock);
> +		if (ret & NOTIFY_STOP_MASK)
> +			break;
>  	}

Since the lock is being dropped while calling notifier_call, how are we
guaranteed caller.next is valid ? It might have been unregistered.
> +
> +	list_del(&caller.node);
> +	spin_unlock(&nh->lock);
>  	return ret;
>  }
>  

-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------