TCP Fast Open (TFO) [RFC7413] Server-side Implementation

[pkelsey]

Summary

This is an implementation of server-side support for TCP Fast Open.
It supports multiple concurrently valid keys for TFO cookie
generation, and those keys can be generated automatically or manually
installed. The TFO SYN|ACK is sent using the delayed ACK timer in
order give the application time to include response data with the
SYN|ACK. See the top comment in tcp_fastopen.h for other
implementation particulars.

Design diagrams/details are available at https://people.freebsd.org/~pkelsey/TFO_Design_Details.pdf

With a few exceptions, all of the code is enabled/disabled by the
kernel config option TCP_RFC7413, so effectively all of the danger is
voluntary. The exceptions to this are:

• a few bits of code that are clearly dependent upon TF_FASTOPEN in tp->t_flags being set and that to #ifdef would have made more spaghetti than I'd like.

• a few changes that I think are proper outside of the TFO context.

Once this passes review, I intend to MFC it to 10-STABLE shortly
thereafter as it is compiled-out by default.

Test Plan

I have tested this primarily by using homebrew tools built using
Packet Construction Set as well as by using the Linux client support
in the 4.2 kernel via Ubuntu 15.10 (there has been client side support
since kernel 3.6, but in kernels prior to 4.1, the IANA experimental
option code is used and they are thus not compatible with this
implementation).

A rudimentary TFO client (Linux 4.1+ and Mac OS X 10.11+) and server are available at https://people.freebsd.org/~pkelsey/tfo-tools/, as is a utility to install TFO keys on the server.

[pkelsey]

sys/netinet/tcp_input.c

1026 ↗

(On Diff #10676)

This is an instance of new code not protected by TCP_RFC7413 that is clearly dependent on TF_FASTOPEN being set in tp->t_flags in order to have any effect.

2196 ↗

(On Diff #10676)

I believe adding TCPS_SYN_RECEIVED to the exclusion logic here still meets the intent of RFC5961, and is desirable behavior for both TFO and simultaneous open.

2978 ↗

(On Diff #10676)

This is an instance of new code not protected by TCP_RFC7413 that is clearly dependent on TF_FASTOPEN being set in tp->t_flags in order to have any effect.

2999 ↗

(On Diff #10676)

This is an instance of new code not protected by TCP_RFC7413 that is clearly dependent on TF_FASTOPEN being set in tp->t_flags in order to have any effect.

sys/netinet/tcp_timer.c

618 ↗

(On Diff #10676)

This comment change needs to be backed out. Reducing cwnd to 1 segment in the case described in the comment change is accomplished by cc_cong_signal(..., CC_RTO) below, and I *think* for TFO SYN|ACK we want to apply the tp->t_rxtshift == 1 logic as in general data is present.

648 ↗

(On Diff #10676)

I believe this is desirable behavior in the other case where we would be in TCPS_SYNC_RECEIVED here, which is simultaneous open.

sys/netinet/tcp_var.h

337 ↗

(On Diff #10676)

As this struct is kernel-only, I don't believe guards are needed on the TFO-specific members.

[pkelsey]

File contents for tcp_fastopen.[ch] were missing from the initial diff.

[stas]

sys/netinet/tcp_fastopen.c

232 ↗

(On Diff #10678)

Isn't there a race here? Counter can get decremented just after the check and end up being 0. I'd suggest to use atomic_fetchadd_int and check the returned value instead.

[pkelsey]

sys/netinet/tcp_fastopen.c

232 ↗

(On Diff #10678)

I do not think there is a race. This is a reference counter that only gets decremented by a reference owner. If the counter is 1, that means the decrement is being performed by the last reference holder, so there is no possibility of another decrement occurring. If the counter is 1, it is also not possible for another increment to happen as all increments are performed by a reference holder.

As another example, the same principle is in operation in mb_dupcl() in uipc_mbuf.c, where no atomic increment is necessary on the external buffer reference counter if it is 1.

[pkelsey]

sys/netinet/tcp_fastopen.c

81 ↗

(On Diff #10678)

Typo here: Clearnig -> Clearing

86 ↗

(On Diff #10678)

Add note that connections created using a valid TFO SYN that fall back to using non-TFO SYN|ACK due to initial TFO SYN|ACK retransmit timeout will still have TCP_FASTOPEN set when queried.

[pkelsey]

sys/netinet/tcp_input.c

1975 ↗

(On Diff #10678)

This needs to include TH_ACK as well

2442 ↗

(On Diff #10678)

snd_una should be incremented here.

[pkelsey]

Fixed issue with discarding TFO ACKs in tcp_do_segment() and fixed issue with snd_una not being properly advanced when TFO connections transition to ESTABLISHED, also in tcp_do_segment(). Also some minor cleanups (comments, whitespace, etc).

[pkelsey]

Absent any issues raised, I am going to commit this sometime in the next 24-48 hours.

[jch]

I approve this server-side TFO implementation, it is a solid first implementation we can build on.

Note:

• I tried to come a version with less 'goto' but the resulting code was worse
• I tested the TCP stack behavior without TCP_RFC7413 option: All good
• The tests with TCP_RFC7413 option are ongoing