[PATCH net] net: fix the address copy size to flow_keys

[PATCH net] net: fix the address copy size to flow_keys

[Hangbin Liu]

kernel test robot reported a warning when build bonding module:

                 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 memcpy the whole ip/ip6 address to the flow_key,
while we only point the to ip/ip6 saddr. It is efficient since we only need
to do copy once for both saddr and daddr. But to fix the build warning,
let's break the memcpy to 2 parts. This may affect bonding's performance
slightly, but shouldn't too much.

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>
---
 include/net/ip.h   | 3 ++-
 include/net/ipv6.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 038097c2a152..8ab4cea47ceb 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -563,7 +563,8 @@ 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.src, &iph->saddr, sizeof(flow->addrs.v4addrs.src));
+	memcpy(&flow->addrs.v4addrs.dst, &iph->daddr, sizeof(flow->addrs.v4addrs.dst));
 	flow->control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
 }
 
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 37943ba3a73c..f6ff7d30ca49 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -897,7 +897,8 @@ 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.src, &iph->saddr, sizeof(flow->addrs.v6addrs.src));
+	memcpy(&flow->addrs.v6addrs.dst, &iph->daddr, sizeof(flow->addrs.v6addrs.dst));
 	flow->control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
 }
 
-- 
2.38.1

Re: [PATCH net] net: fix the address copy size to flow_keys

[Paolo Abeni]

Hello,

On Wed, 2022-11-09 at 10:44 +0800, Hangbin Liu wrote:
> kernel test robot reported a warning when build bonding module:
> 
>                  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 memcpy the whole ip/ip6 address to the flow_key,
> while we only point the to ip/ip6 saddr. It is efficient since we only need
> to do copy once for both saddr and daddr. But to fix the build warning,
> let's break the memcpy to 2 parts. This may affect bonding's performance
> slightly, but shouldn't too much.

The compiler should be able to unroll the memcpy loop in both cases,
but if it fails to do that, the performance hit could be measurable,
see commit 236222d39347e0e486010f10c1493e83dbbdfba8.

This looks like a straight struct_group() use-case, I suggest to use
the latter construct instead.

Cheers,

Paolo