Details

Reviewers

rrs
jtl
tuexen
cc
glebius

Group Reviewers

transport

Commits

rGb2d895f70ded: tcp: Handle <RST,ACK> in SYN-RCVD
rG110b2f4d47dd: tcp: Handle <RST,ACK> in SYN-RCVD
rGb352ef58c29b: tcp: Handle <RST,ACK> in SYN-RCVD

Summary

RFC793 (original TCP specification) declared that during the
SYN-RCVD state, only pure <RST> segments are to be processed.

However, RFC5961 and the current TCP specification RFC9293
improved this by stating that also <RST,ACK> should first
be checked for acceptability, and if acceptable, may either
trigger a challenge ACK, reset the connection, or get
silently ignored.

This was found with a hyperscaler loadbalancer, which would
validate the reachability of TCP services by frequently performing
a TCP 3WHS, immediately followed by a <RST,ACK>. When the <RST,ACK>
overtakes the final <ACK> of the 3WHS, that RST got ignoried, leading
to exhaustion of TCP tables, as the processing was different between
SYN-RCVD and ESTABLISHED state.

MFC to 13 and 12 after 1 week...

Test Plan

Verify that

<SYN
>SYN,ACK
<RST,ACK

with the exact Sequence number resets the session,

<SYN
>SYN,ACK
<RST,ACK

with a lower (not acceptable) sequence number is silently ignored, and

<SYN
>SYN,ACK
<RST,ACK

with a sequence number+1 elicits a challenge ACK

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 52858
Build 49749: arc lint + arc unit

Event Timeline

rscheff created this revision.Jul 12 2023, 8:20 AM

Herald added a reviewer: transport. · View Herald TranscriptJul 12 2023, 8:20 AM

Herald added subscribers: melifaro, imp. · View Herald Transcript

rscheff requested review of this revision.Jul 12 2023, 8:20 AM

Harbormaster completed remote builds in B52573: Diff 124529.Jul 12 2023, 8:20 AM

rscheff edited the summary of this revision. (Show Details)Jul 12 2023, 8:35 AM

Give me a day or two to add corresponding packetdrill test to the TCP testsuite...

No problem; I copied the ipv4 tests from rcv-rst-syn-rcvd and added the ACK bit into them; they don't seem to check if RST,FIN, RST,SYN are silently ignored though; also, in the simultaneous open case the ECONNREFUSED is apparently not reached. So something in tcp_input is still amiss.

Would you please elaborate the corresponding specification in RFC5961 and RFC9293?

it's in the changed comment (RFC9293, Section 3.10.7.4)

In short, RFC793 declared that only pure RSTs (no RST/ACK) would be acceptable in SYN-RCVD; since RFC5961 / RFC9293, the acceptance test (Seq / Ack of the RST or RST/ACK) has to be checked for acceptability, and if the RST is exactly at the left edge of the window, the session should be closed; if it's in-window, a challange ACK is to be sent, and in all other cases the RST is silently ignored.

This patch does NOT yet address the simultaneous SYN case - where the session seems to linger and no error is returned to the socket, when the session should get closed...

fix SYN-RCVD path for RST

Harbormaster completed remote builds in B52605: Diff 124586.Jul 12 2023, 6:58 PM

rscheff edited the test plan for this revision. (Show Details)Jul 12 2023, 7:03 PM

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

sys/netinet/tcp_input.c
2168–2170	Is this a DUP check? From the comment, it looks to be the same handling for simultaneous opening. This makes me wonder what happens in your test plan before this change? Or can you also add simultaneous opening test if not?
sys/netinet/tcp_syncache.c
654–660	For non-simultaneous case (pure passive open), the comment should match RFC9293, Section 3.10.7.4.

This revision now requires changes to proceed.Jul 16 2023, 8:22 PM

remove RST special case handling for SYN-RCVD

Harbormaster completed remote builds in B52671: Diff 124755.Jul 16 2023, 8:49 PM

In D40982#934677, @cc wrote:

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

Don't quite understand. The logic is doing exactly that, with the SEQ_LT and SEQ_GT checks; if the RST (with or without ACK) is ourside the acceptable window, the incoming segment is droped; if the RST matches exactly the next expected sequence number, the connection is reset and socket error code updated, and in the remaining cases, a challenge ACK is sent...

sys/netinet/tcp_input.c
2168–2170	The problem here (and I was initially confused too) is, that originally,this code never executes with RST/ACK - as there is the special handling of RST in SYN-RCVD state, and an RST/ACK was effectively illegal (silently ignored), or with RACK, the ACK is processed before the RST (@rrs is looking into the ordering in the RACK stack). In the non simultaneous SYN, the base stack uses the syncache (and that RST or RST/ACK handling is done there); because of the special handling of SYN-RCVD and SYN-SENT states above, the RST handling here is really only during later states. This particular switch case could be removed, however - since all the special RST handling for the two SYN states is done further up.
sys/netinet/tcp_syncache.c
654–660	If the code is updated to conform fully to RFC9293 certainly. Since this is currently a point update for the SYN-RCVD state for RST/ACK (in addition to pure RST), I don't think this comment here needs updating yet.

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

I meant the unit test shall cover both cases of the "out of window seq number test": lower and higher.

In D40982#934698, @cc wrote:

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

I meant the unit test shall cover both cases of the "out of window seq number test": lower and higher.

That is on my plate...