netdev - Re: [PATCH net-next] net: skip offload for NETIF_F_IPV6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6716d00429a9f_6fc77294c4@willemb.c.googlers.com.notmuch>
Date: Mon, 21 Oct 2024 18:04:52 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Benoît Monin <benoit.monin@....fr>, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Jiri Pirko <jiri@...nulli.us>, 
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>, 
 Lorenzo Bianconi <lorenzo@...nel.org>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 netdev@...r.kernel.org
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: skip offload for NETIF_F_IPV6_CSUM if ipv6
 header contains extension

Benoît Monin wrote:
> 10/10/2024 Benoît Monin wrote:
> > 07/10/2024 Willem de Bruijn wrote:
> > > Benoît Monin wrote:
> > > > 07/10/2024 Willem de Bruijn wrote :
> > > > > Benoît Monin wrote:
> [...]
> > > > > > Signed-off-by: Benoît Monin <benoit.monin@....fr>
> > > > > > ---
> > > > > >  net/core/dev.c | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > > 
> > > > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > > > index ea5fbcd133ae..199831d86ec1 100644
> > > > > > --- a/net/core/dev.c
> > > > > > +++ b/net/core/dev.c
> > > > > > @@ -3639,6 +3639,9 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> > > > > >  		return 0;
> > > > > > 
> > > > > >  	if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
> > > > > > +		if (ip_hdr(skb)->version == 6 &&
> > > > > > +		    skb_network_header_len(skb) != sizeof(struct ipv6hdr))
> > > > > > +			goto sw_checksum;
> > > 
> > > This check depends on skb->transport_header and skb->network_header
> > > being set. This is likely true for all CHECKSUM_PARTIAL packets that
> > > originate in the local stack. As well as for the injected packets and
> > > forwarded packets, as far as I see, so Ack.
> > > 
> > > Access to the network header at this point likely requires
> > > skb_header_pointer, however. As also used in qdisc_pkt_len_init called
> > > from the same __dev_queue_xmit_nit.
> > > 
> > > Perhaps this test should be in can_checksum_protocol, which already
> > > checks that the packet is IPv6 when testing NETIF_F_IPV6_CSUM.
> > > 
> > You're right, moving this to can_checksum_protocol() makes more sense. I will 
> > do that, retest and post a new version of the patch.
> > 
> Looking more into it, can_checksum_protocol() is called from multiple places 
> where network header length cannot easily extracted, in particular from 
> vxlan_features_check().
> 
> How about keeping the length check in skb_csum_hwoffload_help() but using 
> vlan_get_protocol() to check for IPv6 instead of ip_hdr(skb)->version?

Yes, both sound good to me.