Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220213150234.31602-1-thomas.liu@ucloud.cn> <CA+FuTSdODATw3hSAMv9aZUmJNM8ZE-YP58pr17bO9rGJUgfegw@mail.gmail.com>
 <CFD9B65A-6762-4D9B-ADEB-B4C0B1902E02@ucloud.cn> <CA+FuTSfQOUEyEDnOU8VVZ=STw_ii-hTwyg-cvpcViPkVK4pLUA@mail.gmail.com>
 <42554FCB-9180-4B32-B5CF-6D3236237D99@ucloud.cn> <CAF=yD-+1RSj_o8n5LDOLVyn_dvVQvmDQo5pacSoDFPOR3M2g5g@mail.gmail.com>
 <CANn89i+T=Ny7pfUomSsa1ub77u8LfYtRZPzmp_0-=oWKt0abLg@mail.gmail.com>
In-Reply-To: <CANn89i+T=Ny7pfUomSsa1ub77u8LfYtRZPzmp_0-=oWKt0abLg@mail.gmail.com>
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date:   Tue, 15 Feb 2022 10:45:24 -0500
Message-ID: <CA+FuTSc9ZeuLE7tqNT-GnqHb27SE7UAtVRVsZHR+dV6ua=UKPA@mail.gmail.com>
Subject: Re: [PATCH] gso: do not skip outer ip header in case of ipip and net_failover
To:     Eric Dumazet <edumazet@google.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
        Tao Liu <thomas.liu@ucloud.cn>,
        David Miller <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        David Ahern <dsahern@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        "Samudrala, Sridhar" <sridhar.samudrala@intel.com>,
        Network Development <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > Got it. That is an uncommon combination. SKB_GSO_DODGY is set from
> > external virtio_net, which does not support tunnels. But a path with
> > an added tunnel might cause this combination.
> >
> > And inet_gso_segment resets the network header, both times, before
> > calling callbacks.gso_segment()
> >
> >         skb_reset_network_header(skb);
> >         nhoff = skb_network_header(skb) - skb_mac_header(skb);
> >
> >         [...]
> >
> >         if (likely(ops && ops->callbacks.gso_segment))
> >                 segs = ops->callbacks.gso_segment(skb, features);
> >
> > And resets that after for each skb in segs.
> >
> >         skb = segs;
> >         do {
> >                 [...]
> >                 skb->network_header = (u8 *)iph - skb->head;
> >
> > But does not do this if segs == NULL.
> >
> > The packet has to be restored before it is passed to the device. I
> > think we have to handle this case correctly in inet_gso_segment,
> > instead of patching it up in all the various tunnel devices.
> >
> > The same holds for ipv6_gso_segment.
>
> Back in the days, GRO was modified so that we passed a context (nhoff)
> in called functions,
> instead of changing skb offsets. The concept of outer/inner header
> only works with 1 encap.
>
> Perhaps it is time to do the same in GSO, to allow arbitrary levels of
> encapsulation.
> Then we no longer mess with these limited
> 'network_header/inner_network_header' fields
> in the skb.
>
> Stuffing state in the skb has been a mistake I think.

If we could unwind those skb inner_* fields (and reclaim the skbuff
space!) that would be fantastic.

Immediately for this bug: perhaps it can be fixed by resetting the
network_header on the gso skb if segs == NULL. As the offset is stored
on the stack.