| 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+wEVJbwPPJ6_DOXChTCQygQrraK785WCu97jdVpdUqJ483mdA@mail.gmail.com>
Date: Thu, 3 Jul 2025 18:39:18 +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 Thu, Jul 3, 2025 at 6:18 PM Gabriele Paoloni <gpaoloni@...hat.com> wrote:
>
> 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...?
Sorry I had a better look at the code and event_mutex is locked/unlocked
also outside of trace_events.c so the modification below is not needed.
However I think we could still replace the atomic counter.
Gab
>
> --- 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