| 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: <CA+wEVJYeDACQTO8g2L9_ZAM0mC2JwBm_pDahiGVmfc=c43caAA@mail.gmail.com>
Date: Thu, 3 Jul 2025 18:18:36 +0200
From: Gabriele Paoloni <gpaoloni@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linux trace kernel <linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu <mhiramat@...nel.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH] tracing: Remove EVENT_FILE_FL_SOFT_MODE flag
On Wed, Jul 2, 2025 at 8:37 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> From: Steven Rostedt <rostedt@...dmis.org>
>
> When soft disabling of trace events was first created, it needed to have a
> way to know if a file had a user that was using it with soft disabled (for
> triggers that need to enable or disable events from a context that can not
> really enable or disable the event, it would set SOFT_DISABLED to state it
> is disabled). The flag SOFT_MODE was used to denote that an event had a
> user that would enable or disable it via the SOFT_DISABLED flag.
>
> Commit 1cf4c0732db3c ("tracing: Modify soft-mode only if there's no other
> referrer") fixed a bug where if two users were using the SOFT_DISABLED
> flag the accounting would get messed up as the SOFT_MODE flag could only
> handle one user. That commit added the sm_ref counter which kept track of
> how many users were using the event in "soft mode". This made the
> SOFT_MODE flag redundant as it should only be set if the sm_ref counter is
> non zero.
>
> Remove the SOFT_MODE flag and just use the sm_ref counter to know the
> event is in soft mode or not. This makes the code a bit simpler.
>
> Link: https://lore.kernel.org/all/20250702111908.03759998@batman.local.home/
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
> include/linux/trace_events.h | 3 ---
> kernel/trace/trace_events.c | 24 ++++++++++++------------
> 2 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index fa9cf4292dff..04307a19cde3 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -480,7 +480,6 @@ enum {
> EVENT_FILE_FL_RECORDED_TGID_BIT,
> EVENT_FILE_FL_FILTERED_BIT,
> EVENT_FILE_FL_NO_SET_FILTER_BIT,
> - EVENT_FILE_FL_SOFT_MODE_BIT,
> EVENT_FILE_FL_SOFT_DISABLED_BIT,
> EVENT_FILE_FL_TRIGGER_MODE_BIT,
> EVENT_FILE_FL_TRIGGER_COND_BIT,
> @@ -618,7 +617,6 @@ extern int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...);
> * RECORDED_TGID - The tgids should be recorded at sched_switch
> * FILTERED - The event has a filter attached
> * NO_SET_FILTER - Set when filter has error and is to be ignored
> - * SOFT_MODE - The event is enabled/disabled by SOFT_DISABLED
> * SOFT_DISABLED - When set, do not trace the event (even though its
> * tracepoint may be enabled)
> * TRIGGER_MODE - When set, invoke the triggers associated with the event
> @@ -633,7 +631,6 @@ enum {
> EVENT_FILE_FL_RECORDED_TGID = (1 << EVENT_FILE_FL_RECORDED_TGID_BIT),
> EVENT_FILE_FL_FILTERED = (1 << EVENT_FILE_FL_FILTERED_BIT),
> EVENT_FILE_FL_NO_SET_FILTER = (1 << EVENT_FILE_FL_NO_SET_FILTER_BIT),
> - EVENT_FILE_FL_SOFT_MODE = (1 << EVENT_FILE_FL_SOFT_MODE_BIT),
> EVENT_FILE_FL_SOFT_DISABLED = (1 << EVENT_FILE_FL_SOFT_DISABLED_BIT),
> EVENT_FILE_FL_TRIGGER_MODE = (1 << EVENT_FILE_FL_TRIGGER_MODE_BIT),
> EVENT_FILE_FL_TRIGGER_COND = (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 120531268abf..0980f4def360 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -768,6 +768,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> {
> struct trace_event_call *call = file->event_call;
> struct trace_array *tr = file->tr;
> + bool soft_mode = atomic_read(&file->sm_ref) != 0;
I was checking why an atomic counter is needed here, and, since this
function should be called with
event_mutex locked, I think the atomic counter is not needed, so maybe
it should be removed...
On a different angle I also checked the callers of this function and
it seems that in some cases
event_mutex is not held when calling this function. E.g.:
trace_event_trigger_enable_disable() -> trace_event_enable_disable()
-> __ftrace_event_enable_disable()
In general it seems that all the callers of
trace_event_enable_disable() are outside trace_events.c, so maybe
the modification below could solve this issue and concurrently we
could replace the atomic sm_ref with a
standard one...?
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -880,7 +880,9 @@ static int __ftrace_event_enable_disable(struct
trace_event_file *file,
int trace_event_enable_disable(struct trace_event_file *file,
int enable, int soft_disable)
{
+ mutex_lock(&event_mutex);
return __ftrace_event_enable_disable(file, enable, soft_disable);
+ mutex_unlock(&event_mutex);
}
Thanks
Gab
> int ret = 0;
> int disable;
>
> @@ -782,7 +783,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> * is set we do not want the event to be enabled before we
> * clear the bit.
> *
> - * When soft_disable is not set but the SOFT_MODE flag is,
> + * When soft_disable is not set but the soft_mode is,
> * we do nothing. Do not disable the tracepoint, otherwise
> * "soft enable"s (clearing the SOFT_DISABLED bit) wont work.
> */
> @@ -790,11 +791,11 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> if (atomic_dec_return(&file->sm_ref) > 0)
> break;
> disable = file->flags & EVENT_FILE_FL_SOFT_DISABLED;
> - clear_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags);
> + soft_mode = false;
> /* Disable use of trace_buffered_event */
> trace_buffered_event_disable();
> } else
> - disable = !(file->flags & EVENT_FILE_FL_SOFT_MODE);
> + disable = !soft_mode;
>
> if (disable && (file->flags & EVENT_FILE_FL_ENABLED)) {
> clear_bit(EVENT_FILE_FL_ENABLED_BIT, &file->flags);
> @@ -812,8 +813,8 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>
> WARN_ON_ONCE(ret);
> }
> - /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT, else clear it */
> - if (file->flags & EVENT_FILE_FL_SOFT_MODE)
> + /* If in soft mode, just set the SOFT_DISABLE_BIT, else clear it */
> + if (soft_mode)
> set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
> else
> clear_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
> @@ -823,7 +824,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> * When soft_disable is set and enable is set, we want to
> * register the tracepoint for the event, but leave the event
> * as is. That means, if the event was already enabled, we do
> - * nothing (but set SOFT_MODE). If the event is disabled, we
> + * nothing (but set soft_mode). If the event is disabled, we
> * set SOFT_DISABLED before enabling the event tracepoint, so
> * it still seems to be disabled.
> */
> @@ -832,7 +833,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> else {
> if (atomic_inc_return(&file->sm_ref) > 1)
> break;
> - set_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags);
> + soft_mode = true;
> /* Enable use of trace_buffered_event */
> trace_buffered_event_enable();
> }
> @@ -840,7 +841,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> if (!(file->flags & EVENT_FILE_FL_ENABLED)) {
> bool cmd = false, tgid = false;
>
> - /* Keep the event disabled, when going to SOFT_MODE. */
> + /* Keep the event disabled, when going to soft mode. */
> if (soft_disable)
> set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
>
> @@ -1792,8 +1793,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> !(flags & EVENT_FILE_FL_SOFT_DISABLED))
> strcpy(buf, "1");
>
> - if (flags & EVENT_FILE_FL_SOFT_DISABLED ||
> - flags & EVENT_FILE_FL_SOFT_MODE)
> + if (atomic_read(&file->sm_ref) != 0)
> strcat(buf, "*");
>
> strcat(buf, "\n");
> @@ -3584,7 +3584,7 @@ static int probe_remove_event_call(struct trace_event_call *call)
> continue;
> /*
> * We can't rely on ftrace_event_enable_disable(enable => 0)
> - * we are going to do, EVENT_FILE_FL_SOFT_MODE can suppress
> + * we are going to do, soft mode can suppress
> * TRACE_REG_UNREGISTER.
> */
> if (file->flags & EVENT_FILE_FL_ENABLED)
> @@ -3997,7 +3997,7 @@ static int free_probe_data(void *data)
>
> edata->ref--;
> if (!edata->ref) {
> - /* Remove the SOFT_MODE flag */
> + /* Remove soft mode */
> __ftrace_event_enable_disable(edata->file, 0, 1);
> trace_event_put_ref(edata->file->event_call);
> kfree(edata);
> --
> 2.47.2
>
>
Powered by blists - more mailing lists