Re: [PATCH net-next v2 2/2] selftests: net: add an amt test for UDP_SEGMENT through the relay

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] If a capture fails, its checks pass anyway.
- [Low] The commit message says the test checks that "the relay's egress
  device and the gateway report no drops or checksum errors".
- [Low] wait_tunnel() (amt_gso.sh:126-142) counts the tunnel as up only
  when a relayed, non-GSO probe reaches the listener.
- [Low] The READY handshakes are not enforced.
- [Low] do_send() computes `total = c->cnt * c->seg + c->tail`
  (amt_gso.c:138) in unsigned int and only checks `total > MAX_DGRAM`…

--

selftests: net: add an amt test for UDP_SEGMENT through the relay

This adds amt_gso.sh and its C helper amt_gso. The test sends a
UDP_SEGMENT burst from the relay namespace through an amt relay and
gateway to a listener, with tx offload on the amtr device off and on.

It checks delivery on the listener and uses AF_PACKET captures to check
frame sizes, both those handed to amt_dev_xmit() and those put on the
wire. CONFIG_PACKET is added to the net selftests config.

> diff --git a/tools/testing/selftests/net/amt_gso.c b/tools/testing/selftests/net/amt_gso.c
> new file mode 100644
> index 0000000000000..eebf3ecb94979
> --- /dev/null
> +++ b/tools/testing/selftests/net/amt_gso.c
> @@ -0,0 +1,457 @@

[ ... ]

> +static int do_send(const struct cfg *c)
> +{
> +	static uint8_t buf[MAX_DGRAM];
> +	struct sockaddr_storage dst, src;
> +	socklen_t dlen, slen;
> +	unsigned int total, ifindex, i;
> +	int fd, ttl = 8, zero = 0;
> +
> +	total = c->cnt * c->seg + c->tail;
> +	if (total > MAX_DGRAM ||
> +	    (c->tail && c->tail < sizeof(struct chunk_hdr)) ||
> +	    c->seg < sizeof(struct chunk_hdr))
> +		error(2, 0, "bad sizes");

[Severity: Low]
Can this multiplication wrap before the total > MAX_DGRAM check runs?
cnt, seg, tail and num all come from atoi() with no range checks. For
example, -s 65536 -c 65536 gives a total equal to tail, and -s 1200
-c 3579140 gives 704.

fill_dgram() ignores total and writes cnt chunks of seg bytes into the
static buffer:

	for (i = 0; i < n; i++) {
		unsigned int len = chunk_len(c, i);
		...
		buf += len;
	}

Would that overflow buf[MAX_DGRAM] in do_send()? Also, chunk_hdr.len is
a uint16_t, so any seg above 65535 is silently truncated.

do_recv() has the same pattern:

	unsigned int per = chunks_per_dgram(c), expected = c->num * per;
	...
	seen = calloc(expected ? expected : 1, 1);
	...
	if (seen[h.seq * per + h.idx]++) {

If c->num * per wraps, can seen[] end up too small? The index is only
bounded by h.seq < c->num and h.idx < per, not by the allocation size.

amt_gso.sh only passes small fixed values, so this only applies when the
helper is run by hand with large arguments.

[ ... ]

> diff --git a/tools/testing/selftests/net/amt_gso.sh b/tools/testing/selftests/net/amt_gso.sh
> new file mode 100755
> index 0000000000000..3c37cb59d9b02
> --- /dev/null
> +++ b/tools/testing/selftests/net/amt_gso.sh
> @@ -0,0 +1,269 @@

[ ... ]

> +# Send one single-datagram probe every second until the listener sees it,
> +# which means that discovery, request and update are done for this group.
> +wait_tunnel()
> +{
> +	local fam=$1 v6="" grp=$GRP4 src=$SRC4 port=4999 i
> +
> +	[ "$fam" = 6 ] && { v6=-6; grp=$GRP6; src=$SRC6; port=6999; }
> +
> +	for i in $(seq 40); do
> +		ip netns exec "$LISTENER" $AMT_GSO recv $v6 -p $port \
> +			"${PROBE_OPTS[@]}" -T 1 >"$TMPD/probe.out" &
> +		local pid=$!
> +		busywait 5000 grep -q READY "$TMPD/probe.out"

[Severity: Low]
Can this grep match a stale READY? probe.out is never removed, either
between iterations or between the IPv4 and IPv6 calls. The truncating
open for the redirection happens in the background child after fork.

If the parent's grep runs before the new child has truncated the file
and bound its socket, the probe goes to nobody. recv then waits out its
1 second idle timeout, exits 1, and the iteration is wasted.

The busywait return value is also ignored here and in run_burst():

	for f in recv amtr gw; do
		busywait 5000 grep -q READY "$TMPD/$f.out"
	done

If one of the helpers is not ready within 5 seconds, the burst is sent
anyway. Could that cause a spurious listener failure? Could a sniffer
also get SIGTERM before its handler is installed and exit without a
report?

> +		ip netns exec "$RELAY" $AMT_GSO send $v6 -I amtr -g $grp \
> +			-b $src -p $port "${PROBE_OPTS[@]}"
> +		wait $pid && return 0
> +	done
> +	return 1
> +}

[ ... ]

> +	wait $pid_r
> +	RECV_RC=$?
> +	# The receiver is done, so every frame the captures will see is already
> +	# queued on their sockets. Ask them to drain it and report.
> +	kill -TERM $pid_a $pid_g
> +	wait $pid_a $pid_g

[Severity: Medium]
What happens here if one of the sniffers has already died? The exit
status is discarded. With two PIDs, wait only returns the status of the
last one anyway.

do_sniff() exits through error(2, ...) when recvfrom() fails or poll()
fails with something other than EINTR. It also dies with no output if
SIGTERM arrives before this line runs:

	signal(SIGTERM, sniff_sig);

In that case field() turns the missing key into 0:

	echo "${val:-0}"

so big and lost both read as 0 for the amtr capture.

In the control and plain datagram cases, would check_err $((big != 0))
and the lost check then pass with no capture behind them? In the tx-on
UDP_SEGMENT cases, the result becomes a skip ("no GSO skb reached
amt_dev_xmit()") rather than an error.

A failed gw capture is still caught indirectly by gwn != EXPECT. A
failed amtr capture does not seem to be caught at all.

> +
> +	DROPS=$(($(dev_stat "$RELAY" relay_gw tx_dropped) - DROP0))
> +	TXP=$(($(dev_stat "$RELAY" relay_gw tx_packets) - TXP0))
> +	CSUMERR=$(($(csum_errors) - CSUM0))
> +	GRX=$(($(dev_stat "$GATEWAY" amtg rx_packets) - GRX0))
> +	LRX=$(($(dev_stat "$LISTENER" l_gw rx_packets) - LRX0))
> +	GRXD=$(($(dev_stat "$GATEWAY" amtg rx_dropped) - GRXD0))
> +	EXPECT=$((NUM * (cnt + (tail ? 1 : 0))))
> +}

[ ... ]

> +	stats="relay_gw tx +$TXP drop +$DROPS; gw csum_err +$CSUMERR"
> +	stats="$stats; amtg rx +$GRX drop +$GRXD; listener rx +$LRX"
> +	log_info "$name: $stats"
> +
> +	check_err $(($(field "$TMPD/amtr.out" lost) + \
> +		     $(field "$TMPD/gw.out" lost) != 0)) \
> +		"a packet capture lost frames, the verdict is unreliable"
> +	check_err $RECV_RC "listener did not get every datagram intact"
> +	check_err $((DROPS != 0)) "relay_gw dropped $DROPS packets"
> +	check_err $((CSUMERR != 0)) "gateway counted $CSUMERR csum errors"

[Severity: Low]
The commit message says the test checks:

  "that the relay's egress device and the gateway report no drops or
  checksum errors."

GRXD is sampled in run_burst() but only printed in the log_info line
above. No check_err uses it, so a nonzero rx_dropped on amtg never
fails a case.

Some of those drops have nothing to do with the tested data. For
example, amt_membership_query_handler() in drivers/net/amt.c does this
on a failed query delivery:

	amt->dev->stats.rx_dropped++;

so a case can pass with GRXD > 0. TXP, GRX and LRX are also computed
but only logged.

Should GRXD be passed to check_err, or should the commit message be
reworded to match what is actually checked?

The commit message also only describes the UDP_SEGMENT cases with tx
offload off and on. It does not mention the two "plain datagrams, amt
tx offload on" cases.

[ ... ]

> +setup_topology
> +
> +wait_tunnel 4 || skip_all "IPv4 AMT tunnel did not come up"
> +wait_tunnel 6 || skip_all "IPv6 AMT tunnel did not come up"

[Severity: Low]
wait_tunnel() only succeeds when a relayed probe reaches the listener.
That probe goes through amt_send_multicast_data(). The preceding commit
"amt: mark relay data as a UDP tunnel packet before sending it" makes
that function run this on every relayed packet:

	if (udp_tunnel_handle_offloads(skb, true)) {
		kfree_skb(skb);
		return;
	}

If relay data forwarding breaks completely, every probe is lost and
the whole test exits with KSFT_SKIP. Should that be reported as a
failure instead, given that this is the data path the test is meant to
cover?

The existing amt.sh would still report FAIL for its IPv4/IPv6 multicast
forwarding cases, so the gap is limited to this test's own verdict.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002212458.34247-1-omar%40blockcast.net