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]

Date:	Fri, 8 Jul 2016 15:11:00 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
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 Fri, Jul 8, 2016 at 2:51 PM, Hannes Frederic Sowa
<hannes@...essinduktion.org> wrote:
> 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.

The "Old" tunnel protocols can be configured to behave the same way as
well.  It is just that most of them default to a mode where they
support getting a path MTU if I recall correctly.

>> 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?

I think that logic is kind of backwards.  It is already there.
Instead of asking people to prove that this change is invalid the onus
should be on the submitter to prove the change causes no harm.

The whole argument is kind of moot anyway based on the comment from
Tom.  This is based around being able to aggregate frames with a zero
checksum which we can already do, but it requires that the hardware
has validated the inner checksum.  What this patch set is doing is
trying to take frames that have no checksum and force us to perform a
checksum for the inner headers in software.  Really we don't do that
now for general GRO, why should we do it in the case of tunnels?  Also
I think there is a test escape for the case of a frame with an outer
checksum that was not validated by hardware.  In that case the
checksum isn't converted until you hit the UDP handler which will then
convert it to checksum complete and it then would rely on the GRO
cells to merge the frames after we have already validated the checksum
for the outer header.

- Alex