linux-kernel - Re: [RFC PATCH v3 2/3] tracing: Introduce tracepoint_is

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: <9e9c37d6-3fa8-4f58-9d27-a629b4a817f5@efficios.com>
Date: Mon, 28 Oct 2024 09:35:31 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, 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>,
 Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [RFC PATCH v3 2/3] tracing: Introduce tracepoint_is_syscall()

On 2024-10-28 01:06, Steven Rostedt wrote:
> On Sun, 27 Oct 2024 08:30:54 -0400
> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
>>>
>>> I wonder if we should call it "sleepable" instead? For this patch set
>>> do we really care if it's a system call or not? It's really if the
>>> tracepoint is sleepable or not that's the issue. System calls are just
>>> one user of it, there may be more in the future, and the changes to BPF
>>> will still be needed.
>>
>> Remember that syscall tracepoint probes are allowed to handle page
>> faults, but should not generally block, otherwise it would postpone the
>> grace periods of all RCU tasks trace users.
>>
>> So naming this "sleepable" would be misleading, because probes are
>> not allowed general blocking, just to handle page faults.
> 
> I'm fine with "faultable" too.
> 
>>
>> If we look at the history of this tracepoint feature, we went with
>> the following naming over the various versions of the patch series:
>>
>> 1) Sleepable tracepoints: until we understood that we just want to
>>      allow page fault, not general sleeping, so we needed to change
>>      the name,
>>
>> 2) Faultable tracepoints: until Linus requested that we aim for
>>      something that is specific to system calls, rather than a generic
>>      thing.
>>
>>      https://lore.kernel.org/lkml/CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@mail.gmail.com/
> 
> Reading that thread again, I believe that Linus was talking more about
> all the infrastructure going around to make a special "faultable"
> tracepoint (I could be wrong, and Linus may correct me here). When in
> fact, the only user is system calls. But from the BPF POV, it doesn't
> care if it's a system call, it cares that it is faultable, and the
> check should be on that. Having BPF check if it's a system call is
> requiring that BPF knows the implementation details of system call
> tracepoints. But if it knows it is faultable, then it needs to do
> something special.

It might just be that, indeed. Considering the overwhelming preference
for something a little more general (sleepable/faultable vs syscall),
I am tempted to go for "tracepoint_is_faultable()".

> 
>>
>> 3) Syscall tracepoints: This is what we currently have.
>>
>>> Other than that, I think this could work.
>>
>> Calling this field "sleepable" would be misleading. Calling it "faultable"
>> would be a better fit, but based on Linus' request, I'm tempted to stick
>> with "syscall" for now.
>>
>> Your concern is to name this in a way that is general and future-proof.
>> Linus' point was to make it syscall-specific rather than general. My
>> position is that we should wait until we face other use-cases (if we
>> even do) before consider changing the naming from "syscall" to something
>> more generic.
> 
> Yes, but that was for the infrastructure itself. It really doesnt' make
> sense that BPF needs to know which type of tracepoint can fault. That's
> telling BPF, you need to know the implementation of this type of
> tracepoint.

OK, I'll use tracepoint_is_faultable() and a "faultable" field name, and
see how people react. I really prefer "faultable" to "sleepable" here,
because I envision that in the future we may introduce tracepoints
which are really able to sleep (general blocking), for instance though
use of hazard pointers to protect a list iteration. (if there is ever
a need for it)

Thanks,

Mathieu

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