| 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
| ||
|
Message-ID: <522b01cf-0cb6-4766-9102-2d08a3983d8a@paulmck-laptop>
Date: Thu, 6 Nov 2025 09:01:30 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: rcu@...r.kernel.org, linux-kernel@...r.kernel.org, kernel-team@...a.com,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
bpf@...r.kernel.org
Subject: Re: [PATCH v2 10/16] tracing: Guard __DECLARE_TRACE() use of
__DO_TRACE_CALL() with SRCU-fast
On Thu, Nov 06, 2025 at 11:02:30AM -0500, Steven Rostedt wrote:
> On Wed, 5 Nov 2025 12:32:10 -0800
> "Paul E. McKenney" <paulmck@...nel.org> wrote:
> >
> > The current commit can be thought of as an approximate revert of that
> > commit, with some compensating additions of preemption disabling pointed
> > out by Steven Rostedt (thank you, Steven!). This preemption disabling
>
> > uses guard(preempt_notrace)(), and while in the area a couple of other
> > use cases were also converted to guards.
>
> Actually, please don't do any conversions. That code is unrelated to
> this work and I may be touching it. I don't need unneeded conflicts.
OK, thank you for letting me know. Should I set up for the merge window
after this coming one (of course applying your feedback below), or will
you be making this safe for PREEMPT_RT as part of your work?
If I don't hear otherwise, I will assume the former, though I would be
quite happy with the latter. ;-).
Thanx, Paul
> > ---
> > include/linux/tracepoint.h | 45 ++++++++++++++++++++++--------------
> > include/trace/perf.h | 4 ++--
> > include/trace/trace_events.h | 4 ++--
> > kernel/tracepoint.c | 21 ++++++++++++++++-
> > 4 files changed, 52 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 826ce3f8e1f8..9f8b19cd303a 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -33,6 +33,8 @@ struct trace_eval_map {
> >
> > #define TRACEPOINT_DEFAULT_PRIO 10
> >
> > +extern struct srcu_struct tracepoint_srcu;
> > +
> > extern int
> > tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data);
> > extern int
> > @@ -115,7 +117,10 @@ void for_each_tracepoint_in_module(struct module *mod,
> > static inline void tracepoint_synchronize_unregister(void)
> > {
> > synchronize_rcu_tasks_trace();
> > - synchronize_rcu();
> > + if (IS_ENABLED(CONFIG_PREEMPT_RT))
> > + synchronize_srcu(&tracepoint_srcu);
> > + else
> > + synchronize_rcu();
> > }
>
> Instead of using the IS_ENABLED(CONFIG_PREEMPT_RT) I think it would be
> somewhat cleaner to add macros (all of this is untested):
>
> #ifdef CONFIG_PREEMPT_RT
> extern struct srcu_struct tracepoint_srcu;
> # define tracepoint_sync() synchronizes_srcu(&tracepoint_srcu)
> # define tracepoint_guard() \
> guard(srcu_fast_notrace)(&tracepoint_srcu); \
> guard(migrate)()
> #else
> # define tracepoint_sync() synchronize_rcu();
> # define tracepoint_guard() guard(preempt_notrace)
> #endif
>
> And then the above can be:
>
> static inline void tracepoint_synchronize_unregister(void)
> {
> synchronize_rcu_tasks_trace();
> tracepoint_sync();
> }
>
> and the below:
>
> static inline void __do_trace_##name(proto) \
> { \
> if (cond) { \
> tracepoint_guard(); \
> __DO_TRACE_CALL(name, TP_ARGS(args)); \
> } \
> } \
>
> And not have to duplicate all that code.
>
> > static inline bool tracepoint_is_faultable(struct tracepoint *tp)
> > {
> > @@ -266,23 +271,29 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > return static_branch_unlikely(&__tracepoint_##name.key);\
> > }
> >
> > -#define __DECLARE_TRACE(name, proto, args, cond, data_proto) \
> > +#define __DECLARE_TRACE(name, proto, args, cond, data_proto) \
> > __DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto)) \
> > - static inline void __do_trace_##name(proto) \
> > - { \
> > - if (cond) { \
> > - guard(preempt_notrace)(); \
> > - __DO_TRACE_CALL(name, TP_ARGS(args)); \
> > - } \
> > - } \
> > - static inline void trace_##name(proto) \
> > - { \
> > - if (static_branch_unlikely(&__tracepoint_##name.key)) \
> > - __do_trace_##name(args); \
> > - if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \
> > - WARN_ONCE(!rcu_is_watching(), \
> > - "RCU not watching for tracepoint"); \
> > - } \
> > + static inline void __do_trace_##name(proto) \
> > + { \
> > + if (cond) { \
> > + if (IS_ENABLED(CONFIG_PREEMPT_RT) && preemptible()) { \
> > + guard(srcu_fast_notrace)(&tracepoint_srcu); \
> > + guard(migrate)(); \
> > + __DO_TRACE_CALL(name, TP_ARGS(args)); \
> > + } else { \
> > + guard(preempt_notrace)(); \
> > + __DO_TRACE_CALL(name, TP_ARGS(args)); \
> > + } \
> > + } \
> > + } \
> > + static inline void trace_##name(proto) \
> > + { \
> > + if (static_branch_unlikely(&__tracepoint_##name.key)) \
> > + __do_trace_##name(args); \
> > + if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \
> > + WARN_ONCE(!rcu_is_watching(), \
> > + "RCU not watching for tracepoint"); \
> > + } \
> >
> >
>
> > /*
> > diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
> > index 4f22136fd465..fbc07d353be6 100644
> > --- a/include/trace/trace_events.h
> > +++ b/include/trace/trace_events.h
> > @@ -436,6 +436,7 @@ __DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), PARAMS(tstruct), \
> > static notrace void \
> > trace_event_raw_event_##call(void *__data, proto) \
> > { \
> > + guard(preempt_notrace)(); \
>
> Note, the tracepoint code expects that there's only one level of
> preemption done, as it records the preempt_count and needs to subtract
> what tracing added. Just calling preempt_notrace here if it had already
> disabled preemption will break that code.
>
> It should only disable preemption if it hasn't already done that (when
> PREEMPT_RT is enabled).
>
> > do_trace_event_raw_event_##call(__data, args); \
> > }
> >
> > @@ -447,9 +448,8 @@ static notrace void \
> > trace_event_raw_event_##call(void *__data, proto) \
> > { \
> > might_fault(); \
> > - preempt_disable_notrace(); \
> > + guard(preempt_notrace)(); \
> > do_trace_event_raw_event_##call(__data, args); \
> > - preempt_enable_notrace(); \
>
> I may be modifying the above, so I would leave it alone.
>
> Thanks,
>
> -- Steve
>
Powered by blists - more mailing lists