Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension

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

Eric Dumazet wrote:
> On Thu, Oct 24, 2024 at 4:01 PM Benoît Monin <benoit.monin@gmx.fr> wrote:
> >
> > As documented in skbuff.h, devices with NETIF_F_IPV6_CSUM capability
> > can only checksum TCP and UDP over IPv6 if the IP header does not
> > contains extension.
> >
> > This is enforced for UDP packets emitted from user-space to an IPv6
> > address as they go through ip6_make_skb(), which calls
> > __ip6_append_data() where a check is done on the header size before
> > setting CHECKSUM_PARTIAL.
> >
> > But the introduction of UDP encapsulation with fou6 added a code-path
> > where it is possible to get an skb with a partial UDP checksum and an
> > IPv6 header with extension:
> > * fou6 adds a UDP header with a partial checksum if the inner packet
> > does not contains a valid checksum.
> > * ip6_tunnel adds an IPv6 header with a destination option extension
> > header if encap_limit is non-zero (the default value is 4).
> >
> > The thread linked below describes in more details how to reproduce the
> > problem with GRE-in-UDP tunnel.
> >
> > Add a check on the network header size in skb_csum_hwoffload_help() to
> > make sure no IPv6 packet with extension header is handed to a network
> > device with NETIF_F_IPV6_CSUM capability.
> >
> > Link: https://lore.kernel.org/netdev/26548921.1r3eYUQgxm@benoit.monin/T/#u
> > Fixes: aa3463d65e7b ("fou: Add encap ops for IPv6 tunnels")
> > Signed-off-by: Benoît Monin <benoit.monin@gmx.fr>
> > ---
> > changelog
> > * v2:
> >     - patch against net instead of net-next
> >     - clarify documentation of NETIF_F_IPV6_CSUM
> >     - add link to thread describing the problem
> >     - add fixes tag
> >     - use vlan_get_protocol to check for IPv6
> > * v1:
> >     - https://lore.kernel.org/netdev/0dc0c2af98e96b1df20bd36aeaed4eb4e27d507e.1728056028.git.benoit.monin@gmx.fr/T/#u
> > ---
> >  net/core/dev.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index ea5fbcd133ae..8453e14d301b 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 (vlan_get_protocol(skb) == htons(ETH_P_IPV6) &&
> > +                   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);
> 
> 
> FYI, this patch broke BIG TCP over IPv6.
> 
> [  239.698598] Oops skb_network_header_len()=48 skb->len=67210
> [  239.704122] skb len=67210 headroom=162 headlen=94 tailroom=0
>                mac=(162,14) mac_len=0 net=(176,48) trans=224
>                shinfo(txflags=0 nr_frags=3 gso(size=1428 type=16 segs=47))
>                csum(0x1000e0 start=224 offset=16 ip_summed=3
> complete_sw=0 valid=0 level=0)
>                hash(0xadf29e31 sw=0 l4=1) proto=0x86dd pkttype=0 iif=0
>                priority=0x18020 mark=0x0 alloc_cpu=46 vlan_all=0x0
>                encapsulation=0 inner(proto=0x0000, mac=0, net=0,

I'm looking into the following fix

+++ b/net/core/dev.c
@@ -3642,7 +3642,8 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
 
        if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
                if (vlan_get_protocol(skb) == htons(ETH_P_IPV6) &&
-                   skb_network_header_len(skb) != sizeof(struct ipv6hdr))
+                   skb_network_header_len(skb) != sizeof(struct ipv6hdr) &&
+                   !ipv6_has_hopopt_jumbo(skb))
                        goto sw_checksum;