Page MenuHomeFreeBSD

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

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

Details

Summary

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 ->
net.inet.tcp.fastopen.server_enable).

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 https://people.freebsd.org/~pkelsey/tfo-tools/ 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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pkelsey created this revision.Jan 25 2018, 6:18 AM

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

tuexen added inline comments.Jan 28 2018, 12:15 PM
sys/netinet/tcp_fastopen.c
1054 ↗(On Diff #38432)

Don't you need to

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

since you are inserting cce at the head?

1059 ↗(On Diff #38432)

Doesn't this need to be

} else
    ccb->ccb_num_entries++;

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

1103 ↗(On Diff #38432)

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.

pkelsey added inline comments.Jan 28 2018, 3:35 PM
sys/netinet/tcp_fastopen.c
1054 ↗(On Diff #38432)

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.

1059 ↗(On Diff #38432)

It does appear the increment is missing.

1103 ↗(On Diff #38432)

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.

tuexen added inline comments.Jan 29 2018, 9:50 PM
sys/netinet/tcp_fastopen.c
971 ↗(On Diff #38432)

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

tuexen added inline comments.Jan 29 2018, 9:52 PM
sys/netinet/tcp_fastopen.c
326 ↗(On Diff #38432)

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

pkelsey added inline comments.Jan 30 2018, 6:45 AM
sys/netinet/tcp_fastopen.c
326 ↗(On Diff #38432)

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.

971 ↗(On Diff #38432)

Yup, that can go.

tuexen added inline comments.Jan 30 2018, 8:55 AM
sys/netinet/tcp_input.c
3446 ↗(On Diff #38432)

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.

pkelsey added inline comments.Jan 31 2018, 3:39 AM
sys/netinet/tcp_input.c
3446 ↗(On Diff #38432)

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.

tuexen added inline comments.Jan 31 2018, 8:04 AM
sys/netinet/tcp_input.c
3446 ↗(On Diff #38432)

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

pkelsey updated this revision to Diff 39505.Feb 19 2018, 7:45 PM

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.