Hi Pedro, On 6/25/24 12:55 오전, Pedro Tammela wrote: > On 24/06/2024 12:43, Yunseong Kim wrote: >> Hi Pedro, >> >> On 6/25/24 12:12 오전, Pedro Tammela wrote: >>> On 22/06/2024 01:57, yskelg@xxxxxxxxx wrote: >>>> From: Yunseong Kim <yskelg@xxxxxxxxx> >>>> >>>> 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@xxxxxxxxxxxxxxxxxx/t/ >>>> Fixes: 51270d573a8d ("tracing/net_sched: Fix tracepoints that save >>>> qdisc_dev() as a string") >>>> Cc: netdev@xxxxxxxxxxxxxxx >>>> Cc: stable@xxxxxxxxxxxxxxx # +v6.7.10, +v6.8 >>>> Signed-off-by: Yunseong Kim <yskelg@xxxxxxxxx> >>>> Signed-off-by: Yeoreum Yun <yeoreum.yun@xxxxxxx> >>>> --- >>>> 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. >> >>> 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@xxxxxxxxxxx/ >> >>> 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@xxxxxxxxxxxxxxxxxxxx/ > > Oh! Thanks for the double check, Pedro. >> If it hadn't, I don't think I would have been able to prevent the panic >> by just applying my patch. > > But you must be careful with the backports. > > In any case, perhaps send another patch to net-next updating the new > conventions there and use the 'old convention' for the bug fix? Right, I agree, I'll send a patch for the next version. Warm regards, Yunseong Kim
