[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <06d0ea61-47ee-4e54-9dfa-a711c5bc07d0@mojatatu.com>
Date: Mon, 24 Jun 2024 12:55:35 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Yunseong Kim <yskelg@...il.com>
Cc: netdev@...r.kernel.org, stable@...r.kernel.org,
Jakub Kicinski <kuba@...nel.org>, Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Takashi Iwai <tiwai@...e.de>, "David S. Miller" <davem@...emloft.net>,
Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
"Rafael J. Wysocki" <rafael@...nel.org>, Jamal Hadi Salim
<jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...nulli.us>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Taehee Yoo <ap420073@...il.com>,
Austin Kim <austindh.kim@...il.com>, shjy180909@...il.com,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
ppbuk5246@...il.com, Yeoreum Yun <yeoreum.yun@....com>
Subject: Re: [PATCH v3] tracing/net_sched: NULL pointer dereference in
perf_trace_qdisc_reset()
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@...il.com wrote:
>>> From: Yunseong Kim <yskelg@...il.com>
>>>
>>> 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/
>>> Fixes: 51270d573a8d ("tracing/net_sched: Fix tracepoints that save
>>> qdisc_dev() as a string")
>>> Cc: netdev@...r.kernel.org
>>> Cc: stable@...r.kernel.org # +v6.7.10, +v6.8
>>> Signed-off-by: Yunseong Kim <yskelg@...il.com>
>>> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
>>> ---
>>> 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@goodmis.org/
>
>> 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/
Oh!
>
> 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?
Powered by blists - more mailing lists