[PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string

[PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.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@goodmis.org>
---
 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

Re: [PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string

[Jamal Hadi Salim]

On Thu, Feb 29, 2024 at 2:32 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Steven Rostedt (Google)" <rostedt@goodmis.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@goodmis.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@mojatatu.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
>

Re: [PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string

[Steven Rostedt]

On Fri, 1 Mar 2024 14:24:17 -0500
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> > Fixes: a34dac0b90552 ("net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()")
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>  
> 
> Should this be targeting the net tree?

I was going to say that I need this for my work, but my work is aimed at
the next merge window, but this should go into the kernel now and be marked
for stable. So yes, it probably should go through the net tree.

Do I need to resubmit it?

> Otherwise, LGTM. Just wondering - this worked before because "name"
> was the first field?

Looks like it. See commit 43a71cd66b9c0 ("net-device: reorganize net_device
fast path variables")

I wonder if there's anything else that uses a pointer to struct net_device
thinking it can just be switched to find the name?

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

Thanks,

-- Steve

Re: [PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string

[Jamal Hadi Salim]

On Fri, Mar 1, 2024 at 2:59 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 1 Mar 2024 14:24:17 -0500
> Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> > > Fixes: a34dac0b90552 ("net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()")
> > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> >
> > Should this be targeting the net tree?
>
> I was going to say that I need this for my work, but my work is aimed at
> the next merge window, but this should go into the kernel now and be marked
> for stable. So yes, it probably should go through the net tree.
>
> Do I need to resubmit it?
>

My view is it needs to be merged.
I note there are some big changes in net and net-next trees right now
that move the name - probably from 43a71cd66b9c0
Coco Li and Eric are on your Cc already.

> > Otherwise, LGTM. Just wondering - this worked before because "name"
> > was the first field?
>
> Looks like it. See commit 43a71cd66b9c0 ("net-device: reorganize net_device
> fast path variables")
>
> I wonder if there's anything else that uses a pointer to struct net_device
> thinking it can just be switched to find the name?
>

From a quick scan i cant find anything obvious.

cheers,
jamal

> >
> > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Thanks,
>
> -- Steve

Re: [PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 29 Feb 2024 14:34:44 -0500 you wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.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().
> 
> [...]

Here is the summary with links:
  - tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string
    https://git.kernel.org/netdev/net/c/51270d573a8d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html