sigqueue_free: fix the race with collect_signal()

[Linux Kernel Mailing List]

Gitweb:     
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=60187d2708caa870f0825d753df1612ea688eb9e
Commit:     60187d2708caa870f0825d753df1612ea688eb9e
Parent:     99db67bc04af0f2e8cb710ac92aaeb9af135a7c6
Author:     Oleg Nesterov <[EMAIL PROTECTED]>
AuthorDate: Thu Aug 30 23:56:35 2007 -0700
Committer:  Linus Torvalds <[EMAIL PROTECTED]>
CommitDate: Fri Aug 31 01:42:23 2007 -0700

    sigqueue_free: fix the race with collect_signal()
    
    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.
    
    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 <[EMAIL PROTECTED]>
    Cc: taoyue <[EMAIL PROTECTED]>
    Cc: Jeremy Katz <[EMAIL PROTECTED]>
    Cc: Sukadev Bhattiprolu <[EMAIL PROTECTED]>
    Cc: Alexey Dobriyan <[EMAIL PROTECTED]>
    Cc: Ingo Molnar <[EMAIL PROTECTED]>
    Cc: Thomas Gleixner <[EMAIL PROTECTED]>
    Cc: Roland McGrath <[EMAIL PROTECTED]>
    Cc: <[EMAIL PROTECTED]>
    Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
    Signed-off-by: Linus Torvalds <[EMAIL PROTECTED]>
---
 kernel/signal.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index ad63109..3169bed 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1300,20 +1300,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);
 }
-
To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html