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]

Date:   Mon, 22 May 2023 11:58:33 -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 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

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