Details

Reviewers

tuexen
lstewart
rgrimes

Group Reviewers

transport

Commits

rS361342: MFC r360477: Correctly set up the initial TCP congestion window in all cases
rS360477: Correctly set up the initial TCP congestion window in all cases,
rS360143: Correctly set up the initial TCP congestion window

Summary

During testing i found an issue (filed as Bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235256 ) where the Initial Window would be 11 SMSS for server-side sessions. This is due to the macro BYTES_THIS_ACK including the sequence space of the SYN bit, and regular ACK processing will commence right after cc_conn_init has initialized the initial window. When Appropriate Byte Counting (default) is enabled, this adds 1 to cwnd, but without ABC, an entire SMSS is added.

Found that the most effective way to deal with this appears to be to adjust snd_una right after the connection initialization, to include the one byte in sequence space occupied by the SYN bit.

This does not change the regular ACK processing, while making BYTES_THIS_ACK work properly (skipping cc_ack_received processing for empty SYN-ACK and final 3WHS ACK).

Also addressing an oversight, where RFC5681 mentions that cwnd is only to be updated for ACKs which acknowledge any new data. When doing ABC, this had been done as an empty ACK (keepalive, window update) will increase cwnd only by 0 bytes. But in the non ABC case, keep alives and window updates would still inflate cwnd.

adjusting initwnd when not doing ABC, as SYN-bit takes sequence space

messing with snd_una not good

before ack processing, adjust snd_una to include SYN sequence space

no SEQ_EQ

Test Plan

The following packetdrill scripts check the initial window with and without ABC, for client side and server side sessions. Note that with this patch, as the initial SYN bit is removed from the sequence space, cwnd may be reduced by 1 in other packetdrill tests, who were written against the old behavior.

newreno-iw10-no-abc-server.pkt2 KBDownload

newreno-iw10-abc-server.pkt2 KBDownload

newreno-iw10-no-abc-client.pkt2 KBDownload

newreno-iw10-abc-client.pkt2 KBDownload

(updated test scripts to run without error on newer python versions)

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22211
Build 21420: arc lint + arc unit

Event Timeline

rscheff created this revision.Jan 28 2019, 11:11 AM

Herald added a reviewer: transport. · View Herald TranscriptJan 28 2019, 11:11 AM

Harbormaster completed remote builds in B22208: Diff 53300.Jan 28 2019, 11:11 AM

rscheff edited the summary of this revision. (Show Details)Jan 28 2019, 11:21 AM

rscheff edited the test plan for this revision. (Show Details)

rscheff added reviewers: tuexen, lstewart.

rscheff set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a subscriber: imp. · View Herald TranscriptJan 28 2019, 11:21 AM

adding RACK to the SYN bit sequence space fix

Harbormaster completed remote builds in B22211: Diff 53307.Jan 28 2019, 12:37 PM

The validation scripts for RACK, which was actually similarly affected.

One thing to note, though:

I found that RACK will perform the "RTO" with the next new data segment (eg. 11th) when no ACKs are received. I haven't read the draft closely enough to know if this is the expected behavior (since cwnd does not reflect that the congestion window is then actually 11 segments instead of 10:

window at accept: cwnd: 10000 ssthresh: 1073725440
initial window: cwnd: 10000 ssthresh: 1073725440
(segments 1-10 sent)
(timeout ~2sec)
(segment 11 sent)
RTO window: cwnd: 10000 ssthresh: 1073725440

rack-iw10-no-abc-server.pkt2 KBDownload

rack-iw10-abc-server.pkt2 KBDownload

rack-iw10-no-abc-client.pkt2 KBDownload

rack-iw10-abc-client.pkt2 KBDownload

rscheff mentioned this in D19033: Initialise snd_wnd on the server side always before processing the ack.Jan 30 2019, 8:35 PM

RACK already increments snd_una to deal with FASTOPEN. Consolidate the snd_una++ in the code then?

sys/netinet/tcp_stacks/rack.c
5240	Already have the adjustment of snd_una up here.
5472	snd_una is already adjusted for the FASTOPEN special case

consolidate snd_una adjustment for SYN bit
set snd_wnd in the generic case in rack

Harbormaster completed remote builds in B22293: Diff 53469.Jan 31 2019, 10:49 AM

typo

Harbormaster completed remote builds in B22294: Diff 53470.Jan 31 2019, 11:47 AM

rscheff added inline comments.Jan 31 2019, 2:41 PM

sys/netinet/tcp_stacks/rack.c
5469	snd_una is already advanced for the syn-bit in the fast open case. move this out and do it in all cases when allowable.

Hi Richard,

can you please:

Move your suggested changes in cc/cc_newreno.c to a different review, since they fix a different bug.
Rebase you patch of tcp_stacks/rack.c to an up to date version. The changes related to tp->snd_wnd are already committed in r343661.

I think then this patch looks good.

This revision now requires changes to proceed.Feb 17 2019, 11:05 AM

Merge branch 'master' into fix-not-rfc3465
- remove non-abc fix for cong-avoidance cwnd increase on empty acks

Harbormaster completed remote builds in B22553: Diff 54035.Feb 18 2019, 10:01 AM

rscheff edited the test plan for this revision. (Show Details)Apr 19 2020, 5:59 PM

Herald added a subscriber: melifaro. · View Herald TranscriptApr 19 2020, 5:59 PM

add fix to BBR stack

Harbormaster completed remote builds in B30614: Diff 70787.Apr 19 2020, 6:01 PM

rgrimes accepted this revision.Apr 20 2020, 4:14 AM

tuexen accepted this revision.Apr 20 2020, 10:16 AM

This revision is now accepted and ready to land.Apr 20 2020, 10:16 AM

Closed by commit rS360143: Correctly set up the initial TCP congestion window (authored by rscheff). · Explain WhyApr 21 2020, 1:06 PM

This revision was automatically updated to reflect the committed changes.

rscheff added a commit: rS360143: Correctly set up the initial TCP congestion window.

rscheff added a reverting change: rS360180: revert rS360143 - Correctly set up initial cwnd.Apr 22 2020, 12:16 AM

reopening due to the syzcaller panic found on simultaneous open using URG (out-of-band) data-with-SYN probing.

exclude synchonous open when adjusting for correct initial window

While TFO is prone to a similar panic under simultaneous opens prior to this fix, TFO explicitily does not support simultaneous open in the base stack.

For the base stack, this is done by decreasing the local variable accounting for the number of (data) bytes delivered with the ACK. This prevents any spurious dupack processing with various interactions with SACK, and limited transmit.

In both RACK and BBR, snd_una is adjusted directly. This may mark the ACK to the initial SYN as a dupack (first strike), but SYN retransmissions are handled explicitly independently.

Harbormaster completed remote builds in B30731: Diff 70998.Apr 26 2020, 10:59 AM

missed snd_una adjustment when server-side, as "acked" is also finally used to increment snd_una (off-by-1 error).

Harbormaster completed remote builds in B30743: Diff 71025.Apr 26 2020, 10:34 PM

rscheff added inline comments.Apr 26 2020, 10:39 PM

sys/netinet/tcp_input.c
2436	Alternatively, TF_NEEDSYN could be set here, relying on the snd_una adjustment in the block at line 2676. However, tracking it separately without any other side effects than the intended one (proper accounting for the initial window) is easier to understand.

Request for a comment, non blocking.

sys/netinet/tcp_input.c
2725	Can I get a block comment above this that explains what is being accounted for here? In all the cases you removed they had such block comments, so we have lost some code documentation by adding this here without comment. I believe this is accounting for several possibilities.

This revision is now accepted and ready to land.Apr 28 2020, 1:57 AM

add comment around the accouting change for the SYN in sequence space

This revision now requires review to proceed.Apr 28 2020, 1:19 PM

Harbormaster completed remote builds in B30776: Diff 71106.Apr 28 2020, 1:19 PM

Thank you for the comment, that helps me, small english nit

sys/netinet/tcp_input.c
2726	s/it for/for it in/

This revision is now accepted and ready to land.Apr 28 2020, 1:42 PM

tuexen added inline comments.Apr 28 2020, 2:58 PM

sys/netinet/tcp_input.c
2724	Am I reading this correctly: You initialize acked to 0; Under some condition you set it to -1; Then you check it for non-zero; Then you set it to something else If that is correct, then you are using it as a flag first and as a counter (for the bytes) later. Wouldn't it make sense to use a separate boolean variable for the first steps?