[PATCH net] tcp: reject completely old segments during sequence validation

[PATCH net] tcp: reject completely old segments during sequence validation

[Michael Cohen]

tcp_sequence() rejects an incoming segment when end_seq is before
rcv_wup. Since end_seq is one past the last sequence number consumed by
the segment, this misses the boundary case where end_seq is equal to
rcv_wup.

A segment that consumes sequence space and has end_seq equal to rcv_wup
is therefore allowed to reach later processing, including ACK handling,
even though it should be rejected as a completely old segment.

Reject this boundary case while retaining the existing behavior for
segments that consume no sequence space.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>
Reported-by: Tamir Shahar <tamir.shahar1@gmail.com>
Reported-by: Amit Klein <amit.klein@mail.huji.ac.il>
Signed-off-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>
---

Packetdrill reproducer:

// Off by one bug in tcp_sequence()
// the negative test before(end_seq, tp->rcv_wup) has off by one error, since end_seq is SEG.SEQ+SEG.LEN,
// whereas the RFCs require SEG.SEQ+SEG.LEN-1 (their positive test is RCV.NXT =< SEG.SEQ+SEG.LEN-1)

0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1024) = 0

+0 < S 0:0(0) win 12345
+0 > S. 0:0(0) ack 1 <...>
+0 < . 1:1(0) ack 1 win 12345
+0 accept(3, ..., ...) = 4

// This is not mandatory for the phenomenon, we just do this to increment SND.NXT (set SND.NXT=101, retain SND.UNA=1) so we can show 
// later that the problematic segment is actually accepted (via the tcpi_accepted_bytes count).
+0 send(4, ..., 100, 0) = 100
+0 > P. 1:101(100) ack 1

+0 < P. 1:1001(1000) ack 1 win 12345
+0 > . 101:101(0) ack 1001

// Now RCV.NXT=1001, so according to the RFC, a subsequent 1:1001 should be discarded.
// But in Linux, 1:1001 is accepted(!).
// Note that bytes_acked is incremented to the packet's ack number, which shows the packet is accepted.

+0 < P. 1:1001(1000) ack 23 win 12345   
// +0 < P. 1:1000(999) ack 23 win 12345   // if you use this instead, you get an assertion error, as expected.

// this assert will succeed in the presence of the bug, but per the RFCs, it should fail because the packet should have been discarded
+0 %{ assert(tcpi_bytes_acked==22) }%  

 net/ipv4/tcp_input.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index daff93d51..3bfa0f07d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4844,7 +4844,8 @@ static enum skb_drop_reason tcp_sequence(const struct sock *sk,
 	const struct tcp_sock *tp = tcp_sk(sk);
 	u32 seq_limit;
 
-	if (before(end_seq, tp->rcv_wup))
+	if (before(end_seq, tp->rcv_wup) ||
+	    (end_seq == tp->rcv_wup && seq != end_seq))
 		return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
 
 	seq_limit = tp->rcv_nxt + tcp_max_receive_window(tp);
-- 
2.43.0

Re: [PATCH net] tcp: reject completely old segments during sequence validation

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
tcp: reject completely old segments during sequence validation

tcp_sequence() previously only dropped a segment when end_seq was strictly
before rcv_wup.  This patch also drops the boundary case where end_seq
equals rcv_wup, as long as the segment consumes sequence space, so that
such segments no longer reach ACK processing.

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 0f60a1dbf927..e51c76884460 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4844,7 +4844,8 @@ static enum skb_drop_reason tcp_sequence(const struct sock *sk,
>  	const struct tcp_sock *tp = tcp_sk(sk);
>  	u32 seq_limit;
>
> -	if (before(end_seq, tp->rcv_wup))
> +	if (before(end_seq, tp->rcv_wup) ||
> +	    (end_seq == tp->rcv_wup && seq != end_seq))
>  		return SKB_DROP_REASON_TCP_OLD_SEQUENCE;

[Severity: High]
Does this new clause also match the crossing SYN+ACK of a simultaneous
connect(), and does that re-introduce the behaviour fixed by commit
23e89e8ee7be7 ("tcp: Don't drop SYN+ACK for simultaneous connect.")?

When a TCP_SYN_SENT socket receives a bare SYN, the crossed-SYN branch in
tcp_rcv_synsent_state_process() moves it to TCP_SYN_RECV and leaves
rcv_wup equal to rcv_nxt:

net/ipv4/tcp_input.c:tcp_rcv_synsent_state_process() {
	...
	tcp_set_state(sk, TCP_SYN_RECV);
	...
	WRITE_ONCE(tp->rcv_nxt, TCP_SKB_CB(skb)->seq + 1);
	WRITE_ONCE(tp->copied_seq, tp->rcv_nxt);
	tp->rcv_wup = TCP_SKB_CB(skb)->seq + 1;
	...
}

The following tcp_send_synack() sends a segment with TCPHDR_SYN set, and
__tcp_transmit_skb() only calls tcp_select_window() when TCPHDR_SYN is
clear, so rcv_wup is not advanced:

net/ipv4/tcp_output.c:__tcp_transmit_skb() {
	...
	if (likely(!(tcb->tcp_flags & TCPHDR_SYN))) {
		th->window	= htons(tcp_select_window(sk));
	...
}

The peer's bare SYN+ACK then arrives with end_seq == seq + 1, i.e.
end_seq == tp->rcv_nxt == tp->rcv_wup and seq != end_seq, which is exactly
what the new condition rejects with SKB_DROP_REASON_TCP_OLD_SEQUENCE.

Since th->syn is set, step 1 in tcp_validate_incoming() short-circuits
before step 4:

net/ipv4/tcp_input.c:tcp_validate_incoming() {
	...
	if (reason) {
		if (!th->rst) {
			if (th->syn)
				goto syn_challenge;
	...
}

Is the simultaneous-connect exception in step 4 still reachable after this
change?  It requires seq + 1 == end_seq and seq + 1 == tp->rcv_nxt, which
is the same condition the new step 1 check now filters out first:

net/ipv4/tcp_input.c:tcp_validate_incoming() {
	...
		if (sk->sk_state == TCP_SYN_RECV && sk->sk_socket && th->ack &&
		    TCP_SKB_CB(skb)->seq + 1 == TCP_SKB_CB(skb)->end_seq &&
		    TCP_SKB_CB(skb)->seq + 1 == tp->rcv_nxt &&
		    TCP_SKB_CB(skb)->ack_seq == tp->snd_nxt)
			goto pass;
	...
}

With that path taken, tcp_rcv_state_process() returns before tcp_ack() and
before the TCP_SYN_RECV to TCP_ESTABLISHED transition, so the visible
effects look like the ones described in 23e89e8ee7be7: a challenge ACK is
emitted instead, TCP_MIB_INERRS and LINUX_MIB_TCPSYNCHALLENGE are
incremented, establishment is delayed by an extra round trip until the
peer's pure ACK (seq == end_seq) is accepted, write() or send() right after
connect() returns -EAGAIN, and bpf_skops_parse_hdr() is not run for the
SYN+ACK.  Connect-to-self over loopback follows the same path.

Would restricting the new check so it does not apply while the socket is in
TCP_SYN_RECV with rcv_wup == rcv_nxt, or ordering it after the step 4
simultaneous-connect exception, preserve that case?

Cross-instance finding from sashiko-gemini (9561356177ca94322be4fdd8e89990a9b84fa51eecb45d5907ec165c8b9ca68e):
[Severity: Medium]
The patch breaks IPv6 flowlabel rehashing (ECMP repathing) on spurious RTO for non-SACK connections or when DSACK is disabled.

Re: [PATCH net] tcp: reject completely old segments during sequence validation

[Jakub Kicinski]

On Tue, 18 Aug 2026 23:52:30 +0300 Michael Cohen wrote:
> tcp_sequence() rejects an incoming segment when end_seq is before
> rcv_wup. Since end_seq is one past the last sequence number consumed by
> the segment, this misses the boundary case where end_seq is equal to
> rcv_wup.
> 
> A segment that consumes sequence space and has end_seq equal to rcv_wup
> is therefore allowed to reach later processing, including ACK handling,
> even though it should be rejected as a completely old segment.
> 
> Reject this boundary case while retaining the existing behavior for
> segments that consume no sequence space.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>
> Reported-by: Tamir Shahar <tamir.shahar1@gmail.com>
> Reported-by: Amit Klein <amit.klein@mail.huji.ac.il>
> Signed-off-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>

CI says the AccECN cases need to be adjusted.
Example failure output (tcp_accecn_synack_rexmit.pkt):

  tcp_accecn_synack_rexmit.pkt:15: error handling packet: live packet field
    tcp_ece: expected: 0 (0x0) vs actual: 1 (0x1)
  script packet:  0.104 .W  1:1(0) ack 1 <ECN e1b 1 ceb 0 e0b 1,nop>
  actual packet:  0.102 .EA 1:1(0) ack 1 win 1050 <ECN e1b 1 ceb 0 e0b 1,nop>

The AccECN tests expect the kernel to observe and react to the retransmitted
SYN-ACK, but the new boundary rejection prevents that.