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

[netdev-bot+sashiko@kernel.org]

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