* [PATCH] tracing: Fix crash in print_graph_function_event()
@ 2026-09-20 4:21 Hao Ge
2026-10-14 23:05 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Hao Ge @ 2026-09-20 4:21 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-trace-kernel, Hao Ge, stable
tracing_set_tracer() assigns the flags of the newly selected tracer
with:
tr->current_trace_flags = t->flags ? : t->tracer->flags;
If the new tracer defines neither static flags nor default flags, like
hwlat, irqsoff or wakeup, add_tracer() has no per instance flags copy
for it, and both operands are NULL, so tr->current_trace_flags ends up
being NULL as well.
The ring buffer is not cleared when switching tracers: it is reset
inside tracer_init(), after current_trace_flags was already assigned.
If the buffer still holds function_graph entries of the previous
tracer, printing one of them dereferences the NULL pointer. This was
hit by cat trace during an ftrace stress test:
BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor read access in kernel mode
RIP: 0010:print_graph_function_event+0xe/0x20
Call Trace:
s_show+0x4d/0x110
seq_read_iter+0x2d4/0x490
seq_read+0x130/0x170
vfs_read+0xb5/0x370
ksys_read+0x6d/0xe0
do_syscall_64+0xbc/0x450
The snapshot buffer is not reset at all, so this can be reproduced
without a race:
echo function_graph > /sys/kernel/tracing/current_tracer
echo 1 > /sys/kernel/tracing/snapshot
echo hwlat > /sys/kernel/tracing/current_tracer
cat /sys/kernel/tracing/snapshot
Rather than teaching every user of tr->current_trace_flags about this
corner case, make sure every tracer has a set of flags:
register_tracer() assigns the empty no_tracer_flags to tracers that
define none. The existing users already handle opts == NULL, so these
tracers keep behaving as before.
Fixes: e29aa918a928 ("tracing: Have function graph tracer define options per instance")
Cc: stable@vger.kernel.org
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
kernel/trace/trace.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e4a490d3d08c..1bd757b65be4 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -99,6 +99,12 @@ struct tracers {
struct tracer_flags *flags;
};
+/*
+ * Tracers that do not define their own flags use these empty flags,
+ * as tr->current_trace_flags must never be NULL.
+ */
+static struct tracer_flags no_tracer_flags;
+
/*
* To prevent the comm cache from being overwritten when no
* tracing is active, only save the comm when a trace event
@@ -1485,9 +1491,11 @@ int __init register_tracer(struct tracer *type)
}
}
- /* store the tracer for __set_tracer_option */
+ /* All tracers must have flags, use the empty flags if none are defined */
if (type->flags)
type->flags->trace = type;
+ else if (!type->default_flags)
+ type->default_flags = &no_tracer_flags;
ret = do_run_tracer_selftest(type);
if (ret < 0)
@@ -8096,9 +8104,6 @@ static int add_tracer(struct trace_array *tr, struct tracer *tracer)
flags = tracer->flags;
if (!flags) {
- if (!tracer->default_flags)
- return 0;
-
/*
* If the tracer defines default flags, it means the flags are
* per trace instance.
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] tracing: Fix crash in print_graph_function_event() 2026-09-20 4:21 [PATCH] tracing: Fix crash in print_graph_function_event() Hao Ge @ 2026-10-14 23:05 ` Steven Rostedt 2026-10-15 7:18 ` Hao Ge 0 siblings, 1 reply; 3+ messages in thread From: Steven Rostedt @ 2026-10-14 23:05 UTC (permalink / raw) To: Hao Ge Cc: Masami Hiramatsu, Mathieu Desnoyers, linux-kernel, linux-trace-kernel, stable On Sun, 20 Sep 2026 12:21:39 +0800 Hao Ge <hao.ge@linux.dev> wrote: > tracing_set_tracer() assigns the flags of the newly selected tracer > with: > > tr->current_trace_flags = t->flags ? : t->tracer->flags; > > If the new tracer defines neither static flags nor default flags, like > hwlat, irqsoff or wakeup, add_tracer() has no per instance flags copy > for it, and both operands are NULL, so tr->current_trace_flags ends up > being NULL as well. > > The ring buffer is not cleared when switching tracers: it is reset > inside tracer_init(), after current_trace_flags was already assigned. > If the buffer still holds function_graph entries of the previous > tracer, printing one of them dereferences the NULL pointer. This was > hit by cat trace during an ftrace stress test: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > #PF: supervisor read access in kernel mode > RIP: 0010:print_graph_function_event+0xe/0x20 > Call Trace: > s_show+0x4d/0x110 > seq_read_iter+0x2d4/0x490 > seq_read+0x130/0x170 > vfs_read+0xb5/0x370 > ksys_read+0x6d/0xe0 > do_syscall_64+0xbc/0x450 > > The snapshot buffer is not reset at all, so this can be reproduced > without a race: > > echo function_graph > /sys/kernel/tracing/current_tracer > echo 1 > /sys/kernel/tracing/snapshot > echo hwlat > /sys/kernel/tracing/current_tracer > cat /sys/kernel/tracing/snapshot > > Rather than teaching every user of tr->current_trace_flags about this > corner case, make sure every tracer has a set of flags: > register_tracer() assigns the empty no_tracer_flags to tracers that > define none. The existing users already handle opts == NULL, so these > tracers keep behaving as before. Nice catch, but unfortunately this patch causes this: [ 13.937660] WARNING: kernel/trace/trace.c:8123 at add_tracer.part.0+0x495/0x930, CPU#3: swapper/0/1 [ 13.947844] Modules linked in: [ 13.949520] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.3.0-rc7-test-00017-g26bd9362561c #543 PREEMPT(lazy) [ 13.953831] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-debian-1.17.0-1 04/01/2014 [ 13.957723] RIP: 0010:add_tracer.part.0+0x495/0x930 [ 13.959852] Code: 85 73 03 00 00 49 3b 6d 10 74 26 4c 89 e8 48 c1 e8 03 42 80 3c 38 00 0f 85 3c 03 00 00 4d 8b 6d 00 49 81 fd 28 5b d8 85 75 c5 <0f> 0b e9 73 ff ff ff 48 b8 00 00 00 00 00 fc ff df 49 8d 7d 18 48 [ 13.967362] RSP: 0000:ffff888100e67bf0 EFLAGS: 00010246 [ 13.969686] RAX: 1ffff11020012790 RBX: ffff888109fc62c0 RCX: ffff888109fc62d0 [ 13.972635] RDX: ffff888109fc63c0 RSI: 1ffff1102013c3f2 RDI: ffff888100093c90 [ 13.975626] RBP: ffffffff865c0640 R08: 1ffffffff1006101 R09: 1ffffffff0cb7da4 [ 13.980293] R10: 0000000000000003 R11: fffffbfff0cb7da5 R12: ffff8881009e0000 [ 13.983285] R13: ffffffff85d85b28 R14: fffffbfff0cb80db R15: dffffc0000000000 [ 13.986297] FS: 0000000000000000(0000) GS:ffff8882b4cbf000(0000) knlGS:0000000000000000 [ 13.989781] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 13.992213] CR2: 0000000000000000 CR3: 0000000005aec001 CR4: 0000000000172ef0 [ 13.995221] Call Trace: [ 13.996469] <TASK> [ 13.997680] ? __pfx_init_hwlat_tracer+0x10/0x10 [ 13.999725] register_tracer+0x30f/0x470 [ 14.001594] ? __pfx_init_hwlat_tracer+0x10/0x10 [ 14.003632] init_hwlat_tracer+0x2c/0x120 [ 14.005539] ? __pfx_init_hwlat_tracer+0x10/0x10 [ 14.007604] do_one_initcall+0x11e/0x540 [ 14.011129] ? __pfx_do_one_initcall+0x10/0x10 [ 14.013124] ? __pfx_parse_args+0x10/0x10 [ 14.015006] ? kernel_init_freeable+0x4cb/0x710 [ 14.017032] ? rcu_is_watching+0x16/0xb0 [ 14.018887] kernel_init_freeable+0x65c/0x710 [ 14.020853] ? __pfx_kernel_init_freeable+0x10/0x10 [ 14.023036] ? _raw_spin_unlock_irq+0x38/0x60 [ 14.024995] ? __pfx_do_raw_spin_lock+0x10/0x10 [ 14.027045] ? __pfx_kernel_init+0x10/0x10 [ 14.028894] ? rcu_is_watching+0x16/0xb0 [ 14.030718] ? __pfx_kernel_init+0x10/0x10 [ 14.032555] kernel_init+0x21/0x150 [ 14.034249] ? __pfx_kernel_init+0x10/0x10 [ 14.036086] ret_from_fork+0x65c/0x9e0 [ 14.037893] ? __pfx_ret_from_fork+0x10/0x10 [ 14.041417] ? __switch_to+0x3ad/0xda0 [ 14.043154] ? __switch_to_asm+0x39/0x70 [ 14.044953] ? __switch_to_asm+0x33/0x70 [ 14.046792] ? __pfx_kernel_init+0x10/0x10 [ 14.048646] ret_from_fork_asm+0x1a/0x30 > > Fixes: e29aa918a928 ("tracing: Have function graph tracer define options per instance") > Cc: stable@vger.kernel.org > Signed-off-by: Hao Ge <hao.ge@linux.dev> > --- > kernel/trace/trace.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index e4a490d3d08c..1bd757b65be4 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -99,6 +99,12 @@ struct tracers { > struct tracer_flags *flags; > }; > > +/* > + * Tracers that do not define their own flags use these empty flags, > + * as tr->current_trace_flags must never be NULL. > + */ > +static struct tracer_flags no_tracer_flags; > + > /* > * To prevent the comm cache from being overwritten when no > * tracing is active, only save the comm when a trace event > @@ -1485,9 +1491,11 @@ int __init register_tracer(struct tracer *type) > } > } > > - /* store the tracer for __set_tracer_option */ > + /* All tracers must have flags, use the empty flags if none are defined */ > if (type->flags) > type->flags->trace = type; > + else if (!type->default_flags) > + type->default_flags = &no_tracer_flags; > > ret = do_run_tracer_selftest(type); > if (ret < 0) > @@ -8096,9 +8104,6 @@ static int add_tracer(struct trace_array *tr, struct tracer *tracer) > > flags = tracer->flags; > if (!flags) { > - if (!tracer->default_flags) > - return 0; > - > /* > * If the tracer defines default flags, it means the flags are > * per trace instance. Instead of assigning to the tracer, how about just updating tr->current_trace_flags? Like this: diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 3b22e70c6016..b6b7d86866ef 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -5001,6 +5001,12 @@ static void tracing_set_nop(struct trace_array *tr) static bool tracer_options_updated; +/* + * Tracers that do not define their own flags use these empty flags, + * as tr->current_trace_flags must never be NULL. + */ +static struct tracer_flags no_tracer_flags; + int tracing_set_tracer(struct trace_array *tr, const char *buf) { struct tracer *trace = NULL; @@ -5090,6 +5096,8 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf) } tr->current_trace_flags = t->flags ? : t->tracer->flags; + if (!tr->current_trace_flags) + tr->current_trace_flags = &no_tracer_flags; if (trace->init) { ret = tracer_init(trace, tr); It appears to fix the issue you found without any other side effects. Care to send a v2? Thanks, -- Steve ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] tracing: Fix crash in print_graph_function_event() 2026-10-14 23:05 ` Steven Rostedt @ 2026-10-15 7:18 ` Hao Ge 0 siblings, 0 replies; 3+ messages in thread From: Hao Ge @ 2026-10-15 7:18 UTC (permalink / raw) To: Steven Rostedt Cc: Masami Hiramatsu, Mathieu Desnoyers, linux-kernel, linux-trace-kernel, stable Hi Steven On 2026/10/15 07:05, Steven Rostedt wrote: > On Sun, 20 Sep 2026 12:21:39 +0800 > Hao Ge <hao.ge@linux.dev> wrote: > >> tracing_set_tracer() assigns the flags of the newly selected tracer >> with: >> >> tr->current_trace_flags = t->flags ? : t->tracer->flags; >> >> If the new tracer defines neither static flags nor default flags, like >> hwlat, irqsoff or wakeup, add_tracer() has no per instance flags copy >> for it, and both operands are NULL, so tr->current_trace_flags ends up >> being NULL as well. >> >> The ring buffer is not cleared when switching tracers: it is reset >> inside tracer_init(), after current_trace_flags was already assigned. >> If the buffer still holds function_graph entries of the previous >> tracer, printing one of them dereferences the NULL pointer. This was >> hit by cat trace during an ftrace stress test: >> >> BUG: kernel NULL pointer dereference, address: 0000000000000000 >> #PF: supervisor read access in kernel mode >> RIP: 0010:print_graph_function_event+0xe/0x20 >> Call Trace: >> s_show+0x4d/0x110 >> seq_read_iter+0x2d4/0x490 >> seq_read+0x130/0x170 >> vfs_read+0xb5/0x370 >> ksys_read+0x6d/0xe0 >> do_syscall_64+0xbc/0x450 >> >> The snapshot buffer is not reset at all, so this can be reproduced >> without a race: >> >> echo function_graph > /sys/kernel/tracing/current_tracer >> echo 1 > /sys/kernel/tracing/snapshot >> echo hwlat > /sys/kernel/tracing/current_tracer >> cat /sys/kernel/tracing/snapshot >> >> Rather than teaching every user of tr->current_trace_flags about this >> corner case, make sure every tracer has a set of flags: >> register_tracer() assigns the empty no_tracer_flags to tracers that >> define none. The existing users already handle opts == NULL, so these >> tracers keep behaving as before. > > Nice catch, but unfortunately this patch causes this: > > [ 13.937660] WARNING: kernel/trace/trace.c:8123 at add_tracer.part.0+0x495/0x930, CPU#3: swapper/0/1 > [ 13.947844] Modules linked in: > [ 13.949520] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.3.0-rc7-test-00017-g26bd9362561c #543 PREEMPT(lazy) > [ 13.953831] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-debian-1.17.0-1 04/01/2014 > [ 13.957723] RIP: 0010:add_tracer.part.0+0x495/0x930 > [ 13.959852] Code: 85 73 03 00 00 49 3b 6d 10 74 26 4c 89 e8 48 c1 e8 03 42 80 3c 38 00 0f 85 3c 03 00 00 4d 8b 6d 00 49 81 fd 28 5b d8 85 75 c5 <0f> 0b e9 73 ff ff ff 48 b8 00 00 00 00 00 fc ff df 49 8d 7d 18 48 > [ 13.967362] RSP: 0000:ffff888100e67bf0 EFLAGS: 00010246 > [ 13.969686] RAX: 1ffff11020012790 RBX: ffff888109fc62c0 RCX: ffff888109fc62d0 > [ 13.972635] RDX: ffff888109fc63c0 RSI: 1ffff1102013c3f2 RDI: ffff888100093c90 > [ 13.975626] RBP: ffffffff865c0640 R08: 1ffffffff1006101 R09: 1ffffffff0cb7da4 > [ 13.980293] R10: 0000000000000003 R11: fffffbfff0cb7da5 R12: ffff8881009e0000 > [ 13.983285] R13: ffffffff85d85b28 R14: fffffbfff0cb80db R15: dffffc0000000000 > [ 13.986297] FS: 0000000000000000(0000) GS:ffff8882b4cbf000(0000) knlGS:0000000000000000 > [ 13.989781] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 13.992213] CR2: 0000000000000000 CR3: 0000000005aec001 CR4: 0000000000172ef0 > [ 13.995221] Call Trace: > [ 13.996469] <TASK> > [ 13.997680] ? __pfx_init_hwlat_tracer+0x10/0x10 > [ 13.999725] register_tracer+0x30f/0x470 > [ 14.001594] ? __pfx_init_hwlat_tracer+0x10/0x10 > [ 14.003632] init_hwlat_tracer+0x2c/0x120 > [ 14.005539] ? __pfx_init_hwlat_tracer+0x10/0x10 > [ 14.007604] do_one_initcall+0x11e/0x540 > [ 14.011129] ? __pfx_do_one_initcall+0x10/0x10 > [ 14.013124] ? __pfx_parse_args+0x10/0x10 > [ 14.015006] ? kernel_init_freeable+0x4cb/0x710 > [ 14.017032] ? rcu_is_watching+0x16/0xb0 > [ 14.018887] kernel_init_freeable+0x65c/0x710 > [ 14.020853] ? __pfx_kernel_init_freeable+0x10/0x10 > [ 14.023036] ? _raw_spin_unlock_irq+0x38/0x60 > [ 14.024995] ? __pfx_do_raw_spin_lock+0x10/0x10 > [ 14.027045] ? __pfx_kernel_init+0x10/0x10 > [ 14.028894] ? rcu_is_watching+0x16/0xb0 > [ 14.030718] ? __pfx_kernel_init+0x10/0x10 > [ 14.032555] kernel_init+0x21/0x150 > [ 14.034249] ? __pfx_kernel_init+0x10/0x10 > [ 14.036086] ret_from_fork+0x65c/0x9e0 > [ 14.037893] ? __pfx_ret_from_fork+0x10/0x10 > [ 14.041417] ? __switch_to+0x3ad/0xda0 > [ 14.043154] ? __switch_to_asm+0x39/0x70 > [ 14.044953] ? __switch_to_asm+0x33/0x70 > [ 14.046792] ? __pfx_kernel_init+0x10/0x10 > [ 14.048646] ret_from_fork_asm+0x1a/0x30 > Thanks for catching this. I was confused at first why I didn't hit this in my testing. Then after reading the code, I found it needs an instance to exist before hwlat registers. So it's reliably reproducible with trace_instance=xxx on the cmdline. >> >> Fixes: e29aa918a928 ("tracing: Have function graph tracer define options per instance") >> Cc: stable@vger.kernel.org >> Signed-off-by: Hao Ge <hao.ge@linux.dev> >> --- >> kernel/trace/trace.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >> index e4a490d3d08c..1bd757b65be4 100644 >> --- a/kernel/trace/trace.c >> +++ b/kernel/trace/trace.c >> @@ -99,6 +99,12 @@ struct tracers { >> struct tracer_flags *flags; >> }; >> >> +/* >> + * Tracers that do not define their own flags use these empty flags, >> + * as tr->current_trace_flags must never be NULL. >> + */ >> +static struct tracer_flags no_tracer_flags; >> + >> /* >> * To prevent the comm cache from being overwritten when no >> * tracing is active, only save the comm when a trace event >> @@ -1485,9 +1491,11 @@ int __init register_tracer(struct tracer *type) >> } >> } >> >> - /* store the tracer for __set_tracer_option */ >> + /* All tracers must have flags, use the empty flags if none are defined */ >> if (type->flags) >> type->flags->trace = type; >> + else if (!type->default_flags) >> + type->default_flags = &no_tracer_flags; >> >> ret = do_run_tracer_selftest(type); >> if (ret < 0) >> @@ -8096,9 +8104,6 @@ static int add_tracer(struct trace_array *tr, struct tracer *tracer) >> >> flags = tracer->flags; >> if (!flags) { >> - if (!tracer->default_flags) >> - return 0; >> - >> /* >> * If the tracer defines default flags, it means the flags are >> * per trace instance. > > Instead of assigning to the tracer, how about just updating > tr->current_trace_flags? > > Like this: > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 3b22e70c6016..b6b7d86866ef 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -5001,6 +5001,12 @@ static void tracing_set_nop(struct trace_array *tr) > > static bool tracer_options_updated; > > +/* > + * Tracers that do not define their own flags use these empty flags, > + * as tr->current_trace_flags must never be NULL. > + */ > +static struct tracer_flags no_tracer_flags; > + > int tracing_set_tracer(struct trace_array *tr, const char *buf) > { > struct tracer *trace = NULL; > @@ -5090,6 +5096,8 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf) > } > > tr->current_trace_flags = t->flags ? : t->tracer->flags; > + if (!tr->current_trace_flags) > + tr->current_trace_flags = &no_tracer_flags; > > if (trace->init) { > ret = tracer_init(trace, tr); > > > It appears to fix the issue you found without any other side effects. Right, I verified this patch; both the crash and WARN are gone. But I think we also need to add this logic inside run_tracer_selftest. @@ -1319,6 +1325,8 @@ static int run_tracer_selftest(struct tracer *type) tr->current_trace = type; tr->current_trace_flags = type->flags ? : type->default_flags; + if (!tr->current_trace_flags) + tr->current_trace_flags = &no_tracer_flags; > > Care to send a v2? With the above changes, I'll send out a v2 shortly. Thanks Best Regard Hao > > Thanks, > > -- Steve ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-15 7:17 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-20 4:21 [PATCH] tracing: Fix crash in print_graph_function_event() Hao Ge 2026-10-14 23:05 ` Steven Rostedt 2026-10-15 7:18 ` Hao Ge
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.