Fix handling of RST segments in SYN-RCVD state via the syn cache code path

[tuexen]

Summary

The handling of RST segments in the SYN-RCVD state exists in the code paths. Both are not consistent and the one on the syn cache code does not conform to the relevant specifications (Page 69 of RFC 793 and Section 4.2 of RFC 5961).

This patch fixes this:

• The sequence numbers checks are fixed as specified in page Page 69 RFC 793.
• The sysctl variable net.inet.tcp.insecure_rst is now honored as specified in Section 4.2 of RFC 5961.

Test Plan

Get all tests from packetdrill tests passed.

[bz]

Scrolling through it looks OK with cross-checking to the RFCs.
Mostly commenting on the fact that we should get this in before BETA as it does change the kernel KPI.
Hope someone will do the full technical review (want to add lstewart as well?)

[tuexen]

In D17595#375585, @bz wrote:

Scrolling through it looks OK with cross-checking to the RFCs.
Mostly commenting on the fact that we should get this in before BETA as it does change the kernel KPI.

That is my hope. I could not change the signature of syncache_chkrst(), but it would be better to
include the change. I guess if I can convince people to review it tomorrow, I could try to get it in on Friday.

Hope someone will do the full technical review (want to add lstewart as well?)

Added Lawrence.

[bz]

It would greatly help if the proposed commit message would also cite the RFC sections not just the RFCs; ideally actually comments in the code would do that as well at the right place so that future reference lookups will be easier.

[bz]

For as much as my brain still functions this seems ok

Question to be ignored: does it also apply to non-default TCP stacks (which I have never looked at)?

[glebius]

syncache_chkrst() isn't a public function, so no worries on KBI.

[lstewart]

sys/netinet/tcp_syncache.c

583 ↗

(On Diff #49248)

Please reference Pg69 here and perhaps reproduce the table as well. A note well with respect to syncache_lookup() setting sc_irs to th_seq-1 might also be helpful as it took me a bit to reason about the +1 and SEQ_LEQ -> SEQ_LT changes.

1733 ↗

(On Diff #49248)

I think this may not be strictly correct in the TFO case?

[lstewart]

@tuexen Any thoughts on the TFO case?

[tuexen]

In D17595#379824, @lstewart wrote:

@tuexen Any thoughts on the TFO case?

@lstewart Could you be more specific what you think is not handled correctly?

[tuexen]

In D17595#375876, @bz wrote:

For as much as my brain still functions this seems ok

Question to be ignored: does it also apply to non-default TCP stacks (which I have never looked at)?

This fix also applies to non-default TCP stacks. The code fixed here is called in tcp_input.c:1168. The handling if RST-segments for sockets in the LISTEN state is handled in the generic code.

[tuexen]

@lstewart Could you be more specific what you think is not handled correctly?

@lstewart Found the line you were referring to. Let me check...

[tuexen]

sys/netinet/tcp_syncache.c

583 ↗

(On Diff #49248)

Fixed in r339989.

[tuexen]

sys/netinet/tcp_syncache.c

1733 ↗

(On Diff #49248)

syncache_respond() is used for three different purposes:

1. Transmit a SYN-ACK segment in response to a received SYN segment.
2. Retransmit a SYN-ACK segment after a timeout.
3. Send an ACK segment as a challenge ack in response to a received RST segment.

If the received SYN segment contains a TCP-FO cookie request, the SYN-ACK is transmitted and retransmitted correctly. If a RST segment is received and triggers the sending of a challenge ack, this is transmitted correctly.

If the received SYN segment contains an acceptable TCP-FO cookie, the SYN-ACK is transmitted and retransmitted via a different code path, since you don't process this using syn-cache entries. The same applies to the handling of received RST segments.