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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@mail.gmail.com>
Date: Tue, 3 Sep 2024 12:00:05 -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 Tue, 3 Sept 2024 at 06:42, Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
> If we look at perf_trace_##call(), with the conditional guard, it looks
> like the following. It is not clear to me that code duplication would
> be acceptable here.

Sure it is.

Admittedly, I think it would look a *lot* better off with that macro
creating a helper function for the common part, and then just call
that helper function in two places.

In fact, I think the existing "perf_trace_##call()" function *is* that
helper function already, and all it needs is

 (a) add the traditional double underscore (or "do_") to that function

 (b) create the wrapper function.

IOW, I think the diff would look something like this:

--- a/include/trace/perf.h
+++ b/include/trace/perf.h
@@ -15,7 +15,7 @@
 #undef DECLARE_EVENT_CLASS
 #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
 static notrace void                                                    \
-perf_trace_##call(void *__data, proto)                                 \
+__perf_trace_##call(void *__data, proto)                               \
 {                                                                      \
        struct trace_event_call *event_call = __data;                   \
        struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\
@@ -53,6 +53,17 @@ perf_trace_##call(void *__data, proto)
                         \
        perf_trace_run_bpf_submit(entry, __entry_size, rctx,            \
                                  event_call, __count, __regs,          \
                                  head, __task);                        \
+}                                                                      \
+static notrace void                                                    \
+perf_trace_##call(void *__data, proto)                                 \
+{                                                                      \
+        if ((tp_flags) & TRACEPOINT_MAY_FAULT) {                        \
+                might_fault();                                          \
+                guard(preempt_notrace)();                              \
+                __perf_trace_##call(__data, proto);                    \
+                return;
         \
+        }                                                              \
+        __perf_trace_##call(__data, proto);                            \
 }

 /*

and notice how there is no actual duplicated code on a source level
now (although the compiler may obviously do the inlining and
duplication - although I think that the tp_flags are always constant,
so what actually hopefully happens is that the compiler doesn't
duplicate anything at all, and all the conditionals just go away.).

NOTE! The above patch is garbage. I did it as a "like this" patch on
my current tree, and it doesn't even have the "tp_class" thing at all,
so the patch is obviously not functional. It's just meant as a "look,
wrapper function".

In fact, I think the wrapping might even be done at a higher level (ie
maybe the whole "may_fault" shouldn't be an argument at all, but a
static part of the define, in case some event classes can take faults
and others cannot?)

I dunno. I actually think the whole "TRACEPOINT_MAY_FAULT" thing is an
incredibly ugly hack, and SHOULD NOT EXIST.

In other words - why make these incredibly ugly macros EVEN MORE UGLY
- when there is a single use-case and we sure as hell don't want to
add any more?

IOW - why not get rid of the stupid TRACE_EVENT_FN_MAY_FAULT thing
entirely, and do this trivial wrapping in the *one* place that wants
it, instead of making it look like some generic thing with an
allegedly generic test, when it is anything but generic, and is in
fact just a single special case for system call.

Yes, I know. Computer Science classes say that "generic programming is
good". Those CS classes are garbage. You want to make your code as
specialized as humanly possible, and *not* make some complicated "this
handles all cases" code that is complicated and fragile.

                            Linus