Page MenuHomeFreeBSD

Initialise snd_wnd on the server side always before processing the ack
ClosedPublic

Authored by tuexen on Jan 30 2019, 4:02 PM.

Details

Summary

The current code only initialises tp->snd_wnd before the ack processing when handling the ACK segment in the SYN-RCVD state only when the window scaling option is enabled. This patch ensures that the initialisation is done always to improve consistency.

This fixes the issue that the issue reported in PR235256 only shows up when having window scaling enabled.

Test Plan

Both tests should fail indicating the the CWND is too large:

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 22295

Event Timeline

tuexen created this revision.Jan 30 2019, 4:02 PM

So, what you are saying is that this issue here bascially masked the problem reported in PR235256 from happening when window scale was not negotiated.
At least that's what I'm observing, and what the code appears to be saying.

Because snd_wnd may be smaller than cwnd (as set by tcp_compute_initcwnd), which in turn leads to CCF_CWND_LIMITED not to be set in cc_ack_received wrapper, bypassing the cwnd update in the CC_ALGO(ack_received).

And the attached test cases should both succeed (as they are checking for IW=3 after the 3WHS) if both D19033 and D19000 are applied.

Now, I wonder - was this perhaps intentional, delaying setting snd_wnd (until "step6") after CC_ALGO(ack_received) in order to prevent that inital update from inflating the initial window?

Conversely: Was TCP_FAST_OPEN actually working without window scale then? (is there an existing packetdrill test for that scenario)?

rscheff_gmx.at accepted this revision.Jan 31 2019, 9:12 AM

Maybe a break in line 2433 is actually missing for the original issue (just another thought; browsing the history doesn't show that though). In any case, this change looks sensible enough.

RACK seems to have a similar code in rack_do_syn_recv.

tuexen updated this revision to Diff 53471.Jan 31 2019, 12:09 PM

Also fix the issue in the RACK stack.

RACK seems to have a similar code in rack_do_syn_recv.

Fixed. Thanks for reminding me...

tuexen updated this revision to Diff 53503.Feb 1 2019, 9:01 AM

Simplification of the fix within the RACK code.

rrs accepted this revision.Feb 1 2019, 11:26 AM
This revision is now accepted and ready to land.Feb 1 2019, 11:26 AM
This revision was automatically updated to reflect the committed changes.