1. Patchew
  2. linux
  3. View series

[PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()

Thorsten Blum posted 1 patch 2 months, 3 weeks ago 
include/trace/events/fib6.h | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
[PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Thorsten Blum 2 months, 3 weeks ago 
strcpy() is deprecated; use strscpy() instead.

Since the destination buffer has a fixed length, strscpy() automatically
determines its size using sizeof() when the size argument is omitted.
This makes the explicit size argument unnecessary - remove it.

Now, combine both if-else branches using strscpy() and the same buffer
into a single statement to simplify the code.

No functional changes intended.

Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 include/trace/events/fib6.h | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/fib6.h b/include/trace/events/fib6.h
index 8d22b2e98d48..903a18836bc6 100644
--- a/include/trace/events/fib6.h
+++ b/include/trace/events/fib6.h
@@ -64,11 +64,9 @@ TRACE_EVENT(fib6_table_lookup,
 			__entry->dport = 0;
 		}
 
-		if (res->nh && res->nh->fib_nh_dev) {
-			strscpy(__entry->name, res->nh->fib_nh_dev->name, IFNAMSIZ);
-		} else {
-			strcpy(__entry->name, "-");
-		}
+		strscpy(__entry->name, res->nh && res->nh->fib_nh_dev ?
+				       res->nh->fib_nh_dev->name : "-");
+
 		if (res->f6i == net->ipv6.fib6_null_entry) {
 			in6 = (struct in6_addr *)__entry->gw;
 			*in6 = in6addr_any;
-- 
2.50.0
Re: [PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Steven Rostedt 2 months, 3 weeks ago 
On Mon, 14 Jul 2025 09:54:33 +0200
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> diff --git a/include/trace/events/fib6.h b/include/trace/events/fib6.h
> index 8d22b2e98d48..903a18836bc6 100644
> --- a/include/trace/events/fib6.h
> +++ b/include/trace/events/fib6.h
> @@ -64,11 +64,9 @@ TRACE_EVENT(fib6_table_lookup,
>  			__entry->dport = 0;
>  		}
>  
> -		if (res->nh && res->nh->fib_nh_dev) {
> -			strscpy(__entry->name, res->nh->fib_nh_dev->name, IFNAMSIZ);
> -		} else {
> -			strcpy(__entry->name, "-");
> -		}
> +		strscpy(__entry->name, res->nh && res->nh->fib_nh_dev ?
> +				       res->nh->fib_nh_dev->name : "-");
> +
>  		if (res->f6i == net->ipv6.fib6_null_entry) {
>  			in6 = (struct in6_addr *)__entry->gw;
>  			*in6 = in6addr_any;

Hmm, why is that string hard coded to 16 bytes and doesn't use the
dynamic string facility? Perhaps something like this?

[ I didn't even compile the below, so it may have a syntax error ]

-- Steve

diff --git a/include/trace/events/fib6.h b/include/trace/events/fib6.h
index 8d22b2e98d48..98d2edb02431 100644
--- a/include/trace/events/fib6.h
+++ b/include/trace/events/fib6.h
@@ -32,7 +32,8 @@ TRACE_EVENT(fib6_table_lookup,
 		__field(        u16,	dport		)
 		__field(        u8,	proto		)
 		__field(        u8,	rt_type		)
-		__array(		char,	name,	IFNAMSIZ )
+		__string(	name,	res->nh && res->nh->fib_nh_dev ?
+					res->nh->fib_nh_dev->name : "-")
 		__array(		__u8,	gw,	16	 )
 	),
 
@@ -64,11 +65,7 @@ TRACE_EVENT(fib6_table_lookup,
 			__entry->dport = 0;
 		}
 
-		if (res->nh && res->nh->fib_nh_dev) {
-			strscpy(__entry->name, res->nh->fib_nh_dev->name, IFNAMSIZ);
-		} else {
-			strcpy(__entry->name, "-");
-		}
+		__assign_str(name);
 		if (res->f6i == net->ipv6.fib6_null_entry) {
 			in6 = (struct in6_addr *)__entry->gw;
 			*in6 = in6addr_any;
@@ -82,7 +79,7 @@ TRACE_EVENT(fib6_table_lookup,
 		  __entry->tb_id, __entry->oif, __entry->iif, __entry->proto,
 		  __entry->src, __entry->sport, __entry->dst, __entry->dport,
 		  __entry->flowlabel, __entry->tos, __entry->scope,
-		  __entry->flags, __entry->name, __entry->gw, __entry->err)
+		  __entry->flags, __get_str(name), __entry->gw, __entry->err)
 );
 
 #endif /* _TRACE_FIB6_H */
Re: [PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Thorsten Blum 2 months, 2 weeks ago 
Hi Steve,

On 14. Jul 2025, at 09:38, Steven Rostedt wrote:
> 
> Hmm, why is that string hard coded to 16 bytes and doesn't use the
> dynamic string facility? Perhaps something like this?

Your commit fca8300f68fe3 changed it from __dynamic_array() to __array()
and __string() seems to be just a special version of __dynamic_array()
with a length of -1.

In the commit description you wrote: "Since the size of the name is at
most 16 bytes (defined by IFNAMSIZ), it is not worth spending the effort
to determine the size of the string."

Thanks,
Thorsten
Re: [PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Steven Rostedt 2 months, 2 weeks ago 
On Wed, 23 Jul 2025 10:46:12 -0700
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> Your commit fca8300f68fe3 changed it from __dynamic_array() to __array()
> and __string() seems to be just a special version of __dynamic_array()
> with a length of -1.
> 
> In the commit description you wrote: "Since the size of the name is at
> most 16 bytes (defined by IFNAMSIZ), it is not worth spending the effort
> to determine the size of the string."

So the original had:

	__dynamic_array(char,  name,   IFNAMSIZ )

Which is not dynamic at all. A dynamic_array (like __string) saves the
size in meta data within the event. So basically the above is wasting
bytes to save a fixed size. If you are going to use a dynamic array,
might as well make it dynamic!

I was doing various clean ups back then so I didn't look too deeply
into this event when I made that change. I just saw the obvious waste
of space in the ring buffer.

Just to explain it in more detail. A dynamic_array has in the ring buffer:

	short offset;
	short len;
	[..]
	char  name[len];

That is, 4 bytes are used to know the size of the array and where in
the event it is located. Thus the __dynamic_array() usage basically had:

	short offset;
	short len = IFNAMSIZ;
	[..]
	char name[IFNAMSIZ];

Why have the offset and length? with just __array(char, name, IFNAMSIZ}
it would be just:

	char name[IFNAMSIZ];

See why I changed it?

Now, the change I'm suggesting now would make the __string() be dynamic!

	short offset;
	short len = strlen(res->nh && res->nh->fib_nh_dev ? res->nh->fib_nh_dev->name : "-") + 1;
	[..]
	char name[len];

As IFNAMSIZ is 16, and the above adds 4 bytes to the name, if the name
is less than 7 bytes or less, you save memory on the ring buffer.

	2 bytes: offset
	2 bytes: len;
	7 bytes + '\0'

total: 12 bytes

Note, if there's only one dynamic value, it is always at least 4 bytes aligned.

-- Steve
Re: [PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Thorsten Blum 2 months, 2 weeks ago 
On 23. Jul 2025, at 11:36, Steven Rostedt wrote:
> On Wed, 23 Jul 2025 10:46:12 -0700 Thorsten Blum wrote:
> 
>> Your commit fca8300f68fe3 changed it from __dynamic_array() to __array()
>> and __string() seems to be just a special version of __dynamic_array()
>> with a length of -1.
>> 
>> In the commit description you wrote: "Since the size of the name is at
>> most 16 bytes (defined by IFNAMSIZ), it is not worth spending the effort
>> to determine the size of the string."
> 
> So the original had:
> 
> 	__dynamic_array(char,  name,   IFNAMSIZ )
> 
> Which is not dynamic at all. A dynamic_array (like __string) saves the
> size in meta data within the event. So basically the above is wasting
> bytes to save a fixed size. If you are going to use a dynamic array,
> might as well make it dynamic!
> 
> I was doing various clean ups back then so I didn't look too deeply
> into this event when I made that change. I just saw the obvious waste
> of space in the ring buffer.
> 
> Just to explain it in more detail. A dynamic_array has in the ring buffer:
> 
> 	short offset;
> 	short len;
> 	[..]
> 	char  name[len];
> 
> That is, 4 bytes are used to know the size of the array and where in
> the event it is located. Thus the __dynamic_array() usage basically had:
> 
> 	short offset;
> 	short len = IFNAMSIZ;
> 	[..]
> 	char name[IFNAMSIZ];
> 
> Why have the offset and length? with just __array(char, name, IFNAMSIZ}
> it would be just:
> 
> 	char name[IFNAMSIZ];
> 
> See why I changed it?
> 
> Now, the change I'm suggesting now would make the __string() be dynamic!
> 
> 	short offset;
> 	short len = strlen(res->nh && res->nh->fib_nh_dev ? res->nh->fib_nh_dev->name : "-") + 1;
> 	[..]
> 	char name[len];
> 
> As IFNAMSIZ is 16, and the above adds 4 bytes to the name, if the name
> is less than 7 bytes or less, you save memory on the ring buffer.
> 
> 	2 bytes: offset
> 	2 bytes: len;
> 	7 bytes + '\0'
> 
> total: 12 bytes
> 
> Note, if there's only one dynamic value, it is always at least 4 bytes aligned.

Thanks for the detailed explanation.

I think the better change would be this then:

diff --git a/include/trace/events/fib6.h b/include/trace/events/fib6.h
index 8d22b2e98d48..3f95df1fd155 100644
--- a/include/trace/events/fib6.h
+++ b/include/trace/events/fib6.h
@@ -32,8 +32,9 @@ TRACE_EVENT(fib6_table_lookup,
		__field(        u16,	dport		)
		__field(        u8,	proto		)
		__field(        u8,	rt_type		)
-		__array(		char,	name,	IFNAMSIZ )
-		__array(		__u8,	gw,	16	 )
+		__string(	name,	res->nh && res->nh->fib_nh_dev ?
+					res->nh->fib_nh_dev->name : "-"	)
+		__array(	__u8,	gw,	16	)
	),

	TP_fast_assign(
@@ -64,11 +65,8 @@ TRACE_EVENT(fib6_table_lookup,
			__entry->dport = 0;
		}

-		if (res->nh && res->nh->fib_nh_dev) {
-			strscpy(__entry->name, res->nh->fib_nh_dev->name, IFNAMSIZ);
-		} else {
-			strcpy(__entry->name, "-");
-		}
+		__assign_str(name);
+
		if (res->f6i == net->ipv6.fib6_null_entry) {
			in6 = (struct in6_addr *)__entry->gw;
			*in6 = in6addr_any;
@@ -82,7 +80,7 @@ TRACE_EVENT(fib6_table_lookup,
		  __entry->tb_id, __entry->oif, __entry->iif, __entry->proto,
		  __entry->src, __entry->sport, __entry->dst, __entry->dport,
		  __entry->flowlabel, __entry->tos, __entry->scope,
-		  __entry->flags, __entry->name, __entry->gw, __entry->err)
+		  __entry->flags, __get_str(name), __entry->gw, __entry->err)
);

#endif /* _TRACE_FIB6_H */


I'll submit a v2 if you agree that this is correct.

Thanks,
Thorsten
Re: [PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Steven Rostedt 2 months, 2 weeks ago 
On Wed, 23 Jul 2025 14:15:02 -0700
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> Thanks for the detailed explanation.
> 
> I think the better change would be this then:
> 
> diff --git a/include/trace/events/fib6.h b/include/trace/events/fib6.h
> index 8d22b2e98d48..3f95df1fd155 100644
> --- a/include/trace/events/fib6.h
> +++ b/include/trace/events/fib6.h
> @@ -32,8 +32,9 @@ TRACE_EVENT(fib6_table_lookup,
> 		__field(        u16,	dport		)
> 		__field(        u8,	proto		)
> 		__field(        u8,	rt_type		)
> -		__array(		char,	name,	IFNAMSIZ )
> -		__array(		__u8,	gw,	16	 )
> +		__string(	name,	res->nh && res->nh->fib_nh_dev ?
> +					res->nh->fib_nh_dev->name : "-"	)
> +		__array(	__u8,	gw,	16	)
> 	),
> 
> 	TP_fast_assign(
> @@ -64,11 +65,8 @@ TRACE_EVENT(fib6_table_lookup,
> 			__entry->dport = 0;
> 		}
> 
> -		if (res->nh && res->nh->fib_nh_dev) {
> -			strscpy(__entry->name, res->nh->fib_nh_dev->name, IFNAMSIZ);
> -		} else {
> -			strcpy(__entry->name, "-");
> -		}
> +		__assign_str(name);
> +
> 		if (res->f6i == net->ipv6.fib6_null_entry) {
> 			in6 = (struct in6_addr *)__entry->gw;
> 			*in6 = in6addr_any;
> @@ -82,7 +80,7 @@ TRACE_EVENT(fib6_table_lookup,
> 		  __entry->tb_id, __entry->oif, __entry->iif, __entry->proto,
> 		  __entry->src, __entry->sport, __entry->dst, __entry->dport,
> 		  __entry->flowlabel, __entry->tos, __entry->scope,
> -		  __entry->flags, __entry->name, __entry->gw, __entry->err)
> +		  __entry->flags, __get_str(name), __entry->gw, __entry->err)
> );
> 
> #endif /* _TRACE_FIB6_H */
> 
> 
> I'll submit a v2 if you agree that this is correct.

Isn't the above pretty much what I suggested?

-- Steve
Re: [PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Thorsten Blum 2 months, 2 weeks ago 
On 23. Jul 2025, at 14:21, Steven Rostedt wrote:
> Isn't the above pretty much what I suggested?

Ah sorry, yes it's the same. I'll blame the jet lag from traveling :)

Thanks,
Thorsten
Re: [PATCH net-next] tracing: ipv6: Replace deprecated strcpy() with strscpy()
Posted by Guillaume Nault 2 months, 3 weeks ago 
On Mon, Jul 14, 2025 at 09:54:33AM +0200, Thorsten Blum wrote:
> strcpy() is deprecated; use strscpy() instead.
> 
> Since the destination buffer has a fixed length, strscpy() automatically
> determines its size using sizeof() when the size argument is omitted.
> This makes the explicit size argument unnecessary - remove it.
> 
> Now, combine both if-else branches using strscpy() and the same buffer
> into a single statement to simplify the code.
> 
> No functional changes intended.
> 
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  include/trace/events/fib6.h | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/include/trace/events/fib6.h b/include/trace/events/fib6.h
> index 8d22b2e98d48..903a18836bc6 100644
> --- a/include/trace/events/fib6.h
> +++ b/include/trace/events/fib6.h
> @@ -64,11 +64,9 @@ TRACE_EVENT(fib6_table_lookup,
>  			__entry->dport = 0;
>  		}
>  
> -		if (res->nh && res->nh->fib_nh_dev) {
> -			strscpy(__entry->name, res->nh->fib_nh_dev->name, IFNAMSIZ);
> -		} else {
> -			strcpy(__entry->name, "-");
> -		}
> +		strscpy(__entry->name, res->nh && res->nh->fib_nh_dev ?
> +				       res->nh->fib_nh_dev->name : "-");
> +

Acked-by: Guillaume Nault <gnault@redhat.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.