| Message ID | 20240405063605.649744-1-michal.swiatkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cd8a34cbc853eaeb4de6789d47a42af102bf3b7a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] pfcp: avoid copy warning by simplifing code | expand |
On Fri, Apr 05, 2024 at 08:36:05AM +0200, Michal Swiatkowski wrote: > >From Arnd comments: > "The memcpy() in the ip_tunnel_info_opts_set() causes > a string.h fortification warning, with at least gcc-13: > > In function 'fortify_memcpy_chk', > inlined from 'ip_tunnel_info_opts_set' at include/net/ip_tunnels.h:619:3, > inlined from 'pfcp_encap_recv' at drivers/net/pfcp.c:84:2: > include/linux/fortify-string.h:553:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > 553 | __write_overflow_field(p_size_field, size);" > > It is a false-positivie caused by ambiguity of the union. > > However, as Arnd noticed, copying here is unescessary. The code can be > simplified to avoid calling ip_tunnel_info_opts_set(), which is doing > copying, setting flags and options_len. > > Set correct flags and options_len directly on tun_info. > > Fixes: 6dd514f48110 ("pfcp: always set pfcp metadata") > Reported-by: Arnd Bergmann <arnd@arndb.de> > Closes: https://lore.kernel.org/netdev/701f8f93-f5fb-408b-822a-37a1d5c424ba@app.fastmail.com/ > Acked-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> I agree that it's nice to avoid a copy. But I do wonder where else this pattern may exist. And if it might be worth introducing a helper for it. Regardless, this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org> ...
On Mon, Apr 08, 2024 at 09:18:29AM +0100, Simon Horman wrote: > On Fri, Apr 05, 2024 at 08:36:05AM +0200, Michal Swiatkowski wrote: > > >From Arnd comments: > > "The memcpy() in the ip_tunnel_info_opts_set() causes > > a string.h fortification warning, with at least gcc-13: > > > > In function 'fortify_memcpy_chk', > > inlined from 'ip_tunnel_info_opts_set' at include/net/ip_tunnels.h:619:3, > > inlined from 'pfcp_encap_recv' at drivers/net/pfcp.c:84:2: > > include/linux/fortify-string.h:553:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > > 553 | __write_overflow_field(p_size_field, size);" > > > > It is a false-positivie caused by ambiguity of the union. > > > > However, as Arnd noticed, copying here is unescessary. The code can be > > simplified to avoid calling ip_tunnel_info_opts_set(), which is doing > > copying, setting flags and options_len. > > > > Set correct flags and options_len directly on tun_info. > > > > Fixes: 6dd514f48110 ("pfcp: always set pfcp metadata") > > Reported-by: Arnd Bergmann <arnd@arndb.de> > > Closes: https://lore.kernel.org/netdev/701f8f93-f5fb-408b-822a-37a1d5c424ba@app.fastmail.com/ > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > I agree that it's nice to avoid a copy. > But I do wonder where else this pattern may exist. > And if it might be worth introducing a helper for it. Right, the same is done in vxlan, ip_gre and ip6_gre at least. I will send v3 with helper. Thanks, Michal > > Regardless, this looks good to me. > > Reviewed-by: Simon Horman <horms@kernel.org> > > ...
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Fri, 5 Apr 2024 08:36:05 +0200 you wrote: > From Arnd comments: > "The memcpy() in the ip_tunnel_info_opts_set() causes > a string.h fortification warning, with at least gcc-13: > > In function 'fortify_memcpy_chk', > inlined from 'ip_tunnel_info_opts_set' at include/net/ip_tunnels.h:619:3, > inlined from 'pfcp_encap_recv' at drivers/net/pfcp.c:84:2: > include/linux/fortify-string.h:553:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > 553 | __write_overflow_field(p_size_field, size);" > > [...] Here is the summary with links: - [net-next,v2] pfcp: avoid copy warning by simplifing code https://git.kernel.org/netdev/net-next/c/cd8a34cbc853 You are awesome, thank you!
From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> Date: Mon, 8 Apr 2024 12:46:42 +0200 > On Mon, Apr 08, 2024 at 09:18:29AM +0100, Simon Horman wrote: >> On Fri, Apr 05, 2024 at 08:36:05AM +0200, Michal Swiatkowski wrote: >>> >From Arnd comments: >>> "The memcpy() in the ip_tunnel_info_opts_set() causes >>> a string.h fortification warning, with at least gcc-13: >>> >>> In function 'fortify_memcpy_chk', >>> inlined from 'ip_tunnel_info_opts_set' at include/net/ip_tunnels.h:619:3, >>> inlined from 'pfcp_encap_recv' at drivers/net/pfcp.c:84:2: >>> include/linux/fortify-string.h:553:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] >>> 553 | __write_overflow_field(p_size_field, size);" >>> >>> It is a false-positivie caused by ambiguity of the union. >>> >>> However, as Arnd noticed, copying here is unescessary. The code can be >>> simplified to avoid calling ip_tunnel_info_opts_set(), which is doing >>> copying, setting flags and options_len. >>> >>> Set correct flags and options_len directly on tun_info. >>> >>> Fixes: 6dd514f48110 ("pfcp: always set pfcp metadata") >>> Reported-by: Arnd Bergmann <arnd@arndb.de> >>> Closes: https://lore.kernel.org/netdev/701f8f93-f5fb-408b-822a-37a1d5c424ba@app.fastmail.com/ >>> Acked-by: Arnd Bergmann <arnd@arndb.de> >>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >> >> I agree that it's nice to avoid a copy. >> But I do wonder where else this pattern may exist. >> And if it might be worth introducing a helper for it. > > Right, the same is done in vxlan, ip_gre and ip6_gre at least. I will > send v3 with helper. Dave applied v2 already, so send this helper as a general improvement w/o "Fixes:" :D > > Thanks, > Michal > >> >> Regardless, this looks good to me. >> >> Reviewed-by: Simon Horman <horms@kernel.org> >> >> ... Thanks, Olek
On Mon, Apr 08, 2024 at 12:50:48PM +0200, Alexander Lobakin wrote: > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Date: Mon, 8 Apr 2024 12:46:42 +0200 > > > On Mon, Apr 08, 2024 at 09:18:29AM +0100, Simon Horman wrote: > >> On Fri, Apr 05, 2024 at 08:36:05AM +0200, Michal Swiatkowski wrote: > >>> >From Arnd comments: > >>> "The memcpy() in the ip_tunnel_info_opts_set() causes > >>> a string.h fortification warning, with at least gcc-13: > >>> > >>> In function 'fortify_memcpy_chk', > >>> inlined from 'ip_tunnel_info_opts_set' at include/net/ip_tunnels.h:619:3, > >>> inlined from 'pfcp_encap_recv' at drivers/net/pfcp.c:84:2: > >>> include/linux/fortify-string.h:553:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > >>> 553 | __write_overflow_field(p_size_field, size);" > >>> > >>> It is a false-positivie caused by ambiguity of the union. > >>> > >>> However, as Arnd noticed, copying here is unescessary. The code can be > >>> simplified to avoid calling ip_tunnel_info_opts_set(), which is doing > >>> copying, setting flags and options_len. > >>> > >>> Set correct flags and options_len directly on tun_info. > >>> > >>> Fixes: 6dd514f48110 ("pfcp: always set pfcp metadata") > >>> Reported-by: Arnd Bergmann <arnd@arndb.de> > >>> Closes: https://lore.kernel.org/netdev/701f8f93-f5fb-408b-822a-37a1d5c424ba@app.fastmail.com/ > >>> Acked-by: Arnd Bergmann <arnd@arndb.de> > >>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > >> > >> I agree that it's nice to avoid a copy. > >> But I do wonder where else this pattern may exist. > >> And if it might be worth introducing a helper for it. > > > > Right, the same is done in vxlan, ip_gre and ip6_gre at least. I will > > send v3 with helper. > > Dave applied v2 already, so send this helper as a general improvement > w/o "Fixes:" :D > I missed that, thanks :) . So, I will send new patch. > > > > Thanks, > > Michal > > > >> > >> Regardless, this looks good to me. > >> > >> Reviewed-by: Simon Horman <horms@kernel.org> > >> > >> ... > > Thanks, > Olek
diff --git a/drivers/net/pfcp.c b/drivers/net/pfcp.c index cc5b28c5f99f..69434fd13f96 100644 --- a/drivers/net/pfcp.c +++ b/drivers/net/pfcp.c @@ -80,9 +80,8 @@ static int pfcp_encap_recv(struct sock *sk, struct sk_buff *skb) else pfcp_node_recv(pfcp, skb, md); - __set_bit(IP_TUNNEL_PFCP_OPT_BIT, flags); - ip_tunnel_info_opts_set(&tun_dst->u.tun_info, md, sizeof(*md), - flags); + __set_bit(IP_TUNNEL_PFCP_OPT_BIT, tun_dst->u.tun_info.key.tun_flags); + tun_dst->u.tun_info.options_len = sizeof(*md); if (unlikely(iptunnel_pull_header(skb, PFCP_HLEN, skb->protocol, !net_eq(sock_net(sk),
From Arnd comments: "The memcpy() in the ip_tunnel_info_opts_set() causes a string.h fortification warning, with at least gcc-13: In function 'fortify_memcpy_chk', inlined from 'ip_tunnel_info_opts_set' at include/net/ip_tunnels.h:619:3, inlined from 'pfcp_encap_recv' at drivers/net/pfcp.c:84:2: include/linux/fortify-string.h:553:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] 553 | __write_overflow_field(p_size_field, size);" It is a false-positivie caused by ambiguity of the union. However, as Arnd noticed, copying here is unescessary. The code can be simplified to avoid calling ip_tunnel_info_opts_set(), which is doing copying, setting flags and options_len. Set correct flags and options_len directly on tun_info. Fixes: 6dd514f48110 ("pfcp: always set pfcp metadata") Reported-by: Arnd Bergmann <arnd@arndb.de> Closes: https://lore.kernel.org/netdev/701f8f93-f5fb-408b-822a-37a1d5c424ba@app.fastmail.com/ Acked-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> --- v1 --> v2: Links: https://lore.kernel.org/netdev/Zg6yMB%2F3w4EBQVDm@mev-dev/ * Add Fixes and Closes tag --- drivers/net/pfcp.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)