Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
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>
In-Reply-To: <CAF=yD-+1RSj_o8n5LDOLVyn_dvVQvmDQo5pacSoDFPOR3M2g5g@mail.gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Tue, 15 Feb 2022 07:35:10 -0800
Message-ID: <CANn89i+T=Ny7pfUomSsa1ub77u8LfYtRZPzmp_0-=oWKt0abLg@mail.gmail.com>
Subject: Re: [PATCH] gso: do not skip outer ip header in case of ipip and net_failover
To:     Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc:     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"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Feb 15, 2022 at 7:01 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, Feb 14, 2022 at 8:38 PM Tao Liu <thomas.liu@ucloud.cn> wrote:
> >
> > Sorry to resend it.
> >
> > 2022=E5=B9=B42=E6=9C=8814=E6=97=A5 12:27=EF=BC=8CWillem de Bruijn <will=
emdebruijn.kernel@gmail.com> =E5=86=99=E9=81=93=EF=BC=9A
> >
> > On Sun, Feb 13, 2022 at 11:03 PM Tao Liu <thomas.liu@ucloud.cn> wrote:
> >
> >
> > Sorry for bothering, just repost it.
> >
> > 2022=E5=B9=B42=E6=9C=8814=E6=97=A5 09:28=EF=BC=8CWillem de Bruijn <will=
emdebruijn.kernel@gmail.com> =E5=86=99=E9=81=93=EF=BC=9A
> >
> > On Sun, Feb 13, 2022 at 10:10 AM Tao Liu <thomas.liu@ucloud.cn> wrote:
> >
> >
> > We encouter a tcp drop issue in our cloud environment. Packet GROed in =
host
> > forwards to a VM virtio_net nic with net_failover enabled. VM acts as a
> > IPVS LB with ipip encapsulation. The full path like:
> > host gro -> vm virtio_net rx -> net_failover rx -> ipvs fullnat
> > -> ipip encap -> net_failover tx -> virtio_net tx
> >
> > When net_failover transmits a ipip pkt (gso_type =3D 0x0103), there is =
no gso
> > performed because it supports TSO and GSO_IPXIP4. But network_header ha=
s
> > been pointing to inner ip header.
> >
> >
> > If the packet is configured correctly, and net_failover advertises
> > that it can handle TSO packets with IPIP encap, then still virtio_net
> > should not advertise it and software GSO be applied on its
> > dev_queue_xmit call.
> >
> > This is assuming that the packet not only has SKB_GSO_IPXIP4 correctly
> > set, but also tunneling fields like skb->encapsulated and
> > skb_inner_network_header.
> >
> > Thanks very much for your comment!
> >
> > Yes, the packet is correct. Another thing i have not pointed directly i=
s
> > that the pkt has SKB_GSO_DODGY. net_failover do not advertises GSO_ROBU=
ST
> > but virtio_net do.
> >
> >
> > If net_failover does not advertise NETIF_F_GSO_ROBUST, then
> > tcp_gso_segment will pass a packet with SKB_GSO_DODGY to the
> > software gso stack, not taking the branch
> >
> >        if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
> >
> > As i tested, packet with SKB_GSO_DODGY hits this branch. packet's gso_t=
ype=3D0x0103, which
> > means SKB_GSO_TCPV4, SKB_GSO_DODGY and SKB_GSO_IPXIP4. net_failover mat=
ches
> > the condition.
> >
> > Consequently, tcp_gso_segment returns NULL, there is no software gso di=
d here. And
> > network_header points to inner iph.
> >
> > Software gso is did by virtio_net which not advertises NETIF_F_GSO_IPXI=
P4. It skips the outer
> > iph, and keeps it unchanged.
> >
> > ---
> > net/ipv4/af_inet.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> > index 9c465ba..f8b3f8a 100644
> > --- a/net/ipv4/af_inet.c
> > +++ b/net/ipv4/af_inet.c
> > @@ -1425,10 +1425,18 @@ struct sk_buff *inet_gso_segment(struct sk_buff=
 *skb,
> > static struct sk_buff *ipip_gso_segment(struct sk_buff *skb,
> >                                       netdev_features_t features)
> > {
> > +       struct sk_buff *segs;
> > +       int nhoff;
> > +
> >       if (!(skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP4))
> >               return ERR_PTR(-EINVAL);
> >
> > -       return inet_gso_segment(skb, features);
> > +       nhoff =3D skb_network_header(skb) - skb_mac_header(skb);
> > +       segs =3D inet_gso_segment(skb, features);
> > +       if (!segs)
> > +               skb->network_header =3D skb_mac_header(skb) + nhoff - s=
kb->head;
> > +
> > +       return segs;
> > }
> >
> >
> > If this would be needed for IPIP, then the same would be needed for SIT=
, etc.
> >
> > Is the skb_network_header
> >
> > 1. correctly pointing to the outer header of the TSO packet before the
> > call to inet_gso_segment
> > 2. incorrectly pointing to the inner header of the (still) TSO packet
> > after the call to inet_gso_segment
> >
> > inet_gso_segment already does the same operation: save nhoff, pull
> > network header, call callbacks.gso_segment (which can be
> > ipip_gso_segment->inet_gso_segment), then place the network header
> > back at nhoff.
> >
> > values print in skb_mac_gso_segment() before callbacks.gso_segment:
> > ipip:               vlan_depth=3D0 mac_len=3D0 skb->network_header=3D20=
6
> > net_failover:  vlan_depth=3D14 mac_len=3D14 skb->network_header=3D186
> > virtio_net:      vlan_depth=3D34 mac_len=3D34 skb->network_header=3D206
> >
> > agree to add sit/ip4ip6/ip6ip6, and patch can be simplified as:
> >
> >
> > If IPIP GSO was so broken, I think we would have found it long before.
> >
> > As said, inet_gso_segment should already do the right thing for ipip:
> > it will be called twice.
> >
> >
> > SKB_GSO_DODGY flag and net_failover conduct this issue. local traffic j=
ust works fine.
>
> 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 =3D skb_network_header(skb) - skb_mac_header(skb);
>
>         [...]
>
>         if (likely(ops && ops->callbacks.gso_segment))
>                 segs =3D ops->callbacks.gso_segment(skb, features);
>
> And resets that after for each skb in segs.
>
>         skb =3D segs;
>         do {
>                 [...]
>                 skb->network_header =3D (u8 *)iph - skb->head;
>
> But does not do this if segs =3D=3D 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.