linux-kernel - Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO

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: <d36281ef-bb8f-4b87-9867-8ac1752ebc1c@efficios.com>
Date: Mon, 25 Nov 2024 09:18:18 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org,
 Thomas Gleixner <tglx@...utronix.de>, Michael Jeanson
 <mjeanson@...icios.com>, Masami Hiramatsu <mhiramat@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>, Alexei Starovoitov <ast@...nel.org>,
 Yonghong Song <yhs@...com>, "Paul E . McKenney" <paulmck@...nel.org>,
 Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Namhyung Kim <namhyung@...nel.org>,
 Andrii Nakryiko <andrii.nakryiko@...il.com>, bpf@...r.kernel.org,
 Joel Fernandes <joel@...lfernandes.org>, Jordan Rife <jrife@...gle.com>,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 4/5] tracing: Remove conditional locking from
 __DO_TRACE()

On 2024-11-23 12:38, Linus Torvalds wrote:
> On Sat, 23 Nov 2024 at 07:31, Mathieu Desnoyers
> <mathieu.desnoyers@...icios.com> wrote:
>>
>>   include/linux/tracepoint.h | 45 ++++++++++----------------------------
>>   1 file changed, 12 insertions(+), 33 deletions(-)
> 
> Thanks. This looks much more straightforward, and obviously is smaller too.
> 
> Side note: I realize I was the one suggesting "scoped_guard()", but
> looking at the patch I do think that just unnecessarily added another
> level of indentation. Since you already wrote the
> 
>      if (cond) {
>          ..
>      }
> 
> part as a block statement, there's no upside to the guard having its
> own scoped block, so instead of
> 
>      if (cond) { \
>          scoped_guard(preempt_notrace)           \
>              __DO_TRACE_CALL(name, TP_ARGS(args)); \
>      }
> 
> this might be simpler as just a plain "guard()" and one less indentation:
> 
>      if (cond) { \
>          guard(preempt_notrace);           \
>          __DO_TRACE_CALL(name, TP_ARGS(args)); \
>      }
> 
> but by now this is just an unimportant detail.
> 
> I think I suggested scoped_guard() mainly because that would then just
> make the "{ }" in the if-statement superfluous, but that's such a
> random reason that it *really* doesn't matter.

I tried the following alteration to the code, which triggers an
unexpected compiler warning on master, but not on v6.12. I suspect
this is something worth discussing:

         static inline void trace_##name(proto)                          \
         {                                                               \
                 if (static_branch_unlikely(&__tracepoint_##name.key)) { \
                         if (cond)                                       \
                                 scoped_guard(preempt_notrace)           \
                                         __DO_TRACE_CALL(name, TP_ARGS(args)); \
                 }                                                       \
                 if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {             \
                         WARN_ONCE(!rcu_is_watching(),                   \
                                   "RCU not watching for tracepoint");   \
                 }                                                       \
         }

It triggers this warning with gcc version 12.2.0 (Debian 12.2.0-14):

In file included from ./include/trace/syscall.h:5,
                  from ./include/linux/syscalls.h:94,
                  from init/main.c:21:
./include/trace/events/tlb.h: In function ‘trace_tlb_flush’:
./include/linux/tracepoint.h:261:28: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
   261 |                         if (cond)                                       \
       |                            ^
./include/linux/tracepoint.h:446:9: note: in expansion of macro ‘__DECLARE_TRACE’
   446 |         __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),              \
       |         ^~~~~~~~~~~~~~~
./include/linux/tracepoint.h:584:9: note: in expansion of macro ‘DECLARE_TRACE’
   584 |         DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
       |         ^~~~~~~~~~~~~
./include/trace/events/tlb.h:38:1: note: in expansion of macro ‘TRACE_EVENT’
    38 | TRACE_EVENT(tlb_flush,
       | ^~~~~~~~~~~

I suspect this is caused by the "else" at the end of the __scoped_guard() macro:

#define __scoped_guard(_name, _label, args...)                          \
         for (CLASS(_name, scope)(args);                                 \
              __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);       \
              ({ goto _label; }))                                        \
                 if (0) {                                                \
_label:                                                                 \
                         break;                                          \
                 } else

#define scoped_guard(_name, args...)    \
         __scoped_guard(_name, __UNIQUE_ID(label), args)

AFAIU this is a new warning introduced by

commit fcc22ac5baf ("cleanup: Adjust scoped_guard() macros to avoid potential warning")

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com