On 08/23, Sukadev Bhattiprolu wrote:
>
> Oleg Nesterov wrote:
> >Spotted by taoyue <[EMAIL PROTECTED]> and Jeremy Katz
> ><[EMAIL PROTECTED]>.
> >
> >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.
> >
> >--- 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 = ¤t->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 = ¤t->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 ?
Yes, it does, and this is sufficient, so the patch removes tasklist_lock.
> Isn't the first list_empty() check without lock only an optimization for
> the common
> case ?
Yes, this is optimization, but I strongly believe it is wrong. Please look
at the race description above.
!list_empty(&q->list) does _not_ necessary mean that q is not used and we can
free it. It is possible that collect_signal() just removed this sigqueue from
list (list_empty(&q->list) becomes true) and going to free it.
The whole point is: we can't check list_empty() without ->siglock, this is
racy, and leads to double-free.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
