* [PATCH] trace: propagate registration failure from tracing_start_*_record()
@ 2026-04-17 6:38 Yash Suthar
2026-04-17 15:52 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Yash Suthar @ 2026-04-17 6:38 UTC (permalink / raw)
To: rostedt, mhiramat
Cc: mathieu.desnoyers, linux-kernel, linux-trace-kernel, skhan, me,
Yash Suthar, syzbot+a1d25e53cd4a10f7f2d3
syzbot reported a WARN in tracepoint_probe_unregister():
tracing_start_sched_switch() increments sched_cmdline_ref /
sched_tgid_ref before calling tracing_sched_register(), and its
return value is discarded because the API is void. When the first
register_trace_sched_*() fails (e.g. kmalloc under memory pressure
or failslab), the function's fail_deprobe* labels roll back any
partial probe registration, but the caller's refcount has already
been bumped. The state is now desynced: refs > 0 but no probes in
tp->funcs.
Later, when the caller pairs the start with a stop, the refcount
walks back to 0 and tracing_sched_unregister() calls
unregister_trace_sched_*() against an empty tp->funcs.
func_remove() returns -ENOENT and the
WARN_ON_ONCE(IS_ERR(old)) in tracepoint_remove_func() fires.
Fix: make tracing_start_sched_switch() and the two exported
wrappers, tracing_start_cmdline_record() and
tracing_start_tgid_record(), return int; register the probes
before bumping the refcount; and propagate the error to callers
so refs are only held on behalf of a caller whose registration
actually succeeded.
Fixes: d914ba37d714 ("tracing: Add support for recording tgid of tasks")
Reported-by: syzbot+a1d25e53cd4a10f7f2d3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?id=f93e97cd824071a2577a40cde9ecd957f59f87eb
Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
---
kernel/trace/trace.c | 6 +++---
kernel/trace/trace.h | 4 ++--
kernel/trace/trace_events.c | 28 +++++++++++++++++++--------
kernel/trace/trace_functions.c | 8 +++++++-
kernel/trace/trace_functions_graph.c | 6 +++++-
kernel/trace/trace_sched_switch.c | 29 ++++++++++++++++++----------
kernel/trace/trace_selftest.c | 7 ++++++-
7 files changed, 62 insertions(+), 26 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8bd4ec08fb36..e936eed99b27 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3320,7 +3320,7 @@ void trace_printk_init_buffers(void)
* allocated here, then this was called by module code.
*/
if (global_trace.array_buffer.buffer)
- tracing_start_cmdline_record();
+ (void)tracing_start_cmdline_record();
}
EXPORT_SYMBOL_GPL(trace_printk_init_buffers);
@@ -3329,7 +3329,7 @@ void trace_printk_start_comm(void)
/* Start tracing comms if trace printk is set */
if (!buffers_allocated)
return;
- tracing_start_cmdline_record();
+ (void)tracing_start_cmdline_record();
}
static void trace_printk_start_stop_comm(int enabled)
@@ -3338,7 +3338,7 @@ static void trace_printk_start_stop_comm(int enabled)
return;
if (enabled)
- tracing_start_cmdline_record();
+ (void)tracing_start_cmdline_record();
else
tracing_stop_cmdline_record();
}
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index b6d42fe06115..6fe2c8429560 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -751,9 +751,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace, struct fgraph_ops *gops,
int trace_graph_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
struct ftrace_regs *fregs);
-void tracing_start_cmdline_record(void);
+int tracing_start_cmdline_record(void);
void tracing_stop_cmdline_record(void);
-void tracing_start_tgid_record(void);
+int tracing_start_tgid_record(void);
void tracing_stop_tgid_record(void);
int register_tracer(struct tracer *type);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 137b4d9bb116..e6713aa80a03 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -734,9 +734,9 @@ void trace_event_enable_cmd_record(bool enable)
continue;
if (enable) {
- tracing_start_cmdline_record();
- set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags);
- } else {
+ if (!tracing_start_cmdline_record())
+ set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags);
+ } else if (file->flags & EVENT_FILE_FL_RECORDED_CMD) {
tracing_stop_cmdline_record();
clear_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags);
}
@@ -755,9 +755,9 @@ void trace_event_enable_tgid_record(bool enable)
continue;
if (enable) {
- tracing_start_tgid_record();
- set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags);
- } else {
+ if (!tracing_start_tgid_record())
+ set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags);
+ } else if (file->flags & EVENT_FILE_FL_RECORDED_TGID) {
tracing_stop_tgid_record();
clear_bit(EVENT_FILE_FL_RECORDED_TGID_BIT,
&file->flags);
@@ -847,14 +847,26 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
if (tr->trace_flags & TRACE_ITER(RECORD_CMD)) {
+ ret = tracing_start_cmdline_record();
+ if (ret) {
+ pr_info("event trace: Could not enable event %s\n",
+ trace_event_name(call));
+ break;
+ }
cmd = true;
- tracing_start_cmdline_record();
set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags);
}
if (tr->trace_flags & TRACE_ITER(RECORD_TGID)) {
+ ret = tracing_start_tgid_record();
+ if (ret) {
+ if (cmd)
+ tracing_stop_cmdline_record();
+ pr_info("event trace: Could not enable event %s\n",
+ trace_event_name(call));
+ break;
+ }
tgid = true;
- tracing_start_tgid_record();
set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags);
}
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index c12795c2fb39..14d099734345 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -146,6 +146,8 @@ static bool handle_func_repeats(struct trace_array *tr, u32 flags_val)
static int function_trace_init(struct trace_array *tr)
{
ftrace_func_t func;
+ int ret;
+
/*
* Instance trace_arrays get their ops allocated
* at instance creation. Unless it failed
@@ -165,7 +167,11 @@ static int function_trace_init(struct trace_array *tr)
tr->array_buffer.cpu = raw_smp_processor_id();
- tracing_start_cmdline_record();
+ ret = tracing_start_cmdline_record();
+ if (ret) {
+ ftrace_reset_array_ops(tr);
+ return ret;
+ }
tracing_start_function_trace(tr);
return 0;
}
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 1de6f1573621..6b27ed62fee8 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -487,7 +487,11 @@ static int graph_trace_init(struct trace_array *tr)
ret = register_ftrace_graph(tr->gops);
if (ret)
return ret;
- tracing_start_cmdline_record();
+ ret = tracing_start_cmdline_record();
+ if (ret) {
+ unregister_ftrace_graph(tr->gops);
+ return ret;
+ }
return 0;
}
diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
index c46d584ded3b..683ea4ca1498 100644
--- a/kernel/trace/trace_sched_switch.c
+++ b/kernel/trace/trace_sched_switch.c
@@ -89,12 +89,22 @@ static void tracing_sched_unregister(void)
unregister_trace_sched_wakeup(probe_sched_wakeup, NULL);
}
-static void tracing_start_sched_switch(int ops)
+static int tracing_start_sched_switch(int ops)
{
- bool sched_register;
+ int ret = 0;
mutex_lock(&sched_register_mutex);
- sched_register = (!sched_cmdline_ref && !sched_tgid_ref);
+
+ /*
+ * If the registration fails, do not bump the reference count : the
+ * caller must observe the failure so it can avoid a later matching
+ * stop that would otherwise unregister probes that were never added.
+ */
+ if (!sched_cmdline_ref && !sched_tgid_ref) {
+ ret = tracing_sched_register();
+ if (ret)
+ goto out;
+ }
switch (ops) {
case RECORD_CMDLINE:
@@ -105,10 +115,9 @@ static void tracing_start_sched_switch(int ops)
sched_tgid_ref++;
break;
}
-
- if (sched_register && (sched_cmdline_ref || sched_tgid_ref))
- tracing_sched_register();
+out:
mutex_unlock(&sched_register_mutex);
+ return ret;
}
static void tracing_stop_sched_switch(int ops)
@@ -130,9 +139,9 @@ static void tracing_stop_sched_switch(int ops)
mutex_unlock(&sched_register_mutex);
}
-void tracing_start_cmdline_record(void)
+int tracing_start_cmdline_record(void)
{
- tracing_start_sched_switch(RECORD_CMDLINE);
+ return tracing_start_sched_switch(RECORD_CMDLINE);
}
void tracing_stop_cmdline_record(void)
@@ -140,9 +149,9 @@ void tracing_stop_cmdline_record(void)
tracing_stop_sched_switch(RECORD_CMDLINE);
}
-void tracing_start_tgid_record(void)
+int tracing_start_tgid_record(void)
{
- tracing_start_sched_switch(RECORD_TGID);
+ return tracing_start_sched_switch(RECORD_TGID);
}
void tracing_stop_tgid_record(void)
diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index d88c44f1dfa5..238e7451f8e4 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -1084,7 +1084,12 @@ trace_selftest_startup_function_graph(struct tracer *trace,
warn_failed_init_tracer(trace, ret);
goto out;
}
- tracing_start_cmdline_record();
+ ret = tracing_start_cmdline_record();
+ if (ret) {
+ unregister_ftrace_graph(&fgraph_ops);
+ warn_failed_init_tracer(trace, ret);
+ goto out;
+ }
/* Sleep for a 1/10 of a second */
msleep(100);
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] trace: propagate registration failure from tracing_start_*_record() 2026-04-17 6:38 [PATCH] trace: propagate registration failure from tracing_start_*_record() Yash Suthar @ 2026-04-17 15:52 ` Steven Rostedt 2026-04-18 5:38 ` Yash Suthar 0 siblings, 1 reply; 3+ messages in thread From: Steven Rostedt @ 2026-04-17 15:52 UTC (permalink / raw) To: Yash Suthar Cc: mhiramat, mathieu.desnoyers, linux-kernel, linux-trace-kernel, skhan, me, syzbot+a1d25e53cd4a10f7f2d3 On Fri, 17 Apr 2026 12:08:27 +0530 Yash Suthar <yashsuthar983@gmail.com> wrote: > syzbot reported a WARN in tracepoint_probe_unregister(): > > tracing_start_sched_switch() increments sched_cmdline_ref / > sched_tgid_ref before calling tracing_sched_register(), and its > return value is discarded because the API is void. When the first > register_trace_sched_*() fails (e.g. kmalloc under memory pressure > or failslab), the function's fail_deprobe* labels roll back any > partial probe registration, but the caller's refcount has already > been bumped. The state is now desynced: refs > 0 but no probes in > tp->funcs. > > Later, when the caller pairs the start with a stop, the refcount > walks back to 0 and tracing_sched_unregister() calls > unregister_trace_sched_*() against an empty tp->funcs. > func_remove() returns -ENOENT and the > WARN_ON_ONCE(IS_ERR(old)) in tracepoint_remove_func() fires. > > Fix: make tracing_start_sched_switch() and the two exported > wrappers, tracing_start_cmdline_record() and > tracing_start_tgid_record(), return int; register the probes > before bumping the refcount; and propagate the error to callers > so refs are only held on behalf of a caller whose registration > actually succeeded. > > Fixes: d914ba37d714 ("tracing: Add support for recording tgid of tasks") > Reported-by: syzbot+a1d25e53cd4a10f7f2d3@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?id=f93e97cd824071a2577a40cde9ecd957f59f87eb Did you use AI to create any of this? If so you must disclose it. This reads very much like an AI patch. > > Signed-off-by: Yash Suthar <yashsuthar983@gmail.com> > --- > kernel/trace/trace.c | 6 +++--- > kernel/trace/trace.h | 4 ++-- > kernel/trace/trace_events.c | 28 +++++++++++++++++++-------- > kernel/trace/trace_functions.c | 8 +++++++- > kernel/trace/trace_functions_graph.c | 6 +++++- > kernel/trace/trace_sched_switch.c | 29 ++++++++++++++++++---------- > kernel/trace/trace_selftest.c | 7 ++++++- > 7 files changed, 62 insertions(+), 26 deletions(-) NAK on all this. If you are under severe memory constraints that causes this to fail, then you'll be hitting a bunch more errors. > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 8bd4ec08fb36..e936eed99b27 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -3320,7 +3320,7 @@ void trace_printk_init_buffers(void) > * allocated here, then this was called by module code. > */ > if (global_trace.array_buffer.buffer) > - tracing_start_cmdline_record(); > + (void)tracing_start_cmdline_record(); WTF??? Why are you adding the typecast of (void) here? Don't do that! > } > EXPORT_SYMBOL_GPL(trace_printk_init_buffers); > > @@ -3329,7 +3329,7 @@ void trace_printk_start_comm(void) > /* Start tracing comms if trace printk is set */ > if (!buffers_allocated) > return; > - tracing_start_cmdline_record(); > + (void)tracing_start_cmdline_record(); > } > > static void trace_printk_start_stop_comm(int enabled) > @@ -3338,7 +3338,7 @@ static void trace_printk_start_stop_comm(int enabled) > return; > > if (enabled) > - tracing_start_cmdline_record(); > + (void)tracing_start_cmdline_record(); > else > tracing_stop_cmdline_record(); > } > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index b6d42fe06115..6fe2c8429560 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -751,9 +751,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace, struct fgraph_ops *gops, > int trace_graph_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops, > struct ftrace_regs *fregs); > > -void tracing_start_cmdline_record(void); > +int tracing_start_cmdline_record(void); > void tracing_stop_cmdline_record(void); > -void tracing_start_tgid_record(void); > +int tracing_start_tgid_record(void); > void tracing_stop_tgid_record(void); > > int register_tracer(struct tracer *type); > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > index 137b4d9bb116..e6713aa80a03 100644 > --- a/kernel/trace/trace_events.c > +++ b/kernel/trace/trace_events.c > @@ -734,9 +734,9 @@ void trace_event_enable_cmd_record(bool enable) > continue; > > if (enable) { > - tracing_start_cmdline_record(); > - set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > - } else { > + if (!tracing_start_cmdline_record()) > + set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > + } else if (file->flags & EVENT_FILE_FL_RECORDED_CMD) { > tracing_stop_cmdline_record(); > clear_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > } > @@ -755,9 +755,9 @@ void trace_event_enable_tgid_record(bool enable) > continue; > > if (enable) { > - tracing_start_tgid_record(); > - set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags); > - } else { > + if (!tracing_start_tgid_record()) > + set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags); > + } else if (file->flags & EVENT_FILE_FL_RECORDED_TGID) { > tracing_stop_tgid_record(); > clear_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, > &file->flags); > @@ -847,14 +847,26 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file, > set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); > > if (tr->trace_flags & TRACE_ITER(RECORD_CMD)) { > + ret = tracing_start_cmdline_record(); > + if (ret) { > + pr_info("event trace: Could not enable event %s\n", > + trace_event_name(call)); > + break; > + } > cmd = true; > - tracing_start_cmdline_record(); > set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > } > > if (tr->trace_flags & TRACE_ITER(RECORD_TGID)) { > + ret = tracing_start_tgid_record(); > + if (ret) { > + if (cmd) > + tracing_stop_cmdline_record(); > + pr_info("event trace: Could not enable event %s\n", > + trace_event_name(call)); > + break; > + } > tgid = true; > - tracing_start_tgid_record(); > set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags); > } > > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c > index c12795c2fb39..14d099734345 100644 > --- a/kernel/trace/trace_functions.c > +++ b/kernel/trace/trace_functions.c > @@ -146,6 +146,8 @@ static bool handle_func_repeats(struct trace_array *tr, u32 flags_val) > static int function_trace_init(struct trace_array *tr) > { > ftrace_func_t func; > + int ret; > + > /* > * Instance trace_arrays get their ops allocated > * at instance creation. Unless it failed > @@ -165,7 +167,11 @@ static int function_trace_init(struct trace_array *tr) > > tr->array_buffer.cpu = raw_smp_processor_id(); > > - tracing_start_cmdline_record(); > + ret = tracing_start_cmdline_record(); > + if (ret) { > + ftrace_reset_array_ops(tr); > + return ret; > + } > tracing_start_function_trace(tr); > return 0; > } > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c > index 1de6f1573621..6b27ed62fee8 100644 > --- a/kernel/trace/trace_functions_graph.c > +++ b/kernel/trace/trace_functions_graph.c > @@ -487,7 +487,11 @@ static int graph_trace_init(struct trace_array *tr) > ret = register_ftrace_graph(tr->gops); > if (ret) > return ret; > - tracing_start_cmdline_record(); > + ret = tracing_start_cmdline_record(); > + if (ret) { > + unregister_ftrace_graph(tr->gops); > + return ret; > + } > > return 0; > } > diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c > index c46d584ded3b..683ea4ca1498 100644 > --- a/kernel/trace/trace_sched_switch.c > +++ b/kernel/trace/trace_sched_switch.c > @@ -89,12 +89,22 @@ static void tracing_sched_unregister(void) > unregister_trace_sched_wakeup(probe_sched_wakeup, NULL); > } > > -static void tracing_start_sched_switch(int ops) > +static int tracing_start_sched_switch(int ops) > { > - bool sched_register; > + int ret = 0; > > mutex_lock(&sched_register_mutex); > - sched_register = (!sched_cmdline_ref && !sched_tgid_ref); > + > + /* > + * If the registration fails, do not bump the reference count : the > + * caller must observe the failure so it can avoid a later matching > + * stop that would otherwise unregister probes that were never added. > + */ > + if (!sched_cmdline_ref && !sched_tgid_ref) { > + ret = tracing_sched_register(); > + if (ret) > + goto out; > + } > > switch (ops) { > case RECORD_CMDLINE: > @@ -105,10 +115,9 @@ static void tracing_start_sched_switch(int ops) > sched_tgid_ref++; > break; > } > - > - if (sched_register && (sched_cmdline_ref || sched_tgid_ref)) > - tracing_sched_register(); The only change that should deal with this would be: if (sched_register && (sched_cmdline_ref || sched_tgid_ref)) { WARN_ONCE(tracing_sched_register() < 0, "Failed to register trace command line caching. Requires reboot to fix"); } -- Steve > +out: > mutex_unlock(&sched_register_mutex); > + return ret; > } > > static void tracing_stop_sched_switch(int ops) > @@ -130,9 +139,9 @@ static void tracing_stop_sched_switch(int ops) > mutex_unlock(&sched_register_mutex); > } > > -void tracing_start_cmdline_record(void) > +int tracing_start_cmdline_record(void) > { > - tracing_start_sched_switch(RECORD_CMDLINE); > + return tracing_start_sched_switch(RECORD_CMDLINE); > } > > void tracing_stop_cmdline_record(void) > @@ -140,9 +149,9 @@ void tracing_stop_cmdline_record(void) > tracing_stop_sched_switch(RECORD_CMDLINE); > } > > -void tracing_start_tgid_record(void) > +int tracing_start_tgid_record(void) > { > - tracing_start_sched_switch(RECORD_TGID); > + return tracing_start_sched_switch(RECORD_TGID); > } > > void tracing_stop_tgid_record(void) > diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c > index d88c44f1dfa5..238e7451f8e4 100644 > --- a/kernel/trace/trace_selftest.c > +++ b/kernel/trace/trace_selftest.c > @@ -1084,7 +1084,12 @@ trace_selftest_startup_function_graph(struct tracer *trace, > warn_failed_init_tracer(trace, ret); > goto out; > } > - tracing_start_cmdline_record(); > + ret = tracing_start_cmdline_record(); > + if (ret) { > + unregister_ftrace_graph(&fgraph_ops); > + warn_failed_init_tracer(trace, ret); > + goto out; > + } > > /* Sleep for a 1/10 of a second */ > msleep(100); ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] trace: propagate registration failure from tracing_start_*_record() 2026-04-17 15:52 ` Steven Rostedt @ 2026-04-18 5:38 ` Yash Suthar 0 siblings, 0 replies; 3+ messages in thread From: Yash Suthar @ 2026-04-18 5:38 UTC (permalink / raw) To: Steven Rostedt Cc: mhiramat, mathieu.desnoyers, linux-kernel, linux-trace-kernel, skhan, me, syzbot+a1d25e53cd4a10f7f2d3 Hello Steven, Thank you for taking a look and really sorry. I did use ai assistance for commit message, but I reviewed, modified and tested(with syzbot locally) the code myself. I should have disclosed really sorry. One thing I want to know (or I am still missing something): sched_cmdline_ref is incremented before tracing_sched_register() and register fails, but sched_cmdline_ref stays at 1 and on disable tracepoint_remove_func() sees NULL and return error (as syzbot reported and reproduce also locally). your suggestion WARN_ONCE correctly flags the upstream failure, but the secondary WARN at tracepoint.c:358 will still fire on the next disable, since the refcount desync isn't addressed. Was that intentional ? in my earlier approach, I tried to propagate the error and handle this desync under fault injection. I understand this may not align with the preferred direction, but that was the motivation behind the change. If you think it makes sense, I can prepare a v2 with your suggestion while also addressing the refcount consistency. Thank you On Fri, Apr 17, 2026 at 9:23 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 17 Apr 2026 12:08:27 +0530 > Yash Suthar <yashsuthar983@gmail.com> wrote: > > > syzbot reported a WARN in tracepoint_probe_unregister(): > > > > tracing_start_sched_switch() increments sched_cmdline_ref / > > sched_tgid_ref before calling tracing_sched_register(), and its > > return value is discarded because the API is void. When the first > > register_trace_sched_*() fails (e.g. kmalloc under memory pressure > > or failslab), the function's fail_deprobe* labels roll back any > > partial probe registration, but the caller's refcount has already > > been bumped. The state is now desynced: refs > 0 but no probes in > > tp->funcs. > > > > Later, when the caller pairs the start with a stop, the refcount > > walks back to 0 and tracing_sched_unregister() calls > > unregister_trace_sched_*() against an empty tp->funcs. > > func_remove() returns -ENOENT and the > > WARN_ON_ONCE(IS_ERR(old)) in tracepoint_remove_func() fires. > > > > Fix: make tracing_start_sched_switch() and the two exported > > wrappers, tracing_start_cmdline_record() and > > tracing_start_tgid_record(), return int; register the probes > > before bumping the refcount; and propagate the error to callers > > so refs are only held on behalf of a caller whose registration > > actually succeeded. > > > > Fixes: d914ba37d714 ("tracing: Add support for recording tgid of tasks") > > Reported-by: syzbot+a1d25e53cd4a10f7f2d3@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?id=f93e97cd824071a2577a40cde9ecd957f59f87eb > > Did you use AI to create any of this? If so you must disclose it. This > reads very much like an AI patch. > > > > > Signed-off-by: Yash Suthar <yashsuthar983@gmail.com> > > --- > > kernel/trace/trace.c | 6 +++--- > > kernel/trace/trace.h | 4 ++-- > > kernel/trace/trace_events.c | 28 +++++++++++++++++++-------- > > kernel/trace/trace_functions.c | 8 +++++++- > > kernel/trace/trace_functions_graph.c | 6 +++++- > > kernel/trace/trace_sched_switch.c | 29 ++++++++++++++++++---------- > > kernel/trace/trace_selftest.c | 7 ++++++- > > 7 files changed, 62 insertions(+), 26 deletions(-) > > NAK on all this. If you are under severe memory constraints that causes > this to fail, then you'll be hitting a bunch more errors. > > > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index 8bd4ec08fb36..e936eed99b27 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -3320,7 +3320,7 @@ void trace_printk_init_buffers(void) > > * allocated here, then this was called by module code. > > */ > > if (global_trace.array_buffer.buffer) > > - tracing_start_cmdline_record(); > > + (void)tracing_start_cmdline_record(); > > WTF??? Why are you adding the typecast of (void) here? Don't do that! > > > > } > > EXPORT_SYMBOL_GPL(trace_printk_init_buffers); > > > > @@ -3329,7 +3329,7 @@ void trace_printk_start_comm(void) > > /* Start tracing comms if trace printk is set */ > > if (!buffers_allocated) > > return; > > - tracing_start_cmdline_record(); > > + (void)tracing_start_cmdline_record(); > > } > > > > static void trace_printk_start_stop_comm(int enabled) > > @@ -3338,7 +3338,7 @@ static void trace_printk_start_stop_comm(int enabled) > > return; > > > > if (enabled) > > - tracing_start_cmdline_record(); > > + (void)tracing_start_cmdline_record(); > > else > > tracing_stop_cmdline_record(); > > } > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > > index b6d42fe06115..6fe2c8429560 100644 > > --- a/kernel/trace/trace.h > > +++ b/kernel/trace/trace.h > > @@ -751,9 +751,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace, struct fgraph_ops *gops, > > int trace_graph_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops, > > struct ftrace_regs *fregs); > > > > -void tracing_start_cmdline_record(void); > > +int tracing_start_cmdline_record(void); > > void tracing_stop_cmdline_record(void); > > -void tracing_start_tgid_record(void); > > +int tracing_start_tgid_record(void); > > void tracing_stop_tgid_record(void); > > > > int register_tracer(struct tracer *type); > > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > > index 137b4d9bb116..e6713aa80a03 100644 > > --- a/kernel/trace/trace_events.c > > +++ b/kernel/trace/trace_events.c > > @@ -734,9 +734,9 @@ void trace_event_enable_cmd_record(bool enable) > > continue; > > > > if (enable) { > > - tracing_start_cmdline_record(); > > - set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > > - } else { > > + if (!tracing_start_cmdline_record()) > > + set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > > + } else if (file->flags & EVENT_FILE_FL_RECORDED_CMD) { > > tracing_stop_cmdline_record(); > > clear_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > > } > > @@ -755,9 +755,9 @@ void trace_event_enable_tgid_record(bool enable) > > continue; > > > > if (enable) { > > - tracing_start_tgid_record(); > > - set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags); > > - } else { > > + if (!tracing_start_tgid_record()) > > + set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags); > > + } else if (file->flags & EVENT_FILE_FL_RECORDED_TGID) { > > tracing_stop_tgid_record(); > > clear_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, > > &file->flags); > > @@ -847,14 +847,26 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file, > > set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); > > > > if (tr->trace_flags & TRACE_ITER(RECORD_CMD)) { > > + ret = tracing_start_cmdline_record(); > > + if (ret) { > > + pr_info("event trace: Could not enable event %s\n", > > + trace_event_name(call)); > > + break; > > + } > > cmd = true; > > - tracing_start_cmdline_record(); > > set_bit(EVENT_FILE_FL_RECORDED_CMD_BIT, &file->flags); > > } > > > > if (tr->trace_flags & TRACE_ITER(RECORD_TGID)) { > > + ret = tracing_start_tgid_record(); > > + if (ret) { > > + if (cmd) > > + tracing_stop_cmdline_record(); > > + pr_info("event trace: Could not enable event %s\n", > > + trace_event_name(call)); > > + break; > > + } > > tgid = true; > > - tracing_start_tgid_record(); > > set_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags); > > } > > > > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c > > index c12795c2fb39..14d099734345 100644 > > --- a/kernel/trace/trace_functions.c > > +++ b/kernel/trace/trace_functions.c > > @@ -146,6 +146,8 @@ static bool handle_func_repeats(struct trace_array *tr, u32 flags_val) > > static int function_trace_init(struct trace_array *tr) > > { > > ftrace_func_t func; > > + int ret; > > + > > /* > > * Instance trace_arrays get their ops allocated > > * at instance creation. Unless it failed > > @@ -165,7 +167,11 @@ static int function_trace_init(struct trace_array *tr) > > > > tr->array_buffer.cpu = raw_smp_processor_id(); > > > > - tracing_start_cmdline_record(); > > + ret = tracing_start_cmdline_record(); > > + if (ret) { > > + ftrace_reset_array_ops(tr); > > + return ret; > > + } > > tracing_start_function_trace(tr); > > return 0; > > } > > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c > > index 1de6f1573621..6b27ed62fee8 100644 > > --- a/kernel/trace/trace_functions_graph.c > > +++ b/kernel/trace/trace_functions_graph.c > > @@ -487,7 +487,11 @@ static int graph_trace_init(struct trace_array *tr) > > ret = register_ftrace_graph(tr->gops); > > if (ret) > > return ret; > > - tracing_start_cmdline_record(); > > + ret = tracing_start_cmdline_record(); > > + if (ret) { > > + unregister_ftrace_graph(tr->gops); > > + return ret; > > + } > > > > return 0; > > } > > diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c > > index c46d584ded3b..683ea4ca1498 100644 > > --- a/kernel/trace/trace_sched_switch.c > > +++ b/kernel/trace/trace_sched_switch.c > > @@ -89,12 +89,22 @@ static void tracing_sched_unregister(void) > > unregister_trace_sched_wakeup(probe_sched_wakeup, NULL); > > } > > > > -static void tracing_start_sched_switch(int ops) > > +static int tracing_start_sched_switch(int ops) > > { > > - bool sched_register; > > + int ret = 0; > > > > mutex_lock(&sched_register_mutex); > > - sched_register = (!sched_cmdline_ref && !sched_tgid_ref); > > + > > + /* > > + * If the registration fails, do not bump the reference count : the > > + * caller must observe the failure so it can avoid a later matching > > + * stop that would otherwise unregister probes that were never added. > > + */ > > + if (!sched_cmdline_ref && !sched_tgid_ref) { > > + ret = tracing_sched_register(); > > + if (ret) > > + goto out; > > + } > > > > switch (ops) { > > case RECORD_CMDLINE: > > @@ -105,10 +115,9 @@ static void tracing_start_sched_switch(int ops) > > sched_tgid_ref++; > > break; > > } > > - > > - if (sched_register && (sched_cmdline_ref || sched_tgid_ref)) > > - tracing_sched_register(); > > The only change that should deal with this would be: > > if (sched_register && (sched_cmdline_ref || sched_tgid_ref)) { > WARN_ONCE(tracing_sched_register() < 0, > "Failed to register trace command line caching. Requires reboot to fix"); > } > > -- Steve > > > > > +out: > > mutex_unlock(&sched_register_mutex); > > + return ret; > > } > > > > static void tracing_stop_sched_switch(int ops) > > @@ -130,9 +139,9 @@ static void tracing_stop_sched_switch(int ops) > > mutex_unlock(&sched_register_mutex); > > } > > > > -void tracing_start_cmdline_record(void) > > +int tracing_start_cmdline_record(void) > > { > > - tracing_start_sched_switch(RECORD_CMDLINE); > > + return tracing_start_sched_switch(RECORD_CMDLINE); > > } > > > > void tracing_stop_cmdline_record(void) > > @@ -140,9 +149,9 @@ void tracing_stop_cmdline_record(void) > > tracing_stop_sched_switch(RECORD_CMDLINE); > > } > > > > -void tracing_start_tgid_record(void) > > +int tracing_start_tgid_record(void) > > { > > - tracing_start_sched_switch(RECORD_TGID); > > + return tracing_start_sched_switch(RECORD_TGID); > > } > > > > void tracing_stop_tgid_record(void) > > diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c > > index d88c44f1dfa5..238e7451f8e4 100644 > > --- a/kernel/trace/trace_selftest.c > > +++ b/kernel/trace/trace_selftest.c > > @@ -1084,7 +1084,12 @@ trace_selftest_startup_function_graph(struct tracer *trace, > > warn_failed_init_tracer(trace, ret); > > goto out; > > } > > - tracing_start_cmdline_record(); > > + ret = tracing_start_cmdline_record(); > > + if (ret) { > > + unregister_ftrace_graph(&fgraph_ops); > > + warn_failed_init_tracer(trace, ret); > > + goto out; > > + } > > > > /* Sleep for a 1/10 of a second */ > > msleep(100); > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-18 5:38 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-04-17 6:38 [PATCH] trace: propagate registration failure from tracing_start_*_record() Yash Suthar 2026-04-17 15:52 ` Steven Rostedt 2026-04-18 5:38 ` Yash Suthar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox