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

Authored by pkelsey on Dec 2 2015, 7:44 PM.

Details

Reviewers

stas
hiren
rrs
glebius
jch
lstewart
gnn

Commits

rS292706: Implementation of server-side TCP Fast Open (TFO) [RFC7413].

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.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

pkelsey updated this revision to Diff 10676.Dec 2 2015, 7:44 PM

pkelsey retitled this revision from to TCP Fast Open (TFO) [RFC7413] Server-side Implementation.

pkelsey updated this object.

pkelsey edited the test plan for this revision. (Show Details)

pkelsey added reviewers: glebius, hiren, jch, lstewart.

Herald added a subscriber: imp. · View Herald TranscriptDec 2 2015, 7:44 PM

pkelsey added inline comments.Dec 2 2015, 8:00 PM

sys/netinet/tcp_input.c
1026	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	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	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	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	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	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	As this struct is kernel-only, I don't believe guards are needed on the TFO-specific members.

gnn added a reviewer: gnn.Dec 2 2015, 8:06 PM

pkelsey added a reviewer: rrs.Dec 2 2015, 8:06 PM

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

stas added a subscriber: stas.Dec 2 2015, 10:13 PM

stas added inline comments.

sys/netinet/tcp_fastopen.c
232	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 added inline comments.Dec 3 2015, 12:53 AM

sys/netinet/tcp_fastopen.c
232	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 added inline comments.Dec 3 2015, 2:31 AM

sys/netinet/tcp_fastopen.c
81	Typo here: Clearnig -> Clearing
86	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.

florent_guiliani.fr added a subscriber: florent_guiliani.fr.Dec 3 2015, 3:50 PM

jtl added a subscriber: transport.Dec 3 2015, 5:14 PM

pkelsey added inline comments.Dec 9 2015, 7:31 PM

sys/netinet/tcp_input.c
1975	This needs to include TH_ACK as well
2442	snd_una should be incremented here.

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 marked 7 inline comments as done.Dec 9 2015, 7:45 PM

pkelsey updated this object.Dec 17 2015, 5:58 PM

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

gnn accepted this revision.Dec 23 2015, 1:18 AM

gnn edited edge metadata.

This revision is now accepted and ready to land.Dec 23 2015, 1:18 AM

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

Note: