lkml.org 
[lkml]   [2007]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sigqueue_free: fix the race with collect_signal()
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);
> - 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);



yue.tao
-
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/

\
 
 \ /
  Last update: 2007-08-24 04:33    [W:0.064 / U:0.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site