[GIT PULL] tracing: Updates for v6.13

[GIT PULL] tracing: Updates for v6.13

[Steven Rostedt]

Linus,

tracing updates for v6.13:

- Addition of faultable tracepoints

  There's a tracepoint attached to both a system call entry and exit. This
  location is known to allow page faults. The tracepoints are called under
  an rcu_read_lock() which does not allow faults that can sleep. This limits
  the ability of tracepoint handlers to page fault in user space system call
  parameters. Now these tracepoints have been made "faultable", allowing the
  callbacks to fault in user space parameters and record them.

  Note, only the infrastructure has been implemented. The consumers (perf,
  ftrace, BPF) now need to have their code modified to allow faults.

- Fix up of BPF code for the tracepoint faultable logic

- Update tracepoints to use the new static branch API

- Remove trace_*_rcuidle() variants and the SRCU protection they used

- Remove unused TRACE_EVENT_FL_FILTERED logic

- Replace strncpy() with strscpy() and memcpy()

- Use replace per_cpu_ptr(smp_processor_id()) with this_cpu_ptr()

- Fix perf events to not duplicate samples when tracing is enabled

- Replace atomic64_add_return(1, counter) with atomic64_inc_return(counter)

- Make stack trace buffer 4K instead of PAGE_SIZE

- Remove TRACE_FLAG_IRQS_NOSUPPORT flag as it was never used

- Get the true return address for function tracer when function graph tracer
  is also running.

  When function_graph trace is running along with function tracer,
  the parent function of the function tracer sometimes is
  "return_to_handler", which is the function graph trampoline to record
  the exit of the function. Use existing logic that calls into the
  fgraph infrastructure to find the real return address.

- Remove (un)regfunc pointers out of tracepoint structure

- Added last minute bug fix for setting pending modules in stack function
  filter.

  echo "write*:mod:ext3" > /sys/kernel/tracing/stack_trace_filter

  Would cause a kernel NULL dereference.

- Minor clean ups


Please pull the latest trace-v6.13 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace-v6.13

Tag SHA1: 8f623cff04bb42e08806c4d6ed5ccb6cd49c1365
Head SHA1: 45af52e7d3b8560f21d139b3759735eead8b1653


Andrii Nakryiko (3):
      bpf: put bpf_link's program when link is safe to be deallocated
      bpf: decouple BPF link/attach hook and BPF program sleepable semantics
      bpf: ensure RCU Tasks Trace GP for sleepable raw tracepoint BPF links

Colin Ian King (1):
      tracing: Remove redundant check on field->field in histograms

Jeff Xie (1):
      ftrace: Get the true parent ip for function tracer

Jinjie Ruan (1):
      tracing: Replace strncpy() with strscpy() when copying comm

Josh Poimboeuf (1):
      tracepoints: Use new static branch API

Justin Stitt (2):
      tracing/branch-profiler: Replace deprecated strncpy with strscpy
      tracing: Replace multiple deprecated strncpy with memcpy

Levi Yun (1):
      trace/trace_event_perf: remove duplicate samples on the first tracepoint event

Li Chen (1):
      ftrace: Use this_cpu_ptr() instead of per_cpu_ptr(smp_processor_id())

Mathieu Desnoyers (12):
      tracing: Declare system call tracepoints with TRACE_EVENT_SYSCALL
      tracing/ftrace: disable preemption in syscall probe
      tracing/perf: disable preemption in syscall probe
      tracing/bpf: disable preemption in syscall probe
      tracing: Allow system call tracepoints to handle page faults
      tracing/ftrace: Add might_fault check to syscall probes
      tracing/perf: Add might_fault check to syscall probes
      tracing/bpf: Add might_fault check to syscall probes
      tracing: Introduce tracepoint extended structure
      tracing: Introduce tracepoint_is_faultable()
      tracing: Fix syscall tracepoint use-after-free
      tracing: Add might_fault() check in __DECLARE_TRACE_SYSCALL

Ryan Roberts (1):
      tracing: Make percpu stack trace buffer invariant to PAGE_SIZE

Sebastian Andrzej Siewior (1):
      tracing: Remove TRACE_FLAG_IRQS_NOSUPPORT

Steven Rostedt (2):
      tracing: Remove definition of trace_*_rcuidle()
      tracepoint: Remove SRCU protection

Tatsuya S (1):
      tracing: Fix function name for trampoline

Uros Bizjak (1):
      tracing: Use atomic64_inc_return() in trace_clock_counter()

Zheng Yejian (1):
      tracing: Remove TRACE_EVENT_FL_FILTERED logic

guoweikang (1):
      ftrace: Fix regression with module command in stack_trace_filter

----
 Documentation/trace/ftrace.rst       |   3 -
 include/linux/bpf.h                  |  20 ++++-
 include/linux/trace_events.h         |  17 ----
 include/linux/tracepoint-defs.h      |  14 ++-
 include/linux/tracepoint.h           | 169 ++++++++++++++++++++---------------
 include/trace/bpf_probe.h            |  14 +++
 include/trace/define_trace.h         |   5 ++
 include/trace/events/preemptirq.h    |   8 --
 include/trace/events/syscalls.h      |   4 +-
 include/trace/perf.h                 |  44 ++++++++-
 include/trace/trace_events.h         |  62 ++++++++++++-
 init/Kconfig                         |   1 +
 kernel/bpf/syscall.c                 |  67 ++++++++++----
 kernel/trace/ftrace.c                |   3 +
 kernel/trace/trace.c                 |  81 ++++++++---------
 kernel/trace/trace.h                 |  11 ++-
 kernel/trace/trace_branch.c          |  10 +--
 kernel/trace/trace_clock.c           |   2 +-
 kernel/trace/trace_event_perf.c      |   6 ++
 kernel/trace/trace_events.c          |   2 -
 kernel/trace/trace_events_filter.c   |   8 +-
 kernel/trace/trace_events_hist.c     |  11 +--
 kernel/trace/trace_events_user.c     |   4 +-
 kernel/trace/trace_functions.c       |  36 ++++++--
 kernel/trace/trace_functions_graph.c |   8 +-
 kernel/trace/trace_hwlat.c           |   4 +-
 kernel/trace/trace_mmiotrace.c       |   8 +-
 kernel/trace/trace_osnoise.c         |  12 +--
 kernel/trace/trace_output.c          |   5 +-
 kernel/trace/trace_preemptirq.c      |  26 ++----
 kernel/trace/trace_sched_switch.c    |   2 +-
 kernel/trace/trace_sched_wakeup.c    |   8 +-
 kernel/trace/trace_syscalls.c        |  28 ++++++
 kernel/tracepoint.c                  |  75 +++-------------
 scripts/tags.sh                      |   2 -
 35 files changed, 452 insertions(+), 328 deletions(-)
---------------------------

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

[Linus Torvalds]

On Wed, 20 Nov 2024 at 18:45, Steven Rostedt <rostedt@goodmis.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 '@syscall=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

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

[Steven Rostedt]

On Fri, 22 Nov 2024 13:54:49 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

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

Thanks.

Mathieu,

Care to send a patch to address Linus's concerns.

-- Steve

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

[Steven Rostedt]

On Fri, 22 Nov 2024 13:54:49 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 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);                 \

Hmm, if we make a __DO_TRACE_SYSCALL(), I don't think it needs to even have
that condition parameter. The TP_CONDITION() is to allow tracepoints to
check a condition within the static_branch() portion of the tracepoint so
the condition does not get called when tracepoints are disabled (and just a
nop).  By default we have:

#define DECLARE_TRACE_SYSCALL(name, proto, args)			\
	__DECLARE_TRACE_SYSCALL(name, PARAMS(proto), PARAMS(args),	\
				cpu_online(raw_smp_processor_id()),	\
				PARAMS(void *__data, proto))


Where the tracepoint has to at least be called when the current CPU is
online. But is there every a case where a CPU is not online when a system
call is entered or exited?

And there isn't any use cases of calling the system call tracepoint with a
condition. Hence, I think the __DO_TRACE_SYSCALL() doesn't even need to
include the condition:

#define __DO_TRACE_SYSCALL(name, args)					\
	do {								\
		int __maybe_unused __idx = 0;				\

Oh, also the __idx isn't used since I removed the SRCU, so we can remove
that too. Maybe make that a separate patch?

		rcu_read_lock_trace();					\
									\
		__DO_TRACE_CALL(name, TP_ARGS(args));			\
									\
		rcu_read_unlock_trace();				\
	} while (0)

-- Steve

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

[Mathieu Desnoyers]

On 2024-11-22 17:00, Steven Rostedt wrote:
> On Fri, 22 Nov 2024 13:54:49 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> I have pulled this, but I expect this horror to be fixed.
> 
> Thanks.
> 
> Mathieu,
> 
> Care to send a patch to address Linus's concerns.

Sure, I will prepare a fix.

Linus's concerns are clearly expressed and I missed that they applied
to __DO_TRACE() as well the last time.

Sorry about that!

Thanks,

Mathieu


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

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

[Linus Torvalds]

On Fri, 22 Nov 2024 at 14:12, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Hmm, if we make a __DO_TRACE_SYSCALL(), I don't think it needs to even have
> that condition parameter.

That was my point. The whole conditional - and the parameter - seems
to be completely pointless as far as I can tell.

That said, I think you can actually simplify things even further: if
you move the TO_CONDITION() checking into the caller, you could move
the locking there too.

IOW, instead of this pattern:

                if (static_branch_unlikely(&__tracepoint_##name.key))   \
                        __DO_TRACE(name,                                \
                                TP_ARGS(args),                          \
                                TP_CONDITION(cond), 0);                 \

you could make it be something like this instead:

                if (static_branch_unlikely(&__tracepoint_##name.key)) \
                        if (TP_CONDITION(cond)) \
                                scoped_guard(preempt_notrace) \
                                        __DO_TRACE(name, TP_ARGS(args)); \

where __DO_TRACE() would get neither the "cond" argument _nor_ that
locking argument, because both are just done by the two users (the
other one would use "scoped_guard(rcu_read_trace)" of course.

And look, this is another reason why unconditional locking is a good
thing: now you can use the "guard()" model for the lock, and don't
need an explicit unlock, simplifying the code more.

Of course, you want "guard(rcu_read_trace)" (for system call events)
and "guard(preempt_notrace)" (for the regular trace event case), and
we don't have the "notrace" versions of those guard classes yet.

But adding those would literally be trivial, ie something like

  DEFINE_LOCK_GUARD_0(rcu_notrace,
        rcu_read_lock_notrace(), rcu_read_unlock_notrace())

wouldn't that make it all look really nice?

NOTE NOTE NOTE! I didn't actually try any of the above in real life,
so I might be missing some important detail. I'm just pointing out
that making this all unconditional and not based on random flags has
the potential for even more cleanups.

And I might have gotten the different lock names confused too.

              Linus

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

[pr-tracker-bot]

The pull request you sent on Wed, 20 Nov 2024 21:45:31 -0500:

> git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace-v6.13

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/06afb0f36106ecb839c5e2509905e68c1e2677de

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[Steven Rostedt]

On Fri, 22 Nov 2024 14:30:10 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, 22 Nov 2024 at 14:12, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Hmm, if we make a __DO_TRACE_SYSCALL(), I don't think it needs to even have
> > that condition parameter.  
> 
> That was my point. The whole conditional - and the parameter - seems
> to be completely pointless as far as I can tell.
> 
> That said, I think you can actually simplify things even further: if
> you move the TO_CONDITION() checking into the caller, you could move
> the locking there too.
> 
> IOW, instead of this pattern:
> 
>                 if (static_branch_unlikely(&__tracepoint_##name.key))   \
>                         __DO_TRACE(name,                                \
>                                 TP_ARGS(args),                          \
>                                 TP_CONDITION(cond), 0);                 \
> 
> you could make it be something like this instead:
> 
>                 if (static_branch_unlikely(&__tracepoint_##name.key)) \
>                         if (TP_CONDITION(cond)) \
>                                 scoped_guard(preempt_notrace) \
>                                         __DO_TRACE(name, TP_ARGS(args)); \

Hmm, I wonder why I didn't do that in the first place :-/

But doing a little git forensics, I added that with:

  287050d390264 ("tracing: Add TRACE_EVENT_CONDITIONAL()")

Which goes back to December of 2010!

I have no idea what I was thinking back then :-p

> 
> where __DO_TRACE() would get neither the "cond" argument _nor_ that
> locking argument, because both are just done by the two users (the
> other one would use "scoped_guard(rcu_read_trace)" of course.

IOW, remove __DO_TRACE() and just call __DO_TRACE_CALL() directly.

> 
> And look, this is another reason why unconditional locking is a good
> thing: now you can use the "guard()" model for the lock, and don't
> need an explicit unlock, simplifying the code more.
> 
> Of course, you want "guard(rcu_read_trace)" (for system call events)
> and "guard(preempt_notrace)" (for the regular trace event case), and
> we don't have the "notrace" versions of those guard classes yet.
> 
> But adding those would literally be trivial, ie something like
> 
>   DEFINE_LOCK_GUARD_0(rcu_notrace,
>         rcu_read_lock_notrace(), rcu_read_unlock_notrace())
> 
> wouldn't that make it all look really nice?
> 
> NOTE NOTE NOTE! I didn't actually try any of the above in real life,
> so I might be missing some important detail. I'm just pointing out
> that making this all unconditional and not based on random flags has
> the potential for even more cleanups.
> 
> And I might have gotten the different lock names confused too.

Thanks for the analysis.

-- Steve