Re: [PATCH] net: fix wrong network header length

From: Steffen Klassert <steffen.klassert@secunet.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: "Maciej Żenczykowski" <maze@google.com>,
	"Lina Wang" <lina.wang@mediatek.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Linux NetDev" <netdev@vger.kernel.org>,
	"Kernel hackers" <linux-kernel@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>, "Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@kernel.org>,
	"Willem Bruijn" <willemb@google.com>,
	"Eric Dumazet" <edumazet@google.com>
Subject: Re: [PATCH] net: fix wrong network header length
Date: Thu, 10 Feb 2022 08:49:31 +0100	[thread overview]
Message-ID: <20220210074931.GB1223722@gauss3.secunet.de> (raw)
In-Reply-To: <a62abfeb0c06bf8be7f4fa271e2bcdef9d86c550.camel@redhat.com>

On Tue, Feb 08, 2022 at 05:01:07PM +0100, Paolo Abeni wrote:
> + Steffen
> On Tue, 2022-02-08 at 04:57 -0800, Maciej Żenczykowski wrote:
> > > 
> > > If traversing the segments become too costly, you can try replacing
> > > GRO_FRAGLIST with GRO_UDP_FWD.
> > 
> > Yeah, I don't know...
> > 
> > I've considered that we could perhaps fix the 6to4 helper, and 4to6 helper...
> > but then I think every *other* helper / code path that plays games
> > with the packet header needs fixing as well,
> > ie. everything dealing with encap/decap, vlan, etc..
> > 
> > At that point it seems to me like it's worth fixing here rather than
> > in all those other places.
> > 
> > In general it seems gro fraglist as implemented is just a bad idea...
> > Packets (and things we treat like packets) really should only have 1 header.
> > GRO fraglist - as implemented - violates this pretty fundamental assumption.
> > As such it seems to be on the gro fraglist implementation to deal with it.
> > That to me seems to mean it should be fixed here, and not elsewhere.
> 
> @Steffen: IIRC GRO_FRAGLIST was originally added to support some
> forwarding scenarios. Now we have GRO_UDP_FWD which should be quite
> comparable. I'm wondering if the latter feature addresses your use
> case, too.

The advantage of GRO_FRAGLIST for forwarding is that GRO and GSO
happen with almost no overhead, because the packets are left in
the skbs we received them and are not mangled during processing.

So if there is no hardware segmentation support, GRO_FRAGLIST is
still much faster than GRO_UDP_FWD.

> If so, could we consider deprecating (and in a longer run, drop) the
> GRO_FRAGLIST feature? 

Maybe we can make it exclusive for forwarding or bring the header
processing a bit closer to GRO_UDP_FWD, but I'd like to keep that
feature.