Page MenuHomeFreeBSD

Fix server side TCP fast open
ClosedPublic

Authored by tuexen on Jan 16 2020, 7:34 PM.

Details

Summary

The server side of TCP fast open relies on the delayed ACK timer for sending the SYN-AK to allow including user data in the SYN-ACK.
When DSACK support was added in r347382, an immediate ACK was requested even for the received SYN with user data. This patch fixes that.

The problem was reported by Jeremy Harris on the transport@ mailing list.

Test Plan

Run the following packetdrill 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

tuexen created this revision.Jan 16 2020, 7:34 PM

There is a special case handling in DSACK when start and end (2nd and 3rd param) are equal. Placing the todrop before the block will prevent DSACKs from generated if the last in-sequence packet was sent twice (is to be dsacked).

I suggest to move the (todrop > 0) check just over the tf_ACKNOW statement, possibly with a check if we are not to send out a (D)SACK block (tp->rcv_numsacks >= 1).

tuexen updated this revision to Diff 66859.Jan 16 2020, 8:20 PM

Same change for RACK and BBR.

tuexen added a comment.EditedJan 16 2020, 8:24 PM

There is a special case handling in DSACK when start and end (2nd and 3rd param) are equal. Placing the todrop before the block will prevent DSACKs from generated if the last in-sequence packet was sent twice (is to be dsacked).

Please note that todrop is positive when we enter the block. It is only reduced when a SYN segment is received. So the case where todrop is zero and caught by the condition I added is only when todrop is initially 1 and reduced to 0 because we received a SYN segment. The condition todrop > 0 and tlen == 0 would be handled wrong. Can it happen?

I suggest to move the (todrop > 0) check just over the tf_ACKNOW statement, possibly with a check if we are not to send out a (D)SACK block (tp->rcv_numsacks >= 1).

rscheff_gmx.at accepted this revision.Jan 16 2020, 10:10 PM

I can distinctively remember that i deliberately have that special case where the two last parameters are identical in the first DSACK patch.

However, going over the history, here we are save. While discussing this, all the DSACK test cases were run with this patch, and all passed. There is not yet a specific test case for DSACK for TFO though.

So, one thing to validate would be the retransmission of a TFO SYN with data, if the DSACK code emits a DSACK block for the twice received data. It may be off by 1 (including the seq no of the SYN, rather the first data byte).

This revision is now accepted and ready to land.Jan 16 2020, 10:10 PM
rrs accepted this revision.Jan 24 2020, 12:44 PM