netdev - Re: [PATCH] tracing/net_sched: Fix tracepoints that save qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMkOgTezVLnN7f1GoXTURQ73LmXjHnBjQBSBRPnv58K-VQ@mail.gmail.com>
Date: Fri, 1 Mar 2024 14:24:17 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org, 
	Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, 
	Cong Wang <xiyou.wangcong@...il.com>, vaclav.zindulka@...pnet.cz, 
	Jiri Pirko <jiri@...nulli.us>, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev()
 as a string

On Thu, Feb 29, 2024 at 2:32 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
>
> I'm updating __assign_str() and will be removing the second parameter. To
> make sure that it does not break anything, I make sure that it matches the
> __string() field, as that is where the string is actually going to be
> saved in. To make sure there's nothing that breaks, I added a WARN_ON() to
> make sure that what was used in __string() is the same that is used in
> __assign_str().
>
> In doing this change, an error was triggered as __assign_str() now expects
> the string passed in to be a char * value. I instead had the following
> warning:
>
> include/trace/events/qdisc.h: In function ‘trace_event_raw_event_qdisc_reset’:
> include/trace/events/qdisc.h:91:35: error: passing argument 1 of 'strcmp' from incompatible pointer type [-Werror=incompatible-pointer-types]
>    91 |                 __assign_str(dev, qdisc_dev(q));
>
> That's because the qdisc_enqueue() and qdisc_reset() pass in qdisc_dev(q)
> to __assign_str() and to __string(). But that function returns a pointer
> to struct net_device and not a string.
>
> It appears that these events are just saving the pointer as a string and
> then reading it as a string as well.
>
> Use qdisc_dev(q)->name to save the device instead.
>
> Fixes: a34dac0b90552 ("net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()")
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

Should this be targeting the net tree?
Otherwise, LGTM. Just wondering - this worked before because "name"
was the first field?

Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal
> ---
>  include/trace/events/qdisc.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h
> index a3995925cb05..1f4258308b96 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)    )
> -               __string(       kind,           q->ops->id      )
> -               __field(        u32,            parent          )
> -               __field(        u32,            handle          )
> +               __string(       dev,            qdisc_dev(q)->name      )
> +               __string(       kind,           q->ops->id              )
> +               __field(        u32,            parent                  )
> +               __field(        u32,            handle                  )
>         ),
>
>         TP_fast_assign(
> -               __assign_str(dev, qdisc_dev(q));
> +               __assign_str(dev, qdisc_dev(q)->name);
>                 __assign_str(kind, q->ops->id);
>                 __entry->parent = q->parent;
>                 __entry->handle = q->handle;
> @@ -106,14 +106,14 @@ TRACE_EVENT(qdisc_destroy,
>         TP_ARGS(q),
>
>         TP_STRUCT__entry(
> -               __string(       dev,            qdisc_dev(q)    )
> -               __string(       kind,           q->ops->id      )
> -               __field(        u32,            parent          )
> -               __field(        u32,            handle          )
> +               __string(       dev,            qdisc_dev(q)->name      )
> +               __string(       kind,           q->ops->id              )
> +               __field(        u32,            parent                  )
> +               __field(        u32,            handle                  )
>         ),
>
>         TP_fast_assign(
> -               __assign_str(dev, qdisc_dev(q));
> +               __assign_str(dev, qdisc_dev(q)->name);
>                 __assign_str(kind, q->ops->id);
>                 __entry->parent = q->parent;
>                 __entry->handle = q->handle;
> --
> 2.43.0
>