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

[Kent Overstreet]

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@xxxxxxxxx> 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 :)