linux-kernel - Re: [PATCH] firmware: wake all waiters

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: <CA+55aFxivLtaFEur99eZpoafcweK1WNvSu2vEvxXvj5gKB8CdA@mail.gmail.com>
Date:   Mon, 26 Jun 2017 14:44:17 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        oss-drivers@...ronome.com
Subject: Re: [PATCH] firmware: wake all waiters

On Fri, Jun 23, 2017 at 4:37 PM, Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
> -               swake_up(&fw_st->wq);
> +               swake_up_all(&fw_st->wq);

Why is that code using the braindamaed "swait" model in the first place?

That's the real problem here - swait() is a very specialized
interface, and it does not make sense to use it here.

Among all the simplifications it has is exactly the fact that it wakes
up only one thing, because it is *so* specialized.

But the *only* reason for swait is extreme memory issues and some very
special realtime issues, where it saves a couple of bytes in
structures that need close packing, and doesn't even use normal
spinlocks, so it saves a couple of cycles at wakeup/sleep because it
doesn't do a good job in general.

The "avoid normal spinlocks" is because it is meant for code that is
*so* special that it needs the magical low-level raw spinlocks.

I really have *no* idea why the firmware code uses that idiotic
special wait-queue. It has no reason to do so, except this comment
from the commit that added it:

  "We use also swait instead of wait because don't need all the additional
    features wait provides."

which is bogus, since it clearly just got the waiting wrong exactly
*because* swait is pretty damn bad and specialized.

I think the two valid users are RCU (which needed it for RT), and kvm
(which also needed it for similar issues - it needs to be
non-preemptible).

I don't see any similar reason for the firmware loading, and all it
did was use an odd interface that resulted in this bug.

Why is the firmware code being so damn odd on purpose?

           Linus