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
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 14571

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

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

Doesn't this need to be

} else
    ccb->ccb_num_entries++;

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

1103

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

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

It does appear the increment is missing.

1103

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

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

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

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

Yup, that can go.

tuexen added inline comments.Jan 30 2018, 8:55 AM
sys/netinet/tcp_input.c
3446

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

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

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.