LGTM with caveats. any plans for a Kyua test to catch the strafe fire in the ISN space this looks like it should close against?

I would like to understand why this change is proposed? Maybe we can reach the goal by implementing RFC 5956 or RFC 6191 in this case. This is now possible, since we have now the full tcb in TIMEWAIT. I am willing to do the work, once I understand the problem this proposed change wants to solve.

In D56307#1288534, @tuexen wrote:

I would like to understand why this change is proposed?

This was initially triggered by some of our clients recycling four-tuples within the TIME_WAIT period triggering what looked like random drops since we will sometimes accept the connection and other times not depending on the sequence number in the new SYN. The current behavior of allowing greater sequence numbers was, from what I can gather, introduced back when ISN generation (at least in BSD) was globally sequential. IMHO this doesn't make sense anymore given that ISNs are randomly generated nowadays. My understanding is that we need to make sure we're not accepting SYNs for the same connection as is currently in TIME_WAIT, leading me to believe that not allowing the same ISN is a good enough check before recycling the connection.

Maybe we can reach the goal by implementing RFC 5956 or RFC 6191 in this case. This is now possible, since we have now the full tcb in TIMEWAIT. I am willing to do the work, once I understand the problem this proposed change wants to solve.

I don't think RFC 5956 is relevant for the problem I'm trying to solve. At first glance I think RFC 6191 is what OpenBSD does. I originally dismissed this approach as my understanding was that TCP timestamps was discouraged, but looking again I see it tends to be enabled by default and the offending packets we see in our network do carry timestamps, so I think it could be a viable alternative to the current approach.

In D56307#1288341, @bms wrote:

LGTM with caveats. any plans for a Kyua test to catch the strafe fire in the ISN space this looks like it should close against?

I'm not familiar with Kyua, are there any docs I could look at?

I think something like this should work if using TCP timestamps instead. Would this be preferable?

diff
--- a/sys/netinet/tcp_timewait.c
+++ b/sys/netinet/tcp_timewait.c
@@ -223,7 +223,8 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to, struct tcphdr *th,
         * Allow UDP port number changes in this case.
         */
        if (((thflags & (TH_SYN | TH_ACK)) == TH_SYN) &&
-           SEQ_GT(th->th_seq, tp->rcv_nxt)) {
+           (((to->to_flags & TOF_TS) && TSTMP_LT(tp->ts_recent, to->to_tsval)) ||
+           SEQ_GT(th->th_seq, tp->rcv_nxt))) {
                /*
                 * In case we can't upgrade our lock just pretend we have
                 * lost this packet.

Checking a connecting in TIMEWAIT's irs doesn't make any sense to me - the actually in-window sequence numbers of the previous connection could be anywhere, depending on how much data was sent by the time the session got closed.

Checking against the old rcv_nxt makes more sense (which happens to be irs in the infancy of a new but abandoned connection; SEQ_GT effectively excludes 2^31 sequence numbers, when you really only would need to exclude rcv_nxt - rcv_wnd, or !(SEQ_GT(th_seq,(rcv_nxt - rcv_wnd) && SEQ_LE(th_seq, rcv_nxt)).

But for me, checking against irs makes the least sense...

In D56307#1288541, @marius.h_lden.org wrote:

In D56307#1288534, @tuexen wrote:

I would like to understand why this change is proposed?

This was initially triggered by some of our clients recycling four-tuples within the TIME_WAIT period triggering what looked like random drops since we will sometimes accept the connection and other times not depending on the sequence number in the new SYN. The current behavior of allowing greater sequence numbers was, from what I can gather, introduced

Question: If you just start a new TCP connection, how do you protect against TCP segments from the old connection being processed in the context of the new connection?

back when ISN generation (at least in BSD) was globally sequential. IMHO this doesn't make sense anymore given that ISNs are randomly generated nowadays. My understanding is that

I am not sure this is true: RFC 9293 describes a monotonic algorithm and tcp_new_isn() is also not random.

we need to make sure we're not accepting SYNs for the same connection as is currently in TIME_WAIT, leading me to believe that not allowing the same ISN is a good enough check before recycling the connection.

Maybe we can reach the goal by implementing RFC 5956 or RFC 6191 in this case. This is now possible, since we have now the full tcb in TIMEWAIT. I am willing to do the work, once I understand the problem this proposed change wants to solve.

I don't think RFC 5956 is relevant for the problem I'm trying to solve. At first glance I think RFC 6191 is what OpenBSD does. I originally dismissed this approach as my understanding was that TCP timestamps was discouraged, but looking again I see it tends to be enabled by default and the offending packets we see in our network do carry timestamps, so I think it could be a viable alternative to the current approach.

Sorry. Typo. I was referring to RFC 5961. I think this solves your problem but assumes that receiving a RST segment moves an endpoint from TIMEWAIT to CLOSED. RFC 1337 describes why FreeBSD doesn't do it.

Timestamps are on by default except for Windows.

In D56307#1288341, @bms wrote:

LGTM with caveats. any plans for a Kyua test to catch the strafe fire in the ISN space this looks like it should close against?

I'm not familiar with Kyua, are there any docs I could look at?

I think something like:

diff --git a/sys/netinet/tcp_timewait.c b/sys/netinet/tcp_timewait.c
index ba5c90c91e43..b1d651c9497e 100644
--- a/sys/netinet/tcp_timewait.c
+++ b/sys/netinet/tcp_timewait.c
@@ -218,12 +218,17 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to, struct tcphdr *th,
        /*
         * If a new connection request is received
         * while in TIME_WAIT, drop the old connection
-        * and start over if the sequence numbers
-        * are above the previous ones.
+        * and start over if allowed by RFC 6191.
         * Allow UDP port number changes in this case.
         */
        if (((thflags & (TH_SYN | TH_ACK)) == TH_SYN) &&
-           SEQ_GT(th->th_seq, tp->rcv_nxt)) {
+           ((((tp->t_flags & TF_RCVD_TSTMP) != 0) &&
+             ((to->to_flags & TOF_TS) != 0) &&
+             TSTMP_LT(tp->ts_recent, to->to_tsval)) ||
+            (((tp->t_flags & TF_RCVD_TSTMP) == 0) &&
+             ((to->to_flags & TOF_TS) != 0) &&
+             (V_tcp_tolerate_missing_ts == 0)) ||
+            SEQ_GT(th->th_seq, tp->rcv_nxt))) {
                /*
                 * In case we can't upgrade our lock just pretend we have
                 * lost this packet.

would implement support for RFC 6191. If that solves your issue, I can put it up for review.

A patch is up for review: D56321.

Thanks, I think that should do the trick.