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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ZGvM2nqmft0IbB2U@moria.home.lan>
Date:   Mon, 22 May 2023 16:13:14 -0400
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
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 11:58:33AM -0700, Linus Torvalds wrote:
> On Mon, May 22, 2023 at 10:13 AM Kent Overstreet
> <kent.overstreet@...ux.dev> wrote:
> >
> 
> > +static inline unsigned u32_mask_to_ulong_bitnr(u32 mask)
> > +{
> > +       union ulong_u32 {
> > +               u32     v32;
> > +               ulong   vlong;
> > +       } v = { .v32 = mask };
> > +
> > +       return ilog2(v.vlong);
> 
> No, this is still wrong.
> 
> The above is actively undefined - the high bits of 'vlong' can contain
> random garbage. And you can't even fix it anyway, because even if you
> add a second 32-bit word and zero it, on big-endian architectures
> you'll get a result that is bigger than 32, and then when you do
> this:L

Uh, I think you're wrong on this one - structs with designated
initializers have unspecified fields initialized to 0, and 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.

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

00000000000002c0 <six_set_nospin>:
 2c0:	e8 00 00 00 00       	call   2c5 <six_set_nospin+0x5>
 2c5:	55                   	push   %rbp
 2c6:	48 89 e5             	mov    %rsp,%rbp
 2c9:	f0 80 4f 03 80       	lock orb $0x80,0x3(%rdi)
 2ce:	5d                   	pop    %rbp
 2cf:	c3                   	ret

meaning the compiler properly constant-propagated and didn't read
uninitialized memory. And I did the same test in userspace too, in the
unlikely event -ftrivial-auto-var-init=zero was affecting things.

> > +static inline void six_set_bitmask(struct six_lock *lock, u32 mask)
> > +{
> > +       unsigned bitnr = u32_mask_to_ulong_bitnr(mask);
> > +
> > +       if (!test_bit(bitnr, (unsigned long *) &lock->state))
> > +               set_bit(bitnr, (unsigned long *) &lock->state);
> 
> you're back to basically just undefined behaviour.
> 
> 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.

> We don't do locking by playing games like this. It's wrong.
> 
> You clearly don't even want the return value, so you're actually much
> better off just using an atomic_t and "atomic_or()", I suspect.
> 
> But these broken games with casting pointers to invalid types MUST END.

...but, atomic_or() is clearly the right way to do this :)