linux-kernel - Re: [PATCH 0/9 RFC] Make wake_up

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: <172419045605.6062.3170152948140066950@noble.neil.brown.name>
Date: Wed, 21 Aug 2024 07:47:36 +1000
From: "NeilBrown" <neilb@...e.de>
To: "Linus Torvalds" <torvalds@...ux-foundation.org>
Cc: "Ingo Molnar" <mingo@...hat.com>, "Peter Zijlstra" <peterz@...radead.org>,
 linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 0/9 RFC] Make wake_up_{bit,var} less fragile

On Mon, 19 Aug 2024, Linus Torvalds wrote:
> On Sun, 18 Aug 2024 at 22:36, NeilBrown <neilb@...e.de> wrote:
> >
> > The main patches here are 7 and 8 which revise wake_up_bit and
> > wake_up_var respectively.  They result in 3 interfaces:
> >   wake_up_{bit,var}           includes smp_mb__after_atomic()
> 
> I actually think this is even worse than the current model, in that
> now it subtle only works after atomic ops, and it's not obvious from
> the name.
> 
> At least the current model, correct code looks like
> 
>       do_some_atomic_op
>       smp_mb__after_atomic()
>       wake_up_{bit,var}
> 
> and the smp_mb__after_atomic() makes sense and pairs with the atomic.
> So the current one may be complex, but at the same time it's also
> explicit. Your changed interface is still complex, but now it's even
> less obvious what is actually going on.
> 
> With your suggested interface, a plain "wake_up_{bit,var}" only works
> after atomic ops, and other ops have to magically know that they
> should use the _mb() version or whatever. And somebody who doesn't
> understand that subtlety, and copies the code (but changes the op from
> an atomic one to something else) now introduces code that looks fine,
> but is really subtly wrong.
> 
> The reason for the barrier is for the serialization with the
> waitqueue_active() check. Honestly, if you worry about correctness
> here, I think you should leave the existing wake_up_{bit,var}() alone,
> and concentrate on having helpers that do the whole "set and wake up".
> 
> IOW, I do not think you should change existing semantics, but *this*
> kind of pattern:
> 
> >  [PATCH 2/9] Introduce atomic_dec_and_wake_up_var().
> >  [PATCH 9/9] Use clear_and_wake_up_bit() where appropriate.
> 
> sounds like a good idea.
> 
> IOW, once you have a whole "atomic_dec_and_wake_up()" (skip the "_var"
> - it's implied by the fact that it's an atomic_dec), *then* that
> function makes for a simple-to-use model, and now the "atomic_dec(),
> the smp_mb__after_atomic(), and the wake_up_var()" are all together.
> 
> For all the same reasons, it makes total sense to have
> "clear_bit_and_wake()" etc.

I can definitely get behind the idea has having a few more helpers and
using them more widely.  But unless we get rid of wake_up_bit(), people
will still use and some will use it wrongly.

What would you think of changing wake_up_bit/var to always have a full
memory barrier, and adding wake_up_bit/var_relaxed() for use when a
different barrier, or not barrier, is wanted?

Thanks,
NeilBrown


> 
> But exposing those "three different memory barrier scenarios" as three
> different helpers is the *opposite* of helpful. It keeps the current
> complexity, and makes it worse by making the barrier rules even more
> opaque, imho.
> 
>                Linus
>