linux-kernel - Re: [PATCH v1 2/2] cleanup.h: Introduce DEFINE_INACTIVE_GUARD and activate

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHk-=wgRefOSUy88-rcackyb4Ss3yYjuqS_TJRJwY_p7E3r0SA@mail.gmail.com>
Date: Mon, 2 Sep 2024 11:46:49 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org, 
	Kees Cook <keescook@...omium.org>, Greg KH <gregkh@...uxfoundation.org>, 
	Sean Christopherson <seanjc@...gle.com>, Steven Rostedt <rostedt@...dmis.org>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Alexei Starovoitov <ast@...nel.org>, Yonghong Song <yhs@...com>, 
	"Paul E . McKenney" <paulmck@...nel.org>, Arnaldo Carvalho de Melo <acme@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Namhyung Kim <namhyung@...nel.org>, 
	bpf@...r.kernel.org, Joel Fernandes <joel@...lfernandes.org>, 
	linux-trace-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v1 2/2] cleanup.h: Introduce DEFINE_INACTIVE_GUARD and activate_guard

On Mon, 2 Sept 2024 at 11:08, Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
> If Linus' objections were mainly about naming, perhaps what I am
> suggestion here may be more to his liking ?

Most of my objections were about having subtle features that then had
very subtle syntax and weren't that clear.

And yes, a small part of it was naming (I absolutely hated the initial
"everything is a guard" thing, when one of the main uses were about
automatic freeing of variables).

But a much larger part was about making the new use greppable and have
really simple syntax.

And the conditional case was never that simple syntax, and I feel it
really violates the whole "this scope is protected".

And no, I do not like Peter's "if_guard()" either.

Honestly, I get the feeling that this is all wrong.

For example, I searched for a few of the places where you wanted to
use this, and see code like this:

    #define __BPF_DECLARE_TRACE(call, proto, args, tp_flags)            \
    static notrace void                                                 \
    __bpf_trace_##call(void *__data, proto)                             \
    {                                                                   \
        DEFINE_INACTIVE_GUARD(preempt_notrace, bpf_trace_guard);        \
                                                                        \
        if ((tp_flags) & TRACEPOINT_MAY_FAULT) {                        \
                might_fault();                                          \
                activate_guard(preempt_notrace, bpf_trace_guard)();     \
        }                                                               \
                                                                        \
        CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data,
CAST_TO_U64(args)); \
    }

and it makes me just go "that's just *WRONG*".

That code should never EVER use a conditional guard.

I find *two* uses of this in your patches, and both of them look
com,pletely wrong to me, because you could have written the code
*without* that conditional activation, and it would have been *better*
that way.

IOW, that code should just have been something like this:

    #define __BPF_DECLARE_TRACE(call, proto, args, tp_flags)    \
    static notrace void                                         \
    __bpf_trace_##call(void *__data, proto)                     \
    {                                                           \
                                                                \
        if ((tp_flags) & TRACEPOINT_MAY_FAULT) {                \
                might_fault();                                  \
                guard(preempt_notrace)();                       \
                CONCATENATE(bpf_trace_run, ...                  \
                return;                                         \
        }                                                       \
        CONCATENATE(bpf_trace_run, ...                          \
    }

instead.

Sure, it duplicates the code inside the guard, but what's the
downside? In both of the cases I saw, the "duplicated" code was
trivial.

And the *upside* is that you don't have any conditional locking or
guarding, and you don't have to make up ridiculous and meaningless
temporary names for the guard etc. So you get to use the *LEGIBLE*
code.

And you don't have a patch that just renames all our existing uses.
Which was also wrong.

So no, I don't like Peter's "if_guard()",  but I find your conditional
activation to be absolutely wrogn and horrible too.

                 Linus