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

[pkelsey]

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

[tuexen]

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

[tuexen]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[pkelsey]

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