linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <670326ed8220a_135479294d1@willemb.c.googlers.com.notmuch>
Date: Sun, 06 Oct 2024 20:10:21 -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>
Cc: netdev@...r.kernel.org, 
 linux-kernel@...r.kernel.org, 
 Benoît Monin <benoit.monin@....fr>
Subject: Re: [PATCH net-next] net: skip offload for NETIF_F_IPV6_CSUM if ipv6
 header contains extension

Benoît Monin wrote:
> Devices with NETIF_F_IP_CSUM capability can checksum TCP and UDP over
> IPv4 with an IP header that may contains options; whereas devices with
> NETIF_F_IPV6_CSUM capability can only checksum TCP and UDP over IPv6 if
> the IP header does not contains extension.

Are both these statements universally true across devices?

I can believe for NETIF_F_IP_CSUM that this is the definition, and
that devices that cannot handle options must fix it up indivually in
ndo_features_check.

And same for NETIF_F_IPV6_CSUM with extension headers.

But it would be good to see where this is asserted in the code, or
examples of drivers that have to perform such actions.

> Enforce that in skb_csum_hwoffload_help by checking the network header
> length in the case where the IP header version is 6. We cannot simply
> rely on the network header length since the IPv4 header can from 20 to
> 60 bytes whereas the IPv6 header must be 40 bytes. So we check the
> version field which is common to IPv4 and IPv6 headers.
> 
> This fixes checksumming errors seen with ip6_tunnel and fou6
> encapsulation, for example with GRE-in-UDP over IPv6:
> * fou6 adds a UDP header with a partial checksum if the inner packet
> does not contains a valid checksum.

Where in the code is this conditional on the inner packet csum?

> * ip6_tunnel adds an IPv6 header with a destination option extension
> header if encap_limit is non-zero (the default value is 4).

If this is a fix, we'll need to target net and best effort find a
suitable fixes tag.

> 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;
>  		switch (skb->csum_offset) {
>  		case offsetof(struct tcphdr, check):
>  		case offsetof(struct udphdr, check):
> @@ -3646,6 +3649,7 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
>  		}
>  	}
> 
> +sw_checksum:
>  	return skb_checksum_help(skb);
>  }
>  EXPORT_SYMBOL(skb_csum_hwoffload_help);