linux-kernel - Re: [GIT PULL] locking/urgent for v5.12

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]

Date:   Sun, 25 Apr 2021 09:37:20 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Borislav Petkov <bp@...e.de>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Ali Saidi <alisaidi@...zon.com>
Cc:     x86-ml <x86@...nel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] locking/urgent for v5.12

[ Side note: this is cc'd to x86-ml, even though x86 is the _one_
architecture that was guaranteed to be not at all affected by the
actual locking bug, since a locked op is always ordered on x86. ]

On Sun, Apr 25, 2021 at 2:39 AM Borislav Petkov <bp@...e.de> wrote:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/locking_urgent_for_v5.12
>
> - Fix ordering in the queued writer lock's slowpath.

So I'm looking at that change, because the code is confusing.

Why did it add that "cnts" variable? We know it must have the value
_QW_WAITING, since that's what the atomic_cond_read_relaxed() waits
for.

I'm assuming it's because of the switch to try_cmpxchg by PeterZ?

HOWEVER.

That actually just makes the code even MORE unreadable.

That code was odd and hard to read even before, but now it's
positively confusing.

New confusion:
 - Why is the truly non-critical cmpxchg using "try_cmpxhg()", when
the _first_ cmpxchg - above the loop - is not?

Pre-existing confusion:
 - Why is the code using "atomic_add()" to set a bit?

Yeah, yeah, neither of these are *bugs*, but Christ is that code hard
to read. The "use add to set a bit" is valid because of the spinlock
serialization (ie only one add can ever happen), and the
cmpxchg-vs-try_cmpxchg confusion isn't buggy, it's just really really
confusing that that same function is using two different - but
equivalent - cmpxchg things on the same variable literally a couple of
lines apart.

I've pulled this, but can we please

 - make *both* of the cmpxchg's use "try_cmpxchg()" (and thus that
"cnts" variable)?

 - add a comment about _why_ it's doing "atomic_add()" instead of the
much more logical "atomic_or()", and about how the spinlock serializes
it

I'm assuming the "atomic_add()" is simply because many more
architectures have that as an actual intrinsic atomic. I understand.
But it's really really not obvious from the code.

                Linus