Page MenuHomeFreeBSD

Avoid cwnd update for SYN sequence space
ClosedPublic

Authored by rscheff on Jan 28 2019, 11:11 AM.

Details

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.

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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.
  • adding RACK to the SYN bit sequence space fix

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

  • consolidate snd_una adjustment for SYN bit
  • set snd_wnd in the generic case in rack
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 requested changes to this revision.Feb 17 2019, 11:05 AM

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.

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
This revision is now accepted and ready to land.Apr 20 2020, 10:16 AM
This revision was automatically updated to reflect the committed changes.

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.
  • missed snd_una adjustment when server-side, as "acked" is also finally used to increment snd_una (off-by-1 error).
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.

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.

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

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/

This revision is now accepted and ready to land.Apr 28 2020, 1:42 PM
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?

  • use dedicate flag variable (should be optimized out) and fix english in comment
This revision now requires review to proceed.Apr 28 2020, 8:43 PM
This revision is now accepted and ready to land.Apr 28 2020, 9:06 PM