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: <20241126084556.GI38837@noisy.programming.kicks-ass.net>
Date: Tue, 26 Nov 2024 09:45:56 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	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>,
	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,
	Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [RFC PATCH 4/5] tracing: Remove conditional locking from
 __DO_TRACE()

On Mon, Nov 25, 2024 at 09:51:43AM -0800, Linus Torvalds wrote:

> That said, I have a "lovely" suggestion. Instead of the "if(0)+goto"
> games, I think you can just do this:
> 
>   #define scoped_guard(_name, args...)                                   \
>          for (CLASS(_name, scope)(args), *_once = (void *)1; _once &&    \
>               (__guard_ptr(_name)(&scope) || !__is_cond_ptr(_name));     \
>               _once = NULL)
> 

Right, so that's more or less what we used to have:

#define scoped_guard(_name, args...)                                    \
        for (CLASS(_name, scope)(args), *done = NULL;			\
	     __guard_ptr(_name)(&scope) && !done; done = (void *)1)

But it turns out the compilers have a hard time dealing with this. From
commit fcc22ac5baf0 ("cleanup: Adjust scoped_guard() macros to avoid
potential warning"):	

    int foo(struct my_drv *adapter)
    {
            scoped_guard(spinlock, &adapter->some_spinlock)
                    return adapter->spinlock_protected_var;
    }

Using that (old) form results in:

    error: control reaches end of non-void function [-Werror=return-type]


Now obviously the above can also be written like:

    int foo(struct my_drv *adapter)
    {
            guard(spinlock)(&adapter->some_spinlock);
	    return adapter->spinlock_protected_var;
    }

But the point is to show the compilers get confused. Additionally Dan
Carpenter noted that smatch has a much easier time dealing with the new
form.

And the commit notes the generated code is actually slighly smaller with
e new form too (probably because the compiler is less confused about
control flow).

Except of course, now we get that dangling-else warning, there is no
winning this :-/

So I merged that patch because of the compilers getting less confused
and better code-gen, but if you prefer the old one we can definitely go
back. In which case we should go talk to compiler folks to figure out
how to make it not so confused.