[PATCH net v2 0/2] tcp: exclude old ACKs from fast path

[PATCH net v2 0/2] tcp: exclude old ACKs from fast path

[Inbal Schussheim]

Exclude ACKs outside [SND.UNA, SND.NXT] from TCP header prediction so
that they fall through to the slow path, where ACK
validation is applied.

Add a packetdrill test for a data segment carrying an
excessively old ACK. The test fails on the unpatched kernel and passes
with the fix.

Changes in v2:

- Exclude all ACKs before SND.UNA from header prediction instead of
  duplicating old-ACK validation in the fast path.
- Cover pure ACKs as well as data-carrying segments.
- Let the existing slow path handle validation and challenge ACKs.
- Remove the packetdrill reproducer from the commit message; add the
  packetdrill test separately.

v1: https://lore.kernel.org/netdev/20260906123151.1391349-1-inbal.lipshtat@mail.huji.ac.il/T/#u

Inbal Schussheim (2):
  tcp: exclude old ACKs from tcp fast path
  selftests: net: packetdrill: test exclusion of old ACK from TCP fast
    path

 net/ipv4/tcp_input.c                          |  2 +-
 .../tcp_rfc5961_reject-old-ack.pkt            | 23 +++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt

-- 
2.43.0

[PATCH net v2 1/2] tcp: exclude old ACKs from tcp fast path

[Inbal Schussheim]

Exclude old ACKs before SND.UNA from the tcp fast path
as well as ACKs after SND.NXT.

Such ACKs will fall through to the slow path, where tcp_ack()
performs the appropriate validation and challenge ACK handling
according to RFC5961 and Commit 3d501dd326fb1c7 ("tcp: do not
accept ACK of bytes we never sent").

This prevents old ACKs from being accepted
or modifying connection state as part of the fast path before
appropriate ACK validation is applied.

Reported-by: Amit Klein <amit.klein@mail.huji.ac.il>
Reported-by: Tamir Shahar <tamir.shahar1@mail.huji.ac.il>
Reported-by: Inbal Schussheim <inbal.lipshtat@mail.huji.ac.il>
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Inbal Schussheim <inbal.lipshtat@mail.huji.ac.il>
---
 net/ipv4/tcp_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index daff93d51342..57fec58a4f15 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6539,7 +6539,7 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
 
 	if ((tcp_flag_word(th) & TCP_HP_BITS) == tp->pred_flags &&
 	    TCP_SKB_CB(skb)->seq == tp->rcv_nxt &&
-	    !after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) {
+	    between(TCP_SKB_CB(skb)->ack_seq, tp->snd_una, tp->snd_nxt)) {
 		int tcp_header_len = tp->tcp_header_len;
 		s32 delta = 0;
 		int flag = 0;
-- 
2.43.0

[PATCH net v2 2/2] selftests: net: packetdrill: test exclusion of old ACK from TCP fast path

[Inbal Schussheim]

Add a packetdrill test for an in-sequence data segment carrying an
excessively old ACK.

Verify that the segment falls through from the TCP fast path to the slow
path, where the existing ACK validation rejects it and sends a challenge
ACK. The payload is not accepted and RCV.NXT remains unchanged.

Based on the reproducer from Commit 3d501dd326fb
("tcp: do not accept ACK of bytes we never sent").

Signed-off-by: Inbal Schussheim <inbal.lipshtat@mail.huji.ac.il>
---
 .../tcp_rfc5961_reject-old-ack.pkt            | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt

diff --git a/tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt b/tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt
new file mode 100644
index 000000000000..8e7229127670
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt
@@ -0,0 +1,23 @@
+// Test rejection of data segments carrying excessively old ACKs
+
+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
+
+// ---------------- Handshake ------------------- //
++0 < S 0:0(0) win 65535
++0 > S. 0:0(0) ack 1 <...>
++0 < . 1:1(0) ack 1 win 65535
++0 accept(3, ..., ...) = 4
+
+// Populate receive memory so the following segment can use
+// header prediction.
++0 < P. 1:501(500) ack 1 win 65535
++0 > . 1:1(0) ack 501
+
+// Send an in-sequence data segment carrying an excessively old ACK.
++0 < P. 501:1501(1000) ack 2794967397 win 65535
+
+// Challenge ACK; RCV.NXT must remain 501.
++0 > . 1:1(0) ack 501
-- 
2.43.0

Re: [PATCH net v2 1/2] tcp: exclude old ACKs from tcp fast path

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] The patch fixes a live, security-relevant input-validation defect
  (three Reported-by tags, RFC5961 reference) but carries no Fixes: tag,…
- [Low] The kerneldoc-style block comment above tcp_rcv_established()
  (net/ipv4/tcp_input.c:6477-6499) enumerates the conditions that…

--

tcp: exclude old ACKs from tcp fast path

This changes the header prediction test in tcp_rcv_established() so that
segments whose ack_seq is below SND.UNA no longer enter the fast path, in
addition to the existing rejection of ack_seq above SND.NXT.  Such segments
now fall through to the slow path where tcp_ack() applies the RFC5961
validation and challenge ACK handling.

No Link: tags are present in the commit header.

Two notes below.

[Severity: Low]
The changelog refers to the validation added by commit 3d501dd326fb1c7
("tcp: do not accept ACK of bytes we never sent") in prose:

    Such ACKs will fall through to the slow path, where tcp_ack()
    performs the appropriate validation and challenge ACK handling
    according to RFC5961 and Commit 3d501dd326fb1c7 ("tcp: do not
    accept ACK of bytes we never sent").

Should this carry a Fixes: tag as well, so stable tooling can pick it up?

git blame on the replaced predicate in tcp_rcv_established() points at
31770e34e43d6 ("tcp: Revert \"tcp: remove header prediction\""), and
3d501dd326fb is the commit that added the too-old-ACK check the fast path
was bypassing, so either looks like a concrete candidate.

There are three Reported-by tags but no Link: to the report, and the
changelog does not state the user-visible impact.  Could both be added?

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 0f60a1dbf927..89f9f3913b52 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6539,7 +6539,7 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
>  
>  	if ((tcp_flag_word(th) & TCP_HP_BITS) == tp->pred_flags &&
>  	    TCP_SKB_CB(skb)->seq == tp->rcv_nxt &&
> -	    !after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) {
> +	    between(TCP_SKB_CB(skb)->ack_seq, tp->snd_una, tp->snd_nxt)) {

[Severity: Low]
Should the block comment above tcp_rcv_established() be updated for this
new rejection class?  It enumerates the cases that disable the fast path
and then claims the enumeration is complete:

 *	- Unexpected TCP flags/window values/header lengths are received
 *	  (detected by checking the TCP header against pred_flags)
 *	- Data is sent in both directions. Fast path only supports pure senders
 *	  or pure receivers (this means either the sequence number or the ack
 *	  value must stay constant)
 *	- Unexpected TCP option.
 *
 *	When these conditions are not satisfied it drops into a standard
 *	receive procedure patterned after RFC793 to handle all cases.
 *	The first three cases are guaranteed by proper pred_flags setting,
 *	the rest is checked inline. Fast processing is turned on in
 *	tcp_data_queue when everything is OK.

Nothing in that list mentions ACK sequence validity.  After this change, a
reordered or duplicated pure ACK with ack_seq strictly below SND.UNA takes
the slow path on a plain pure-sender/pure-receiver connection with matching
pred_flags, which the list above says would be fast-pathed.  A bullet for
ack_seq outside [SND.UNA, SND.NXT] would keep the comment in agreement with
the predicate.

>  		int tcp_header_len = tp->tcp_header_len;
>  		s32 delta = 0;
>  		int flag = 0;

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

Re: [PATCH net v2 2/2] selftests: net: packetdrill: test exclusion of old ACK from TCP fast path

[netdev-bot+sashiko]

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

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

- [Medium] tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-
  old-ack.pkt does not run the suite's `\`./defaults.sh\`` setup block.
- [Medium] The companion kernel patch in this series, commit "tcp:
  exclude old ACKs from tcp fast path" (net/ipv4/tcp_input.c, changing…
- [Low] The new file
  tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt…

--

selftests: net: packetdrill: test exclusion of old ACK from TCP fast path

This adds a new packetdrill script that feeds an in-sequence data segment
carrying an excessively old ACK and expects the connection to fall out of
the header prediction fast path into the slow path, where the RFC 5961 5.2
check drops the payload and emits a challenge ACK.  No Link: tags are
present in the commit header.

A few questions below, one about the companion kernel patch metadata and
two about the new script itself.

> selftests: net: packetdrill: test exclusion of old ACK from TCP fast path

[Severity: Medium]
A question about the companion patch in this series rather than this test:
"tcp: exclude old ACKs from tcp fast path" changes the header prediction
predicate in tcp_rcv_established() from

	!after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)

to

	between(TCP_SKB_CB(skb)->ack_seq, tp->snd_una, tp->snd_nxt)

but carries neither a Fixes: tag nor Cc: stable@vger.kernel.org, even
though it has Reported-by: credits from external researchers.

Before that change, an in-sequence data segment with an arbitrarily old
ACK field satisfied the fast path predicate, and in the fast path the
payload was queued and RCV.NXT advanced before the ACK was validated:

net/ipv4/tcp_input.c:tcp_rcv_established() {
	...
	eaten = tcp_queue_rcv(sk, skb, &fragstolen);

	tcp_event_data_recv(sk, skb);

	if (TCP_SKB_CB(skb)->ack_seq != tp->snd_una) {
		/* Well, only one small jumplet in fast path... */
		tcp_ack(sk, skb, flag | FLAG_DATA);
	...
}

The negative return from tcp_ack() (-SKB_DROP_REASON_TCP_TOO_OLD_ACK,
produced by the RFC 5961 5.2 branch) was ignored there, so the data was
already accepted.  Since the input is remote packet data, should the
companion patch pick up a stable tag?  git blame points at 31770e34e43d6
("tcp: Revert \"tcp: remove header prediction\"") as the Fixes: candidate
for the predicate.

> diff --git a/tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt b/tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt
> new file mode 100644
> index 000000000000..8e7229127670
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp_rfc5961_reject-old-ack.pkt
> @@ -0,0 +1,23 @@
> +// Test rejection of data segments carrying excessively old ACKs
> +
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3

[Severity: Medium]
Should this script source the suite's setup block before the socket call?
Every other .pkt file in tools/testing/selftests/net/packetdrill/ starts
with it, for example tcp_rfc5961_ack-out-of-window.pkt:

	`./defaults.sh
	sysctl -q net.ipv4.tcp_invalid_ratelimit=0
	`

ksft_runner.sh does no sysctl or qdisc setup of its own:

	unshare -n packetdrill ${ip_args[$ip_version]} ${optargs[@]} $script > /dev/null \

so without the block the test runs with host-inherited values for
net.core.default_qdisc, net.ipv4.tcp_rmem/tcp_wmem, tcp_ecn,
tcp_timestamps, tcp_congestion_control and tcp_slow_start_after_idle, and
tun0 never gets the explicit pfifo that defaults.sh installs:

	# Override the default qdisc on the tun device.
	# Many tests fail with timing errors if the default
	# is FQ and that paces their flows.
	tc qdisc add dev tun0 root pfifo

Both expected outbound packets here are asserted at +0, and the receive
memory sysctls feed into the sk_rcvbuf / sk_forward_alloc state that the
500-byte priming segment is there to establish.  Can the result of this
test vary with the host configuration as written?

Also, is tcp_invalid_ratelimit relevant here?  The sibling RFC 5961 test
pins it to 0 before expecting a challenge ACK.

[Severity: Low]
This isn't a bug, but the new file has no SPDX-License-Identifier line.
Both sibling RFC 5961 scripts start with

	// SPDX-License-Identifier: GPL-2.0

(tcp_rfc5961_ack-out-of-window.pkt and tcp_rfc5961_rst-syn-recv.pkt), as
do 105 of the .pkt scripts in that directory, though the tcp_accecn_*.pkt
group does not.  Documentation/process/license-rules.rst asks for the tag
in new files.

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

[ ... ]

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