linux-kernel - Re: [GIT PULL] tracing: Updates for v6.13

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-=witPrLcu22dZ93VCyRQonS7+-dFYhQbna=KBa-TAhayMw@mail.gmail.com>
Date: Fri, 22 Nov 2024 13:54:49 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Andrii Nakryiko <andrii@...nel.org>, Colin Ian King <colin.i.king@...il.com>, 
	Jeff Xie <jeff.xie@...ux.dev>, Jinjie Ruan <ruanjinjie@...wei.com>, 
	Josh Poimboeuf <jpoimboe@...nel.org>, Justin Stitt <justinstitt@...gle.com>, 
	Levi Yun <yeoreum.yun@....com>, Li Chen <chenl311@...natelecom.cn>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Ryan Roberts <ryan.roberts@....com>, 
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>, Tatsuya S <tatsuya.s2862@...il.com>, 
	Uros Bizjak <ubizjak@...il.com>, Zheng Yejian <zhengyejian@...weicloud.com>, 
	guoweikang <guoweikang.kernel@...il.com>
Subject: Re: [GIT PULL] tracing: Updates for v6.13

On Wed, 20 Nov 2024 at 18:45, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> - Addition of faultable tracepoints

Christ.

I had a discussion about how horrible conditional locking is in the
original series of this, and I thought people got the message:

   https://lore.kernel.org/all/CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@mail.gmail.com/

In fact, some of these commits *do* seem like the message was received
and understood.

But some of the code clearly DID NOT understand this at all, and we
end up with that horror in commit 0e6caab8db8b ("tracing: Declare
system call tracepoints with TRACE_EVENT_SYSCALL") which adds a
"syscall" flag that is initially unused.

And then later in the series, we end up with the insane and horrible
conditional locking anyway, in the form

    if (syscall) \
        rcu_read_lock_trace(); \
    else \
        preempt_disable_notrace(); \

so now the actual locking rules are based on a conditional anyway.

And yes, it's a static conditional, but it's *STUPID*. And it makes
the code just harder to follow, for no good reason. The difference is
literally this magic

                        __DO_TRACE(name,                                \
                                TP_ARGS(args),                          \
                                TP_CONDITION(cond), 0);                 \

vs

                        __DO_TRACE(name,                                \
                                TP_ARGS(args),                          \
                                TP_CONDITION(cond), 1);                 \

in the two users, neither of which explain the strange 0-vs-1
difference that is actually very very important.

Why am I upset? "It's static, it's not a real conditional". Yes, true,
but it's static only as far as the *compiler* is concerned, from a
human angle this is a random flag that you have to follow back to its
use and check all callers.

And equally importantly: it's actively stupid and non-productive.

Look here: pretty much exactly *half* of the __DO_TRACE() macro is
literally that silly "if (syscall)" logic that I'm complaining about.

It would have been both cleaner and simpler to *NOT* make it a flag at
all, but simply have two separate macros of __DO_TRACE_SYSCALL() vs
__DO_TRACE() that didn't *need* that stupid flag.

Make it very clear that the syscall tracing path is *different*. Make
the differences *explicit*.

Don't add random hidden flags to "share" code, when sharing completely
different locking rules is *BAD*.

In other words: the flag is pointless and actively harmful. When I
said "get rid of the stupid TRACE_EVENT_FN_MAY_FAULT thing entirely"
in that original discussion, I did not mean "replace it with another
unnamed flag instead".

Stop doing conditional locking. It's broken.

And stop "sharing" code that shouldn't be shared, when the
straightforward non-conditional non-shared code is simpler and easier
to follow.

The code would literally be smaller if you didn't do this. And you
wouldn't need the silly comment to explain the '@...call=0' argument
situations, it would have been sufficient and clearer to just state
that the system call tracing is done under a different locking regime
that allows page faulting.

I have pulled this, but I expect this horror to be fixed.

And equally importantly, I expect people to actually understand that
conditional locking is not a good thing. If the locking rules are
different, USE A DIFFERENT FUNCTION OR MACRO. Not the same function
with a magic flag for locking.

Replacing the old broken TRACE_EVENT_FN_MAY_FAULT with a silent 0/1 was not ok.

            Linus