linux-kernel - Re: [PATCH] sigqueue_free: fix the race with collect

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <46CF4DCB.6030304@windriver.com>
Date:	Fri, 24 Aug 2007 17:29:47 -0400
From:	taoyue <yue.tao@...driver.com>
To:	Oleg Nesterov <oleg@...sign.ru>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Alexey Dobriyan <adobriyan@...ru>, Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org, stable@...nel.org
Subject: Re: [PATCH] sigqueue_free: fix the race with collect_signal()

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