[PATCHv2 net] net: use struct_group to copy addresses

[PATCHv2 net] net: use struct_group to copy addresses

[Hangbin Liu]

kernel test robot reported a warning when build bonding module with
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/bonding/:

                 from ../drivers/net/bonding/bond_main.c:35:
In function ‘fortify_memcpy_chk’,
    inlined from ‘iph_to_flow_copy_v4addrs’ at ../include/net/ip.h:566:2,
    inlined from ‘bond_flow_ip’ at ../drivers/net/bonding/bond_main.c:3984:3:
../include/linux/fortify-string.h:413:25: warning: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of f
ield (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
  413 |                         __read_overflow2_field(q_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘fortify_memcpy_chk’,
    inlined from ‘iph_to_flow_copy_v6addrs’ at ../include/net/ipv6.h:900:2,
    inlined from ‘bond_flow_ip’ at ../drivers/net/bonding/bond_main.c:3994:3:
../include/linux/fortify-string.h:413:25: warning: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of f
ield (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
  413 |                         __read_overflow2_field(q_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is because we try to copy the whole ip/ip6 address to the flow_key,
while we only point the to ip/ip6 saddr. Fix this by using struct_group()
to avoid the compiler warnings/errors.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: c3f8324188fa ("net: Add full IPv6 addresses to flow_keys")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---

v2: use struct_group() instaed of memcpy twice.
---
 include/net/ip.h          | 2 +-
 include/net/ipv6.h        | 2 +-
 include/uapi/linux/ip.h   | 6 ++++--
 include/uapi/linux/ipv6.h | 6 ++++--
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 038097c2a152..144bdfbb25af 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -563,7 +563,7 @@ static inline void iph_to_flow_copy_v4addrs(struct flow_keys *flow,
 	BUILD_BUG_ON(offsetof(typeof(flow->addrs), v4addrs.dst) !=
 		     offsetof(typeof(flow->addrs), v4addrs.src) +
 			      sizeof(flow->addrs.v4addrs.src));
-	memcpy(&flow->addrs.v4addrs, &iph->saddr, sizeof(flow->addrs.v4addrs));
+	memcpy(&flow->addrs.v4addrs, &iph->addrs, sizeof(flow->addrs.v4addrs));
 	flow->control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
 }
 
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 37943ba3a73c..d383c895592a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -897,7 +897,7 @@ static inline void iph_to_flow_copy_v6addrs(struct flow_keys *flow,
 	BUILD_BUG_ON(offsetof(typeof(flow->addrs), v6addrs.dst) !=
 		     offsetof(typeof(flow->addrs), v6addrs.src) +
 		     sizeof(flow->addrs.v6addrs.src));
-	memcpy(&flow->addrs.v6addrs, &iph->saddr, sizeof(flow->addrs.v6addrs));
+	memcpy(&flow->addrs.v6addrs, &iph->addrs, sizeof(flow->addrs.v6addrs));
 	flow->control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
 }
 
diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
index 961ec16a26b8..6f7e833a00f7 100644
--- a/include/uapi/linux/ip.h
+++ b/include/uapi/linux/ip.h
@@ -100,8 +100,10 @@ struct iphdr {
 	__u8	ttl;
 	__u8	protocol;
 	__sum16	check;
-	__be32	saddr;
-	__be32	daddr;
+	struct_group(addrs,
+		__be32	saddr;
+		__be32	daddr;
+	);
 	/*The options start here. */
 };
 
diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index 03cdbe798fe3..3a3a80496c7c 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -130,8 +130,10 @@ struct ipv6hdr {
 	__u8			nexthdr;
 	__u8			hop_limit;
 
-	struct	in6_addr	saddr;
-	struct	in6_addr	daddr;
+	struct_group(addrs,
+		struct	in6_addr	saddr;
+		struct	in6_addr	daddr;
+	);
 };
 
 
-- 
2.38.1

Re: [PATCHv2 net] net: use struct_group to copy addresses

[Jakub Kicinski]

On Mon, 14 Nov 2022 16:12:10 +0800 Hangbin Liu wrote:
> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
> index 961ec16a26b8..6f7e833a00f7 100644
> --- a/include/uapi/linux/ip.h
> +++ b/include/uapi/linux/ip.h
> @@ -100,8 +100,10 @@ struct iphdr {
>  	__u8	ttl;
>  	__u8	protocol;
>  	__sum16	check;
> -	__be32	saddr;
> -	__be32	daddr;
> +	struct_group(addrs,
> +		__be32	saddr;
> +		__be32	daddr;
> +	);
>  	/*The options start here. */
>  };
>  
> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
> index 03cdbe798fe3..3a3a80496c7c 100644
> --- a/include/uapi/linux/ipv6.h
> +++ b/include/uapi/linux/ipv6.h
> @@ -130,8 +130,10 @@ struct ipv6hdr {
>  	__u8			nexthdr;
>  	__u8			hop_limit;
>  
> -	struct	in6_addr	saddr;
> -	struct	in6_addr	daddr;
> +	struct_group(addrs,
> +		struct	in6_addr	saddr;
> +		struct	in6_addr	daddr;
> +	);
>  };
>  

Can you double check the build with clang? It seems to fail with an odd
message, maybe some includes missing?

In file included from ./usr/include/linux/if_tunnel.h:7:
usr/include/linux/ip.h:103:2: error: type name requires a specifier or qualifier
        struct_group(addrs,
        ^
usr/include/linux/ip.h:104:3: error: unexpected type name '__be32': expected identifier
                __be32  saddr;
                ^
usr/include/linux/ip.h:104:10: error: expected ')'
                __be32  saddr;
                        ^
usr/include/linux/ip.h:103:14: note: to match this '('
        struct_group(addrs,
                    ^
usr/include/linux/ip.h:103:15: error: a parameter list without types is only allowed in a function definition
        struct_group(addrs,
                     ^
usr/include/linux/ip.h:106:2: error: type name requires a specifier or qualifier
        );
        ^
usr/include/linux/ip.h:106:2: error: expected member name or ';' after declaration specifiers
usr/include/linux/ip.h:105:16: error: expected ';' at end of declaration list
                __be32  daddr;
                              ^
                              ;

Re: [PATCHv2 net] net: use struct_group to copy addresses

[Hangbin Liu]

On Mon, Nov 14, 2022 at 09:16:45PM -0800, Jakub Kicinski wrote:
> On Mon, 14 Nov 2022 16:12:10 +0800 Hangbin Liu wrote:
> > diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
> > index 961ec16a26b8..6f7e833a00f7 100644
> > --- a/include/uapi/linux/ip.h
> > +++ b/include/uapi/linux/ip.h
> > @@ -100,8 +100,10 @@ struct iphdr {
> >  	__u8	ttl;
> >  	__u8	protocol;
> >  	__sum16	check;
> > -	__be32	saddr;
> > -	__be32	daddr;
> > +	struct_group(addrs,
> > +		__be32	saddr;
> > +		__be32	daddr;
> > +	);
> >  	/*The options start here. */
> >  };
> >  
> > diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
> > index 03cdbe798fe3..3a3a80496c7c 100644
> > --- a/include/uapi/linux/ipv6.h
> > +++ b/include/uapi/linux/ipv6.h
> > @@ -130,8 +130,10 @@ struct ipv6hdr {
> >  	__u8			nexthdr;
> >  	__u8			hop_limit;
> >  
> > -	struct	in6_addr	saddr;
> > -	struct	in6_addr	daddr;
> > +	struct_group(addrs,
> > +		struct	in6_addr	saddr;
> > +		struct	in6_addr	daddr;
> > +	);
> >  };
> >  
> 
> Can you double check the build with clang? It seems to fail with an odd
> message, maybe some includes missing?
> 
> In file included from ./usr/include/linux/if_tunnel.h:7:
> usr/include/linux/ip.h:103:2: error: type name requires a specifier or qualifier
>         struct_group(addrs,
>         ^

Ah, because this is a UAPI header, we need to use __struct_group() here.
I will fix it. Thanks for the info.

Hangbin