Hi Pedro,

On 6/25/24 12:55 오전, Pedro Tammela wrote:

On 24/06/2024 12:43, Yunseong Kim wrote:

quoted

Hi Pedro,

On 6/25/24 12:12 오전, Pedro Tammela wrote:

quoted

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

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)

quoted

         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)

Oh!

Thanks for the double check, Pedro.

quoted

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