This fixes actually a bug. With this patch the following packetdrill runs successfully:

 0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0.000 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.000 bind(3, ..., ...) = 0
+0.000 setsockopt(3, SOL_SOCKET, SO_LINGER, {onoff=1, linger=0}, 8) = 0
+0.000 listen(3, 1) = 0
+0.000 < S    0:0(0)       win 65535 <mss 1460,sackOK,eol,eol>
+0.000 > S.   0:0(0) ack 1 win 65535 <mss 1460,sackOK,eol,eol>
+0.000 <  .   1:1(0) ack 1 win 65535
+0.000 accept(3, ..., ...) = 4
+0.000 close(3) = 0
+0.000 getsockopt(4, SOL_SOCKET, SO_LINGER, {onoff=128, linger=0}, [8]) = 0
+0.000 close(4) = 0
+0.000 > R.   1:1(0) ack 1 win     0

Thanks for finding the bug and fixing it.

Let me have a look into it tomorrow. I agree, it looks strange.

Any chance to have a more specific name than newsack?

Let's discuss this at the next Transport call...

In D28686#641972, @rscheff wrote:

In D28686#641967, @tuexen wrote:

My understanding is that if net.inet.tcp.fastpath.acceptany is set, all cookies are excepted. This includes empty cookies as mentioned in RFC 7413.

Correct, it may generate an empty cookie - and accept SYN data with a empty cookie. However, with acceptany, the server will never actually generate a FastOpen response, thus a Client will not know that TFO is supported by the server.

A client will have to perform SYN+data speculatively, since the Server never response even with an empty TFO.

Looking at the client side, it appears that SYN+data is only sent on a subsequent sendto(), if a prior session carried the TFO in the SYN,ACK from the server - when the client has a key entry for that server.

Let me expand the special case here perhaps, to return a zero-filled, minimum-length cookie without doing the hashing.

My understanding is that if net.inet.tcp.fastpath.acceptany is set, all cookies are excepted. This includes empty cookies as mentioned in RFC 7413.

The above modifications are necessary in combination with D28656 to allow the following packetdrill script to run successfully:

 0.00 `sysctl -w net.inet.tcp.hostcache.purgenow=1`
+0.00 `sysctl -w net.inet.tcp.syncookies_only=0`
+0.00 `sysctl -w net.inet.tcp.syncookies=1`
+0.00 `sysctl -w net.inet.tcp.rfc1323=1`
+0.00 `sysctl -w net.inet.tcp.sack.enable=1`
+0.00 `sysctl -w net.inet.tcp.ecn.enable=2`

Please consider MFC'ing to stable/13.

Let us discuss this on the bi-weekly transport call... I'm not convinced that the gain you get from this is higher than the risk to run into bugs on peer implementations.

I tested the patch. It fixes: panic: pfi_dynaddr_setup: non-NULL dyn (2).

I would suggest to separate the whitspace changes and the functional changes into two commits.

Maybe mm@ can test this?

Can you describe how one can hit the code you are suggesting to be removed?

Committed in cc3c34859eab1b317d0f38731355b53f7d978c97.

Committed in d2b3ceddccac60b563f642898e3a314647666a10.

Rebase.

In D28143#629142, @rscheff wrote:

Functionally, I concur that this is the right way to handle RST packets.

From an implementation, perhaps having the check for TH_RST and the V_tcp_tolerate_missing_ts sysctl of D28142 in a combined || check, rather than two fully independent branches (which is likely if D28142 and D28143 are applied independently).

Address jhb's comments.

I guess you need a similar dance (for 16-bit) in pf_normalize_tcpopt() when updating the MSS.

When committing, please add a reference to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251540

It fixes the issue. For SCTP we would need something like sctpdrop. I can try to add that...

Committed in r367530.