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

[Simon Baatz <gmbnomis@gmail.com>]

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>