linux-kernel - Re: [PATCH v2] locking: SIX locks (shared/intent/exclusive)

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wgxN65O-F3R5detmma2MDjDRn4S_qkeKVCCp95c5+gYmA@mail.gmail.com>
Date:   Mon, 22 May 2023 13:53:36 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Kent Overstreet <kent.overstreet@...ux.dev>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Brian Foster <bfoster@...hat.com>,
        Dave Chinner <dchinner@...hat.com>
Subject: Re: [PATCH v2] locking: SIX locks (shared/intent/exclusive)

On Mon, May 22, 2023 at 1:13 PM Kent Overstreet
<kent.overstreet@...ux.dev> wrote:
>
> Uh, I think you're wrong on this one - structs with designated
> initializers have unspecified fields initialized to 0

That's definitely true for unspecified fields.

But there *are* no fields. Only padding.

That said, gcc does seem to always initialize the whole thing. We even
rely on it when it comes to structures, but I'm not sure it's strictly
standardized - and I'm particularly not sure about unions.

But there are counter-examples, like this:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104607

so maybe even structures aren't always initialized fully?

Anyway, I *really* don't want locking to have something that isn't
obviously true. And it's not obviously true what the union behavior
is.

> I would
> expect the same to hold for unions, and the language in the C standard
> isn't completely explicit but it appears to apply to both.

Only the named member (or, if no name, first) of a union is
initialized, not all members. The rest is padding that may or may not
be initialized depending on how you read the standard.

> And checking the generated assembly for a six_set_nospin() that calls a
> six_set_bitmask() with the test_bit() taken out, for simplicity

Random compiler issue, *and* you're testing on a little-endian
platform that wouldn't show the issue anyway.

> meaning the compiler properly constant-propagated and didn't read
> uninitialized memory.

Again, that only works because it's little-endian, so the low bits of
"vlong" are the same as the low bits of "v32".

Which is not true on BE.

On BE, it *might* work, because who knows what the compiler does for
the padding bytes, but again, it's broken.

It also doesn't even *matter*, because the initialization is only
*one* of the multiple problems here:

> > You *cannot* do "set_bit()" on a u32. It's that simple. Stop trying to
> > do it with these wrappers that happen to work on x86 but are
> > fundamentally broken.
>
> Because of aliasing issues? I thought it had been declared that the
> kernel would never do strict aliasing.

No, Because of *alignment* issues.

An u32 is 4-byte aligned. But "set_bit()" requires "long" alignmnent,.

Again, that doesn't happen to matter on x86, at least when the
"set_bit()" ends up being done as an "orb".

But it will basically not work on other acrchitectures.

You really can't just randomly cast pointers in locking code and think
it makes things ok just because the compiler doesn't complain.

There's a *reason* that "set_bit()" takes a "long *" value, not a
"void *" value. It's literally part of the semantics of the thing, and
a cast does not make things magically ok.

So STOP DOING THOSE UNION TRICKS.

They aren't the "cure trick" you seem to think they are.

They are only broken garbage.

                 Linus