| 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: <20251106110230.08e877ff@batman.local.home>
Date: Thu, 6 Nov 2025 11:02:30 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: "Paul E. McKenney" <paulmck@...nel.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 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.
> ---
> 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