linux-kernel - Re: [RFC PATCH] sched/wait: Make interruptible exclusive waitqueue wakeups reliable

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: <20191209120852.GA5388@redhat.com>
Date:   Mon, 9 Dec 2019 13:08:53 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.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: [RFC PATCH] sched/wait: Make interruptible exclusive waitqueue
 wakeups reliable

On 12/09, Ingo Molnar wrote:
>
> * Linus Torvalds <torvalds@...ux-foundation.org> 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,

But note that it checks "condition" after prepare_to_wait_event(), if it is
true then ___wait_is_interruptible() won't be even called.

> it's possible that the wakeup event has already been
> > consumed, 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 the wakeup basically got forgotten
> > - all the other exclusive waiters will remain waiting.
>
> So the place that detects interruption is prepare_to_wait_event():

Yes,

> long prepare_to_wait_event(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry, int state)
> {
>         unsigned long flags;
>         long ret = 0;
>
>         spin_lock_irqsave(&wq_head->lock, flags);
>         if (signal_pending_state(state, current)) {
>                 /*
>                  * Exclusive waiter must not fail if it was selected by wakeup,
>                  * it should "consume" the condition we were waiting for.
>                  *
>                  * The caller will recheck the condition and return success if
>                  * we were already woken up, we can not miss the event because
>                  * wakeup locks/unlocks the same wq_head->lock.
>                  *
>                  * But we need to ensure that set-condition + wakeup after that
>                  * can't see us, it should wake up another exclusive waiter if
>                  * we fail.
>                  */
>                 list_del_init(&wq_entry->entry);
>                 ret = -ERESTARTSYS;

...

> I think we can indeed lose an exclusive event here, despite the comment
> that argues that we shouldn't: if we were already removed from the list

If we were already removed from the list and condition is true, we can't
miss it, ret = -ERESTARTSYS won't be used. This is what this part of the
comment above

	 * The caller will recheck the condition and return success if
	 * we were already woken up, we can not miss the event because
	 * wakeup locks/unlocks the same wq_head->lock.

tries to explain.

> then list_del_init() does nothing and loses the exclusive event AFAICS.

list_del_init() ensures that wake_up() can't pick this task after
prepare_to_wait_event() returns.

IOW. Suppose that ___wait_event() races with

	condition = true;
	wake_up();

if wake_up() happens before prepare_to_wait_event(), __wait_event() will
see condition == true, -ERESTARTSYS returned by prepare_to_wait_event() has
no effect.

If wake_up() comes after prepare_to_wait_event(), the task was already removed
from the list, another exclusive waiter (if any) will be woken up. In this case
__wait_event() can return success or -ERESTARTSYS, both are correct.

No?

Oleg.