Re: [PATCH net] net: mask NETIF_F_IPV6_CSUM flag on irregular packet header size

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Willem de Bruijn wrote:
> Jakub Ramaseuski wrote:
> > Throughput with GRE on IPv6 drops to 0 on NICs that use ice/bnxt_en
> > or any driver with NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM set and with
> > NETIF_F_HW_CSUM unset, see following dmesg output for more info.
> 
> HW_CSUM is generally not advertised if IP_CSUM and IPV6_CSUM are.
> HW_CSUM is a superset, and generally advisable. But some devices
> only support specific protocols..
> 
> The pertinent comment here is that SKB_GSO_TCPV6 cannot be segmented
> if NETIF_F_IPV6_CSUM is set

and the IPv6 packet has extension headers, I mean to add.

V> And the same for SKB_GSO_UDP_L4 if ETH_P_IPV6.
> 
> > Mask NETIF_F_IPV6_CSUM in gso_features_check if the IPv6 header contains
> > extension headers. This flag indicates that the network interface
> > is capable of computing the checksum only for plain IPv6 headers
> > without any extension headers.
> > 
> > The exception is a BIG TCP extension, which, as stated in 68e068cabd2c6c53:
> > "The feature is only enabled on devices that support BIG TCP TSO.
> > The header is only present for PF_PACKET taps like tcpdump,
> > and not transmitted by physical devices."
> > 
> > Fixes: 04c20a9356f283da ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension")
> 
> That fix is still a valid fix to fall back onto software checksum
> offload for non GSO packets.
> 
> This fix addresses the same for GSO packets.
> 
> That fix is not wrong, and still needed (this patch also does not modify or remove its code).

The question is what the right tag.

Your code also shows a tunnel, but not the one implicated on the above patch:

    Fixes: aa3463d65e7b ("fou: Add encap ops for IPv6 tunnels")

But conceivably this is not tunnel related at all, if any TSO6 traffic can
cause IPv6 extension headers to be present.

Minor: when respinning please put this brief explanation first, and
the log output (stack trace, skb_dump), second.
 
> > Reported-by: Tianhao Zhao <tizhao@redhat.com>
> > Suggested-by: Michal Schmidt <mschmidt@redhat.com>
> > Signed-off-by: Jakub Ramaseuski <jramaseu@redhat.com>
> > ---
> > ---
> >  net/core/dev.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index b28ce68830b2b..118c433c2cb9b 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3778,6 +3778,10 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
> >  		if (!(iph->frag_off & htons(IP_DF)))
> >  			features &= ~NETIF_F_TSO_MANGLEID;
> >  	}
> > +	if (vlan_get_protocol(skb) == htons(ETH_P_IPV6) &&
> > +		skb_network_header_len(skb) != sizeof(struct ipv6hdr) &&
> > +		!ipv6_has_hopopt_jumbo(skb))
> > +		features &= ~NETIF_F_IPV6_CSUM;
> 
> Exit the branch as soon as possible. Avoid the Ethernet header lookup
> if possible.
> 
> And unfortunately transport header cannot be guaranted with
> virtio_net_hdr_to_skb.
> 
>          /* NETIF_F_IPV6_CSUM does not support IPv6 extension headers,
>           * so neither does TSO that depends on it.
>           */
>          if (features & NETIF_F_IPV6_CSUM &&
>              (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6 ||
>               (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 &&
> 	       vlan_get_protocol(skb) == htons(ETH_P_IPV6))) &&
>               skb_transport_header_was_set(skb) &&
>               skb_network_header_len(skb) != sizeof(struct ipv6hdr) &&
>               !ipv6_has_hopopt_jumbo(skb))
>                  features &= ~(NETIF_F_IPV6_CSUM | NETIF_F_TSO6 | NETIF_F_GSO_UDP_L4);
> 
> 
> >  	return features;
> >  }
> > -- 
> > 2.50.1
> > 
> 
>