Re: [PATCH v3] tracing/net_sched: NULL pointer dereference in perf_trace_qdisc_reset()
From: Yunseong Kim <hidden>
Date: 2024-06-24 15:43:59
Also in:
linux-trace-kernel, lkml, stable
Hi Pedro, On 6/25/24 12:12 오전, Pedro Tammela wrote:
On 22/06/2024 01:57, yskelg@gmail.com wrote:quoted
From: Yunseong Kim <redacted> In the TRACE_EVENT(qdisc_reset) NULL dereference occurred from qdisc->dev_queue->dev <NULL> ->name [ 5301.595872] KASAN: null-ptr-deref in range [0x0000000000000130-0x0000000000000137] [ 5301.595877] Mem abort info: [ 5301.595881] ESR = 0x0000000096000006 [ 5301.595885] EC = 0x25: DABT (current EL), IL = 32 bits [ 5301.595889] SET = 0, FnV = 0 [ 5301.595893] EA = 0, S1PTW = 0 [ 5301.595896] FSC = 0x06: level 2 translation fault [ 5301.595900] Data abort info: [ 5301.595903] ISV = 0, ISS = 0x00000006, ISS2 = 0x00000000 [ 5301.595907] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 5301.595911] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 5301.595915] [dfff800000000026] address between user and kernel address ranges [ 5301.595971] Internal error: Oops: 0000000096000006 [#1] SMP Link: https://lore.kernel.org/lkml/20240229143432.273b4871@gandalf.local.home/t/ (local) Fixes: 51270d573a8d ("tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string") Cc: netdev@vger.kernel.org Cc: stable@vger.kernel.org # +v6.7.10, +v6.8 Signed-off-by: Yunseong Kim <redacted> Signed-off-by: Yeoreum Yun <redacted> --- include/trace/events/qdisc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h index f1b5e816e7e5..170b51fbe47a 100644 --- a/include/trace/events/qdisc.h +++ b/include/trace/events/qdisc.h@@ -81,7 +81,7 @@ TRACE_EVENT(qdisc_reset,TP_ARGS(q), TP_STRUCT__entry( - __string( dev, qdisc_dev(q)->name ) + __string(dev, qdisc_dev(q) ? qdisc_dev(q)->name : "noop_queue") __string( kind, q->ops->id ) __field( u32, parent ) __field( u32, handle )You missed the __assign_str portion (see below). Also let's just say "(null)" as it's the correct device name. "noop_queue" could be misleading.
Thanks for the code review Pedro, I agree your advice.
quoted hunk ↗ jump to hunk
diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h index 1f4258308b96..f54e0b4dbcf4 100644 --- a/include/trace/events/qdisc.h +++ b/include/trace/events/qdisc.h@@ -81,14 +81,14 @@ TRACE_EVENT(qdisc_reset,TP_ARGS(q), TP_STRUCT__entry( - __string( dev, qdisc_dev(q)->name ) + __string( dev, qdisc_dev(q) ? qdisc_dev(q)->name : "(null)" ) __string( kind, q->ops->id ) __field( u32, parent ) __field( u32, handle ) ),
It looks better to align the name with the current convention. Link: https://lore.kernel.org/linux-trace-kernel/20240222211442.634192653@goodmis.org/ (local)
TP_fast_assign( - __assign_str(dev, qdisc_dev(q)->name); + __assign_str(dev, qdisc_dev(q) ? qdisc_dev(q)->name : "(null)"); __assign_str(kind, q->ops->id); __entry->parent = q->parent; __entry->handle = q->handle;
The second part you mentioned, Steve recently worked on it and changed it. Link: https://lore.kernel.org/linux-trace-kernel/20240516133454.681ba6a0@rorschach.local.home/ (local) If it hadn't, I don't think I would have been able to prevent the panic by just applying my patch. Link: https://lore.kernel.org/all/e2f9da4e-919d-4a98-919d-167726ef6bc7@gmail.com/ (local) Warm Regards, Yunseong Kim