Re: [PATCH] sigqueue_free: fix the race with collect_signal()

[Sukadev Bhattiprolu <sukadev@us.ibm.com>]

Oleg Nesterov wrote:
> Spotted by taoyue <yue.tao@windriver.com> and Jeremy Katz <jeremy.katz@windriver.com>.
>
> collect_signal:				sigqueue_free:
>
> 	list_del_init(&first->list);
> 						if (!list_empty(&q->list)) {
> 							// not taken
> 						}
> 						q->flags &= ~SIGQUEUE_PREALLOC;
>
> 	__sigqueue_free(first);			__sigqueue_free(q);
>
> Now, __sigqueue_free() is called twice on the same "struct sigqueue" with the
> obviously bad implications.
>
> In particular, this double free breaks the array_cache->avail logic, so the
> same sigqueue could be "allocated" twice, and the bug can manifest itself via
> the "impossible" BUG_ON(!SIGQUEUE_PREALLOC) in sigqueue_free/send_sigqueue.
>
> Hopefully this can explain these mysterious bug-reports, see
>
> 	http://marc.info/?t=118766926500003
> 	http://marc.info/?t=118466273000005
>
> Alexey Dobriyan reports this patch makes the difference for the testcase, but
> nobody has an access to the application which opened the problems originally.
>
> Also, this patch removes tasklist lock/unlock, ->siglock is enough.
>
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
> --- t/kernel/signal.c~SQFREE	2007-08-22 20:06:31.000000000 +0400
> +++ t/kernel/signal.c	2007-08-23 16:02:57.000000000 +0400
> @@ -1297,20 +1297,19 @@ struct sigqueue *sigqueue_alloc(void)
>  void sigqueue_free(struct sigqueue *q)
>  {
>  	unsigned long flags;
> +	spinlock_t *lock = &current->sighand->siglock;
> +
>  	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>  	/*
>  	 * If the signal is still pending remove it from the
> -	 * pending queue.
> +	 * pending queue. We must hold ->siglock while testing
> +	 * q->list to serialize with collect_signal().
>  	 */
> -	if (unlikely(!list_empty(&q->list))) {
> -		spinlock_t *lock = &current->sighand->siglock;
> -		read_lock(&tasklist_lock);
> -		spin_lock_irqsave(lock, flags);
>   
Hmm, but the existing code _does_ take the siglock here. Is that not 
sufficient ?
Isn't the first list_empty() check without lock only an optimization for 
the common
case ?

> -		if (!list_empty(&q->list))
> -			list_del_init(&q->list);
> -		spin_unlock_irqrestore(lock, flags);
> -		read_unlock(&tasklist_lock);
> -	}
> +	spin_lock_irqsave(lock, flags);
> +	if (!list_empty(&q->list))
> +		list_del_init(&q->list);
> +	spin_unlock_irqrestore(lock, flags);
> +
>  	q->flags &= ~SIGQUEUE_PREALLOC;
>  	__sigqueue_free(q);
>  }
>
> -
> 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/
>
>