Avoid cwnd update for SYN sequence space

[rscheff]

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)

[rscheff]

• adding RACK to the SYN bit sequence space fix

[rscheff]

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]

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

sys/netinet/tcp_stacks/rack.c

5240 ↗

(On Diff #53307)

Already have the adjustment of snd_una up here.

5472 ↗

(On Diff #53307)

snd_una is already adjusted for the FASTOPEN special case

[rscheff]

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

[rscheff]

• typo

[rscheff]

sys/netinet/tcp_stacks/rack.c

5470 ↗

(On Diff #53470)

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.

[tuexen]

Hi Richard,

can you please:

1. Move your suggested changes in cc/cc_newreno.c to a different review, since they fix a different bug.
2. 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.

[rscheff]

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

[rscheff]

• add fix to BBR stack

[rscheff]

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

[rscheff]

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

[rscheff]

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

[rscheff]

sys/netinet/tcp_input.c

2402 ↗

(On Diff #71025)

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.

[rgrimes]

Request for a comment, non blocking.

sys/netinet/tcp_input.c

2698 ↗

(On Diff #71025)

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.

[rscheff]

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

[rgrimes]

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

sys/netinet/tcp_input.c

2699 ↗

(On Diff #71106)

s/it for/for it in/

[tuexen]

sys/netinet/tcp_input.c

2706 ↗

(On Diff #71106)

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?

[rscheff]

• use dedicate flag variable (should be optimized out) and fix english in comment