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

[taoyue]

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