linux-kernel - Re: Fundamental race condition in wait_event_interruptible

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20191209173820.GA11415@redhat.com>
Date:   Mon, 9 Dec 2019 18:38:20 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Miklos Szeredi <miklos@...redi.hu>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Felipe Balbi <balbi@...nel.org>
Subject: Re: Fundamental race condition in
 wait_event_interruptible_exclusive() ?

I have alredy replied to Ingo, but if I was not clear...

On 12/08, Linus Torvalds wrote:
>
> The reason it is buggy is that wait_event_interruptible_exclusive()
> does this (inside the __wait_event() macro that it expands to):
>
>                 long __int = prepare_to_wait_event(&wq_head,
> &__wq_entry, state);\
>
>          \
>                 if (condition)
>          \
>                         break;
>          \
>
>          \
>                 if (___wait_is_interruptible(state) && __int) {
>          \
>                         __ret = __int;
>          \
>                         goto __out;
>          \
>
> and the thing is, if does that "__ret = __int" case and returns
> -ERESTARTSYS, it's possible that the wakeup event has already been
> consumed,

Afaics, no.

> because we've added ourselves as an exclusive writer to the
> queue. So it _says_ it was interrupted, not woken up, and the wait got
> cancelled, but because we were an exclusive waiter, we might be the
> _only_ thing that got woken up,

And that is why ___wait_event() always checks the condition after
prepare_to_wait_event(), whatever it returns.

And. If it actually does "__ret = __int" and returns -ERESTARTSYS, then
this task was already removed from the list, so we should not worry about
the case when wake_up() comes after prepare_to_wait_event().

> And the basic point is that the return value
> from wait_event_interruptible_exclusive() seems to not really be
> reliable. You can't really use it -

see above ...

> even if it says you got
> interrupted, you still have to go back and check the condition and do
> the work, and only do interruptability handling after that.

This is exactly what ___wait_event() does. Even if prepare_to_wait_event()
says you got interrupted, it still checks the condition and returns success
if it is true.

Oleg.