Page MenuHomeFreeBSD

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

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

Details

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

2194

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.

2982

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.

3003

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
619

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.

647

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
pkelsey updated this revision to Diff 10678.Dec 2 2015, 9:13 PM
pkelsey removed rS FreeBSD src repository as the repository for this revision.

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
233

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
233

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
82

Typo here: Clearnig -> Clearing

87

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.

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
1973

This needs to include TH_ACK as well

2439

snd_una should be incremented here.

pkelsey updated this revision to Diff 10999.Dec 9 2015, 7:43 PM
pkelsey edited the test plan for this revision. (Show Details)

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
jch edited edge metadata.EditedDec 23 2015, 8:33 AM

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
jch accepted this revision.Dec 23 2015, 8:33 AM
jch edited edge metadata.
stas accepted this revision.Dec 23 2015, 10:25 PM
stas added a reviewer: stas.
This revision was automatically updated to reflect the committed changes.