Page MenuHomeFreeBSD

TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two)

Authored by pkelsey on Jan 25 2018, 6:18 AM.



This is an implementation of the client side of RFC7413. It
also includes a pre-shared key mode of operation in which the server
requires the client to be in possession of a shared secret in order to
successfully open TFO connections with that server.

The names of some existing fastopen sysctls have changed (e.g.,
net.inet.tcp.fastopen.enabled ->

This revision incorporates the comments made on the now-abandoned D13718.

This revision depends on D14046.

Test Plan

Tested using the quick-and-dirty tools at as well as scripts for exercising the sysctls and some hackish packetdrill scripts (it would be nice if one could use packetdrill to extract a cookie value from a SYN-ACK and use it elsewhere in the script).

Diff Detail

rS FreeBSD src repository - subversion
Lint Skipped
Unit Tests Skipped
Build Status
Buildable 14571

Event Timeline

FYI: I'm working on improving packetdrill for TCP fast open and also on some tests...


Don't you need to

if (cce != NULL)
    TAILQ_REMOVE(&ccb->ccb_entries, cce, cce_link)

since you are inserting cce at the head?


Doesn't this need to be

} else

since you need to increment the counter when inserting a new entry?


Doesn't this need to be

KASSERT(ccb->ccb_num_entries <= limit,
            ("%s: ccb->ccb_num_entries %d not within limit %d", __func__,
                ccb->ccb_num_entries, limit));

since there is no requirement that each bucket is full.


That’s true, in the successful reuse case, there should be a remove, otherwise when the bucket has more than one entry, the next pointer on the newly-last entry will point to the newly-first entry instead of to NULL (with the current TAILQ impl.).

It can be put after the return-if-NULL.


It does appear the increment is missing.


Yes, where ‘full’ means ‘occupancy >= new limit’. This assert as written would have been tripped if the bucket limit sysctl was set to a non-zero value less than the current limit and greater than the occupancy of any bucket.


The above line should be removed. cce->server_mss is assigned in both branches of the following if statement.


For consistency with the enabling/disabling sysctl-variables, please use psk_enable instead of psk_enabled. Sorry for missing this earlier...


That’s odd, as I audited the code for ‘enabled’ and adjusted throughout, with the sysctl doc updates in one commit and the code updates separatately. Refactoring the original patch into separate reviews involved about a dozen cherry picks with interstitial manual patching of hand edited diffs. It could be there was a missing cherry pick in that effort that covered the code changes for psk_enabled -> psk_enable. Will go back and review, and in any event, get the psk_enable changes back in.


Yup, that can go.


I guess the above condition should read:

if ((optlen != TCPOLEN_FAST_OPEN_EMPTY) &&
    ((optlen < TCPOLEN_FAST_OPEN_MIN) ||
     (optlen > TCPOLEN_FAST_OPEN_MAX) ||
     ((optlen & 0x01) == 0x01)))

since TCP FO options with invalid length MUST be ignored. So there is a logical error in the condition and the test for the length being odd was missing.


Or maybe the answer is to minimize further or remove this check/continue altogether as the full validation/ignore is really part of the cookie checking code.


That can also be done. I saw redundant code in the cookie handling. Then you only need to make sure that optlen >= 2.
I'll move the checking code in my copy and continue testing...

Addressed reviewer comments.

  • Fixed cookie cache entry reuse-case list processing and added missing entry count bump during create.
  • Fixed cookie cache bucket trim assert
  • Removed redundant setting of server_mss when updating cookie cache entry.
  • psk_enabled -> psk_enable
  • Removed cookie length validation from tcp_dooptions() as it is handled by the client and server cookie processing paths. Added minimum length check to the client side cookie processing path.

Going to commit this soon as there has not been any further review comments in nearly three weeks.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 26 2018, 2:53 AM
This revision was automatically updated to reflect the committed changes.