netdev - Re: [PATCH net-next 0/4] net: cleanup for UDP tunnel's GRO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <643a68e2-b3d8-a21b-79ff-9ba7b7c9f418@stressinduktion.org>
Date:	Fri, 8 Jul 2016 17:51:20 -0400
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Alexander Duyck <alexander.duyck@...il.com>
Cc:	Shmulik Ladkani <shmulik.ladkani@...il.com>,
	Paolo Abeni <pabeni@...hat.com>,
	Netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Jesse Gross <jesse@...nel.org>,
	Tom Herbert <tom@...bertland.com>, Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH net-next 0/4] net: cleanup for UDP tunnel's GRO

On 08.07.2016 17:27, Alexander Duyck wrote:
> On Fri, Jul 8, 2016 at 1:57 PM, Hannes Frederic Sowa
> <hannes@...essinduktion.org> wrote:
>> On 08.07.2016 16:17, Shmulik Ladkani wrote:
>>> On Fri, 8 Jul 2016 09:21:40 -0700 Alexander Duyck <alexander.duyck@...il.com> wrote:
>>>> On Thu, Jul 7, 2016 at 8:58 AM, Paolo Abeni <pabeni@...hat.com> wrote:
>>>>> With udp tunnel offload in place, the kernel can do GRO for some udp tunnels
>>>>> at the ingress device level. Currently both the geneve and the vxlan drivers
>>>>> implement an additional GRO aggregation point via gro_cells.
>>>>> The latter takes effect for tunnels using zero checksum udp packets, which are
>>>>> currently explicitly not aggregated by the udp offload layer.
>>>>>
>>>>> This patch series adapts the udp tunnel offload to process also zero checksum
>>>>> udp packets, if the tunnel's socket allow it. Aggregation, if possible is always
>>>>> performed at the ingress device level.
>>>>>
>>>>> Then the gro_cells hooks, in both vxlan and geneve driver are removed.
>>>>
>>>> I think removing the gro_cells hooks may be taking things one step too far.
>>>
>>> +1
>>>
>>>> I get that there is an impression that it is redundant but there are a
>>>> number of paths that could lead to VXLAN or GENEVE frames being
>>>> received that are not aggregated via GRO.
>>>
>>> There's the case where the vxlan/geneve datagrams get IP fragmented, and
>>> IP frags are not GROed.
>>> GRO aggregation at the vxlan/geneve level is beneficial for this case.
>>
>> Isn't this a misconfiguration? TCP should not fragment at all, not even
>> in vxlan/geneve if one cares about performance? And UDP is not GRO'ed
>> anyway.
> 
> The problem is that the DF bit of the outer header is what matters,
> not the inner one.  I believe the default for many of the UDP tunnels
> is to not set the DF bit on the outer header.  The fact is
> fragmentation shouldn't happen, but it can and we need to keep that in
> mind when we work on these sort of things.

"Old" tunnel protocols inherit the outer DF bit from the inner one,
geneve and vxlan do not. I think we simply never should set DF bit
because vxlan pmtu with source port rss perturbation breaks pmtu
discovery anyway. On the other hand doing so and not having end-to-end
protection of the VNI because of proposed 0 checksum is also...
otherwise at least the Ethernet FCS could protect the frame.

> There have been a number of cases in the past with tunnels where "it
> works for me" has been how things have been implemented and we need to
> avoid that as it creates a significant amount of pain for end users
> when they do things and they don't work because they have strayed off
> of the one use case that has been tested.

We certainly don't want to break fragmentation with vxlan and this patch
doesn't change so.

I really do wonder if GRO on top of fragmentation does have any effect.
Would be great if someone has data for that already?

Bye,
Hannes