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

[taoyue <yue.tao@windriver.com>]

Oleg Nesterov wrote:
> On 08/24, taoyue wrote:
>   
>> Oleg Nesterov wrote:
>>     
>>> --- 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);
>>> -		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);
>>> }
>>>
>>>
>>>  
>>>       
>>    Applying previous patch???it seems likely that the __sigqueue_free() is 
>>    also called twice.
>>
>> collect_signal:				sigqueue_free:
>>
>> 	list_del_init(&first->list);
>>                                        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(first);		__sigqueue_free(q);
>>     
>
> collect_signal() is always called under ->siglock which is also taken by
> sigqueue_free(), so this is not possible.
>
> Basically, this patch is the same one-liner I sent you before
>
> 	http://marc.info/?l=linux-kernel&m=118772206603453&w=2
>
> (Thanks for the additional testing and report, btw).
>
> P.S. It would be nice to know if this patch solves the problems reported
> by Jeremy, but his email is disabled.
>
> Oleg.
>
>   
I know, using current->sighand->siglock to prevent one sigqueue
is free twice. I want to know whether it is possible that the two
function is called in different thread. If that, the spin_lock is useless.

yue.tao