[gnutls-devel] [PATCH] Re: TCP Fast Open

[Nikos Mavrogiannopoulos]

On Fri, Jul 15, 2016 at 12:35 PM, Tim Ruehsen <tim.ruehsen at gmx.de> wrote:
> On Friday, July 15, 2016 9:21:03 AM CEST Nikos Mavrogiannopoulos wrote:
>> On Thu, Jul 14, 2016 at 12:45 PM, Tim Ruehsen <tim.ruehsen at gmx.de> wrote:
>> > Here is my patch, first version.
>> >
>> > Please review and comment.
>> >
>> > - I have no platform without TFO to test with... so compilation might
>> > break on such platforms, could anyone give it a try ?
>>
>> I've committed it on a special branch and run through the CI. The
>> windows and freebsd builds fail:
>> https://gitlab.com/gnutls/gnutls/builds/2370926
>> https://gitlab.com/gnutls/gnutls/builds/2370929
> I haven't taken care of HAVE_WRITEV in _gnutls_writev(). The amended patch
> should do that. Please test again.
Results will be at:
https://gitlab.com/gnutls/gnutls/pipelines/3717100

>> > - To check the timings / test TFO, I added 'return 0;' in line 1245 of
>> > src/
>> > cli.c and run it with and without --fastopen against www.google.com 443.
>> > I have ~35ms ping here and had a reproducable gain of ~32ms when using --
>> > fastopen (152ms vs. 120ms).
>> That's quite nice. Have you noticed firewalls or middle boxes blocking
>> that approach? Is there anything we can fix for them or fallback?
> Not that I know of. A fallback should IMO applied at the application layer.

Makes sense.

> But there is one thing that should be mentioned. The client applications
> (currently only gnutls-cli) need a different retry strategy with TFO.
> Without TFO, socket.c/socket_open() loops over the addrinfo list returned by
> getaddrinfo() until connect() returns success.
> With TFO, we return with the first successful call to socket(). The implicit
> connect happens later - and from there we currently can't easily 'continue
> looping'. Of course we could (we still have 'res' and 'ptr' in the socket_st),
> but that needs some refactoring of socket.c - not part of this patch.

That is going to create some trouble when testing this option with gnutls-cli.

>> This path is a bit worrying. Why not have
>> gnutls_transport_set_fastopen() replace all the pull/push functions
>> with its own version?
> True, but the new writev_tfo functions need a session pointer instead of a
> transport pointer. And I didn't want to take care for user supplied transport
> pointers + user defined write functions (set before or after
> gnutls_transport_set_fastopen()).
> But I see room for a internal refactoring, that uses session for all system
> write/writev funtions... and call user supplied functions from there. IMO,
> that should be another patch.

Would you be interested in making that patch? That would really
simplify any such additions in the future.

>> A test program utilizing this code path should also be added to avoid
>> breaking that functionality in the future.
> It needs client+server connected via TCP to test that code path. Can you
> propose any test that is best to start with ?

The simplest to modify for that, I think is tls-client-with-seccomp.c.
It uses a sockpair with AF_UNIX, which will have to be replaced with a
real socket connection, or a socketpair emulation such as:
https://github.com/ncm/selectable-socketpair/blob/master/socketpair.c

regards,
Nikos