netdev - Re: [net PATCH v2 2/2] ipv4/GRO: Make GRO conform to RFC 6864

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:	Tue, 5 Apr 2016 09:45:51 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	Alexander Duyck <aduyck@...antis.com>,
	Tom Herbert <tom@...bertland.com>,
	Jesse Gross <jesse@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Netdev <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [net PATCH v2 2/2] ipv4/GRO: Make GRO conform to RFC 6864

On Tue, Apr 5, 2016 at 9:30 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Tue, 2016-04-05 at 08:52 -0700, Alexander Duyck wrote:
>
>>
>> I disagree I think it will have to be part of the default
>> configuration.  The problem is the IP ID is quickly becoming
>> meaningless.  When you consider that a 40Gb/s link can wrap the IP ID
>> value nearly 50 times a second using a 1500 MTU the IP ID field should
>> just be ignored anyway because you cannot guarantee that it will be
>> unique without limiting the Tx window size.  That was the whole point
>> of RFC6864.  Basically the IP ID field is so small that as we push
>> into the higher speeds you cannot guarantee that the field will have
>> any meaning so for any case where you don't need to use it you
>> shouldn't because it will likely not provide enough useful data.
>
> Just because a few flows reach 40Gbit , we should remind that vast
> majority of the Internet runs with < 50Mbits flows.
>
> I prefer the argument of IPv6 not having ID ;)

Okay, maybe I'll try to use that argument more often then.. :-)

> We should do our best to keep interoperability, this is the selling
> point.
>
> And quite frankly your last patch makes perfect sense to me :

Yes.  It was a compromise, though I might still have to go through and
refine it more.  It might make sense for the IP header associated with
the TCP flow, but for outer headers it actually is worse because we
end up blocking several different possibilities.  What I might need to
do is capture the state of the DF bit as we work a flow up through the
stack and once it is in the list of GRO SKBs use that DF bit as a flag
to indicate if we support a incrementing or fixed pattern for the
values.  That way tunnels can optionally ignore the IP ID if the DF
bit is set since their values may not be as clean as that of TCP.

> The aggregation is done only if the TCP headers of consecutive packets
> matches. So who cares of IPv4 ID really ?
> This is a very minor detail. The possible gains outperform the
> theoretical 'problem'
>
> GRO already reorder flows, it never had a guarantee of being 'ínvisible'
> as Herbert claims.

I can see what he is trying to get at.  I just think it is a bit too
strict on the interpretation of what values have to be maintained.  My
plan going forward is to add a sysctl that will probably allow us some
wiggle room in regards to IP ID for GRO and GSO so that when it is
disabled we will not perform GSO partial nor allow for repeating IP ID
in GRO on devices that cannot get the IP ID right.

- Alex