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]

Message-ID: <9017e7b9-cd6e-caa1-b8ba-8d85f4a5d87e@redhat.com>
Date:   Sun, 25 Apr 2021 13:06:52 -0400
From:   Waiman Long <llong@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Borislav Petkov <bp@...e.de>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Ali Saidi <alisaidi@...zon.com>,
        Steve Capper <steve.capper@....com>,
        Will Deacon <will@...nel.org>
Cc:     x86-ml <x86@...nel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] locking/urgent for v5.12

On 4/25/21 12:39 PM, Linus Torvalds wrote:
> Oh, and replying to myself only because I spazzed out and pressed
> "send" before I had filled out the full participants line.
>
> Sorry for the duplicate message quoted in full below.
>
>                   Linus
>
> On Sun, Apr 25, 2021 at 9:37 AM Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
>> [ 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?

Yes, try_cmpxchg() requires a variable to hold the new value as well as 
a place to return the actual value before the cmpxchg(). It is just the 
way try_cmpxchg() works.


>>
>> 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?
At least for x86, try_cmpxchg() seems to produce a slight better 
assembly code than the regular cmpxchg(). I guess that may be one of the 
reason Peter changed it to use try_cmpxchg(). Another reason that I can 
think of is to make the code fit in one line instead of splitting it up 
into two lines like the original version from Ali.
>>
>> 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.
As you have said, the spinlock serialization makes sure that only 1 
writer is allowed to do that. I agree that using atomic_or() looks 
better in this case. Both of them are equivalent in this particular case.
>>
>> I've pulled this, but can we please
>>
>>   - make *both* of the cmpxchg's use "try_cmpxchg()" (and thus that
>>     "cnts" variable)?
Yes, we can certainly change the other cmpxchg() to try_cmpxchg().
>>
>>   - 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.
>>
I will post a patch to make the suggested change to qrwlock.c.

Cheers,
Longman