[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>

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] Incomplete fix: the new old-segment rejection in tcp_sequence()
  (net/ipv4/tcp_input.c:4847-4852) is bypassed by any segment that sets…
- [Low] The block comment documenting tcp_sequence()
  (net/ipv4/tcp_input.c:4827-4838) now contradicts the implementation in…

--

tcp: reject completely old segments during sequence validation

This tightens tcp_sequence() so that a segment consuming sequence space with
end_seq equal to tp->rcv_wup is rejected as completely old, instead of only
rejecting end_seq strictly before rcv_wup.  Segments with SYN or FIN set, and
segments that consume no sequence space, keep the previous behaviour.

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 0f60a1dbf927..37316d07f996 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))))

[Severity: High]
Can a data segment that also sets FIN still slip through this check?

For a segment with N bytes of payload and FIN set, tcp_v4_rcv() (and
tcp_v6_rcv()) compute:

	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
				    skb->len - th->doff * 4);

so end_seq = seq + N + 1.  Choosing seq = rcv_wup - N - 1 gives
end_seq == tp->rcv_wup, which means:

  - the outer !after(end_seq, tp->rcv_wup) test is true,
  - before(end_seq, tp->rcv_wup) is false,
  - seq != end_seq is true, but the mask term
    !(tcp_flag_byte(th) & (TCPHDR_SYN | TCPHDR_FIN)) is false because FIN
    is set.

tcp_sequence() then returns SKB_NOT_DROPPED_YET.  In
tcp_validate_incoming() the segment is not RST and not SYN, so step 2 and
step 4 are skipped and control reaches pass:, after which
tcp_rcv_established() calls:

	reason = tcp_ack(sk, skb, FLAG_SLOWPATH | FLAG_UPDATE_TS_RECENT);

Isn't that exactly the ACK processing of a completely old segment that the
changelog and the packetdrill reproducer aim to prevent, reachable again by
setting one flag bit?  The v2 note says the carve-out exists to
"preserve existing SYN/FIN handling, including simultaneous connect and
retransmitted SYN+ACK AccECN processing", but a retransmitted pure FIN has
end_seq == seq + 1 with no payload, so would restricting the exemption to
segments that consume no payload sequence space be enough, rather than
exempting all FIN-flagged segments including data+FIN?

Note tcp_data_queue() still drops the payload and does not call tcp_fin()
because seq != rcv_nxt, so the residual effect is the ACK processing itself
(snd_una and bytes_acked accounting, RTT and window sampling, congestion
control state) driven by the peer-controlled ack field of an
RFC-unacceptable segment.

[Severity: Low]
Does the block comment above tcp_sequence() still match the code after this
change?  It currently says:

 * Segment controls are considered valid, if the segment
 * fits to the window after truncation to the window. Acceptability
 * of data (and SYN, FIN, of course) is checked separately.
 * See tcp_data_queue(), for example.
 *
 * Also, controls (RST is main one) are accepted using RCV.WUP instead
 * of RCV.NXT.

The new branch inspects tcp_flag_byte(th) and seq != end_seq, so the
data versus SYN/FIN acceptability decision now happens inside
tcp_sequence() rather than only in tcp_data_queue().

The second part mentions RST as the main control accepted at RCV.WUP, but
the exemption mask lists only TCPHDR_SYN | TCPHDR_FIN.  A RST carrying
payload (allowed by RFC 1122 4.2.2.12) has end_seq = seq + len with no +1,
so seq != end_seq and neither SYN nor FIN is set.  With
seq = rcv_wup - len such a RST now returns
SKB_DROP_REASON_TCP_OLD_SEQUENCE, and tcp_validate_incoming() takes:

	} else if (tcp_reset_check(sk, skb)) {
		goto reset;
	}
	goto discard;

so it no longer reaches step 2 and no longer emits the RFC 5961 challenge
ACK it produced before.  Would it be better to update the comment to say
that at the RCV.WUP boundary only zero-length segments and SYN/FIN segments
are accepted, or to add RST to the flag mask?

> +			return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
> +	}
>  
>  	seq_limit = tp->rcv_nxt + tcp_max_receive_window(tp);
>  	if (unlikely(after(end_seq, seq_limit))) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831205611.2439538-1-michael.cohen3%40mail.huji.ac.il