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

[Linus Torvalds]

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

> +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.

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.

Locking is subtle enough without doing clearly bogus things that
depend on byte order and word size, and that aren't defined for the
type you want to use.

            Linus