linux-kernel - Re: [PATCH] ipc/sem: Fix race between to-be-woken task and waker

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 for Android: free password hash cracker in your pocket

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

Message-ID: <d16e4d82-8c5a-0663-8c00-32f4750f8a21@redhat.com>
Date:   Thu, 26 Sep 2019 14:12:01 -0400
From:   Waiman Long <longman@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Mathieu Malaterre <malat@...ian.org>,
        Davidlohr Bueso <dave@...olabs.net>, manfred@...orfullife.com
Subject: Re: [PATCH] ipc/sem: Fix race between to-be-woken task and waker

On 9/26/19 5:34 AM, Peter Zijlstra wrote:
> On Fri, Sep 20, 2019 at 11:54:02AM -0400, Waiman Long wrote:
>> While looking at a customr bug report about potential missed wakeup in
>> the system V semaphore code, I spot a potential problem.  The fact that
>> semaphore waiter stays in TASK_RUNNING state while checking queue status
>> may lead to missed wakeup if a spurious wakeup happens in the right
>> moment as try_to_wake_up() will do nothing if the task state isn't right.
>>
>> To eliminate this possibility, the task state is now reset to
>> TASK_INTERRUPTIBLE immediately after wakeup before checking the queue
>> status. This should eliminate the race condition on the interaction
>> between the queue status and the task state and fix the potential missed
>> wakeup problem.
> Bah, this code always makes my head hurt.
>
> Yes, AFAICT the pattern it uses has been broken since 0a2b9d4c7967,
> since that removed doing the actual wakeup from under the sem_lock(),
> which is what it relies on.

After having a second look at the code again, I probably misread the
code the first time around. In the sleeping path, there is a check of
queue.status and setting of task state both under the sem lock in the
sleeping path. So as long as setting of queue status is under lock, they
should synchronize properly.

It looks like queue status setting is under lock, but I can't use
lockdep to confirm that as the locking can be done by either the array
lock or in one of the spinlocks in the array. Are you aware of a way of
doing that?

Anyway, I do think we need to add some comment to clarify the situation
to avoid future confusion.

Cheers,
Longman