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

[PATCH net v3] 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 for segments without SYN or FIN, while
retaining the existing behavior for control segments and segments
that consume no sequence space.

One consequence of the early rejection is that, in some cases, a
completely old duplicate data segment will no longer reach
tcp_rcv_spurious_retrans(). For IPv6, this may prevent a retransmission
with a different flow label from triggering transmit-path rehashing.

We accept this trade-off because the segment is completely old and
outside the receive window, and therefore should be rejected before
normal ACK processing.

We consider preserving the RFC 793 and RFC 9293 sequence-acceptability
boundary for such data segments more important than preserving this
side effect of processing an otherwise unacceptable segment.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>
Reported-by: Tamir Shahar <tamir.shahar1@mail.huji.ac.il>
Reported-by: Amit Klein <amit.klein@mail.huji.ac.il>
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>
---
Changes in v3:
- Group the old-segment checks behind a single unlikely() branch to
  avoid adding unnecessary cost to the TCP fast path.

Changes in v2:
- Exclude SYN and FIN segments from the boundary-old check to preserve
  existing SYN/FIN handling, including simultaneous connect
  and retransmitted SYN+ACK AccECN processing.

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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index daff93d51..10e49e6e1 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4844,8 +4844,12 @@ 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))
-		return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
+	if (unlikely(!after(end_seq, tp->rcv_wup))) {
+		if (before(end_seq, tp->rcv_wup) ||
+		    (seq != end_seq &&
+		     !(tcp_flag_byte(th) & (TCPHDR_SYN | TCPHDR_FIN))))
+			return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
+	}
 
 	seq_limit = tp->rcv_nxt + tcp_max_receive_window(tp);
 	if (unlikely(after(end_seq, seq_limit))) {
-- 
2.43.0

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

[Simon Baatz]

Hi Michael,

On Mon, Aug 31, 2026 at 11:56:11PM +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 for segments without SYN or FIN, while
> retaining the existing behavior for control segments and segments
> that consume no sequence space.
> 
> One consequence of the early rejection is that, in some cases, a
> completely old duplicate data segment will no longer reach
> tcp_rcv_spurious_retrans(). For IPv6, this may prevent a retransmission
> with a different flow label from triggering transmit-path rehashing.
> 
> We accept this trade-off because the segment is completely old and
> outside the receive window, and therefore should be rejected before
> normal ACK processing.
> 
> We consider preserving the RFC 793 and RFC 9293 sequence-acceptability
> boundary for such data segments more important than preserving this
> side effect of processing an otherwise unacceptable segment.

I think the RFC rationale needs tightening: RFC 9293's acceptability
test uses RCV.NXT and RCV.WND. It has no rcv_wup equivalent.

Strictly speaking, whenever rcv_wup < RCV.NXT Linux already accepts
more than the RFC check (and not just at the off-by-one boundary
this patch targets).  I take it that's deliberate: a segment can be
"completely old" as data and still carry fresh control data.

But that's equally true in the rcv_wup == rcv_nxt case, and it is
exactly what lets a non-SACK receiver reach
tcp_rcv_spurious_retrans().  For example, before the patch:

1. peer sends 1:1001, we ACK 1001; now rcv_nxt == rcv_wup == 1001
2. ACK is lost (e.g. a reverse path failure)
3. after RTO the peer retransmits 1:1001; the segment passes
   tcp_sequence() and tcp_rcv_spurious_retrans() is called from
   tcp_data_queue()

So the "unacceptable segment" isn't an obscure corner case.  It may
be the ordinary retransmission of the last segment, which is the
event tcp_rcv_spurious_retrans() is supposed to react to.  What makes
us confident that we may just drop that functionality?

> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>
> Reported-by: Tamir Shahar <tamir.shahar1@mail.huji.ac.il>
> Reported-by: Amit Klein <amit.klein@mail.huji.ac.il>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Michael Cohen <michael.cohen3@mail.huji.ac.il>
> ---
> Changes in v3:
> - Group the old-segment checks behind a single unlikely() branch to
>   avoid adding unnecessary cost to the TCP fast path.
> 
> Changes in v2:
> - Exclude SYN and FIN segments from the boundary-old check to preserve
>   existing SYN/FIN handling, including simultaneous connect
>   and retransmitted SYN+ACK AccECN processing.
> 
> 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 | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index daff93d51..10e49e6e1 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4844,8 +4844,12 @@ 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))
> -		return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
> +	if (unlikely(!after(end_seq, tp->rcv_wup))) {

In a connection that sends unidirectionally, this condition is
unlikely only on the receiving side.  On the sending side, all incoming
segments (pure ACKS) fulfill rcv_wup == rcv_nxt == seq == end_seq and
this becomes the likely case. Do we rely on header prediction
filtering out this case here? (this may be worth a comment if so)

> +		if (before(end_seq, tp->rcv_wup) ||
> +		    (seq != end_seq &&
> +		     !(tcp_flag_byte(th) & (TCPHDR_SYN | TCPHDR_FIN))))
> +			return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
> +	}
>  
>  	seq_limit = tp->rcv_nxt + tcp_max_receive_window(tp);
>  	if (unlikely(after(end_seq, seq_limit))) {
> -- 
> 2.43.0
> 
> 

-- 
Simon Baatz <gmbnomis@gmail.com>