linux-kernel - Re: [PATCH 2/4] swait: add the missing killable swaits

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: <20170630173048.GA2392@templeofstupid.com>
Date:   Fri, 30 Jun 2017 10:30:48 -0700
From:   Krister Johansen <kjlx@...pleofstupid.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Marcelo Tosatti <mtosatti@...hat.com>, h@....cnet,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg KH <gregkh@...uxfoundation.org>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Martin Fuzzey <mfuzzey@...keon.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Daniel Wagner <wagi@...om.org>,
        David Woodhouse <dwmw2@...radead.org>,
        jewalt@...innovations.com, rafal@...ecki.pl,
        Arend Van Spriel <arend.vanspriel@...adcom.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        "Li, Yi" <yi1.li@...ux.intel.com>, atull@...nel.org,
        Moritz Fischer <moritz.fischer@...us.com>,
        Petr Mladek <pmladek@...e.com>,
        Johannes Berg <johannes.berg@...el.com>,
        Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
        "Coelho, Luciano" <luciano.coelho@...el.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Andrew Lutomirski <luto@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        "AKASHI, Takahiro" <takahiro.akashi@...aro.org>,
        David Howells <dhowells@...hat.com>,
        Peter Jones <pjones@...hat.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Alan Cox <alan@...ux.intel.com>, Theodore Ts'o <tytso@....edu>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        Linux API <linux-api@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "stable # 4 . 6" <stable@...r.kernel.org>
Subject: Re: [PATCH 2/4] swait: add the missing killable swaits

On Thu, Jun 29, 2017 at 09:03:42PM -0700, Linus Torvalds wrote:
> On Thu, Jun 29, 2017 at 12:15 PM, Marcelo Tosatti <mtosatti@...hat.com> wrote:
> > On Thu, Jun 29, 2017 at 09:13:29AM -0700, Linus Torvalds wrote:
> >>
> >> swait uses special locking and has odd semantics that are not at all
> >> the same as the default wait queue ones. It should not be used without
> >> very strong reasons (and honestly, the only strong enough reason seems
> >> to be "RT").
> >
> > Performance shortcut:
> >
> > https://lkml.org/lkml/2016/2/25/301
> 
> Now, admittedly I don't know the code and really may be entirely off,
> but looking at the commit (no need to go to the lkml archives - it's
> commit 8577370fb0cb ("KVM: Use simple waitqueue for vcpu->wq") in
> mainline), I really think the swait() use is simply not correct if
> there can be multiple waiters, exactly because swake_up() only wakes
> up a single entry.
> 
> So either there is only a single entry, or *all* the code like
> 
>         dvcpu->arch.wait = 0;
> 
> -       if (waitqueue_active(&dvcpu->wq))
> -               wake_up_interruptible(&dvcpu->wq);
> +       if (swait_active(&dvcpu->wq))
> +               swake_up(&dvcpu->wq);
> 
> is simply wrong. If there are multiple blockers, and you just cleared
> "arch.wait", I think they should *all* be woken up. And that's not
> what swake_up() does.

Code like this is probably wrong for another reason too.  The
swait_active() is likely redudant, since swake_up() also calls
swait_active().  The check in swake_up() returns if it thinks there are
no active waiters.  However, the synchronization needed to ensure a
proper wakeup is left as an exercise to swake_up's caller.

There have been a couple of other discussions around this topic
recently:

https://lkml.org/lkml/2017/5/25/722
https://lkml.org/lkml/2017/6/8/1222

The above is better written as the following, but even then you still
have the single/multiple wakeup problem:

 -       if (waitqueue_active(&dvcpu->wq))
 -               wake_up_interruptible(&dvcpu->wq);
 +       smp_mb();
 +       swake_up(&dvcpu->wq);


Just to add to the confusion, the last time I checked, the semantics of
swake_up() even differ between RT Linux and mainline, which makes this
even more confusing.

-K