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

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

Critical: 0 · High: 0 · Medium: 2 · 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] In amt_gso.sh, a capture helper that fails is not noticed.
- [Medium] The READY handshake in amt_gso.sh does not actually make the
  sender wait for the listeners.
- [Low] The new test depends on AF_PACKET, but CONFIG_PACKET is not in
  the net selftests config fragment.
- [Low] If the AMT tunnel never comes up, the script reports SKIP and
  exits, so a regression in AMT discovery/request/update becomes a skip…
- [Low] In do_send() (amt_gso.c:148), `total = c->cnt * c->seg + c->tail`
  is computed in unsigned int and only then compared with MAX_DGRAM.
- [Low] wait_tunnel() (amt_gso.sh:124-130) reuses $TMPD/probe.out on
  every iteration and for both families, and never clears it in the…

--

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

This adds amt_gso.sh and a C helper, amt_gso. The test sends a
UDP_SEGMENT burst from a sender on the AMT relay to a listener behind the
gateway, over IPv4 and IPv6. It checks payloads and drop and checksum
counters, and uses AF_PACKET captures on amtr and gw_relay to check frame
sizes and AMT message counts.

[ ... ]

[Severity: Low]
This isn't a bug for the values amt_gso.sh passes, but can this product
wrap before it is compared with MAX_DGRAM?

total is an unsigned int, so arguments such as -c 65536 -s 65536 give a
small total that passes the check. fill_dgram() then writes the full
cnt * seg + tail bytes into the 65507-byte static buf[].

do_recv() follows 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 expected wraps, seen[] is allocated too small and this index can write
past it.

This needs large values passed by hand. amt_gso.sh only uses small fixed
sizes.

[ ... ]

[Severity: Low]
Should CONFIG_PACKET be added to tools/testing/selftests/net/config?

Both captures in run_burst() need an AF_PACKET socket here, and every
run_case() verdict depends on them. The config fragment lists
CONFIG_AMT=m, CONFIG_BRIDGE=y and CONFIG_VETH=y, but not CONFIG_PACKET.

On a kernel built from that fragment without CONFIG_PACKET, both sniffers
exit through die() before printing READY. The script then ends up in the
empty field() handling in run_case() described below, and does not skip
cleanly.

[ ... ]

[ ... ]

[Severity: Low]
Can this grep match a READY line left in probe.out by an earlier receiver?

probe.out is reused on every iteration and for both families. The
truncating redirection happens in the background child. If the parent's
grep runs before the child has opened the file, it sees the READY from an
earlier failed attempt, or from the IPv4 run when wait_tunnel 6 starts.

The probe is then sent before the new receiver has bound its socket. The
probe is lost and the loop retries. In the worst case, repeated losses end
in the "tunnel did not come up" skip.

run_burst() avoids this by running rm -f before it starts its helpers.
Could the same be done here?

+		ip netns exec "$RELAY" $AMT_GSO send $v6 -I amtr -g $grp \
+			-b $src -p $port $PROBE_OPTS >/dev/null

[ ... ]

[Severity: Medium]
What happens when one of these busywait calls times out?

busywait() in lib.sh returns 1 after the timeout. That result is ignored
here and in wait_tunnel(), so the counters are sampled and the sender
starts anyway.

If a capture has not bound its AF_PACKET socket yet, frames can cross
amtr or gw_relay without being seen. That would give a spurious
gwn != EXPECT failure. It could also leave big at 0, which turns a tx on
GSO case into a skip.

The commit message says slow helper startup on a loaded guest caused the
earlier flakiness.

There is a related ordering question later in run_burst():

	kill -TERM $pid_a $pid_g
	wait $pid_a $pid_g

do_sniff() installs its handler just before printing READY:

	signal(SIGTERM, sniff_sig);
	printf("READY\n");

A sniffer that is still starting up is killed by the default action and
prints no SNIFF line. run_case() then works with empty fields, as
described below. Since wait discards the exit status, a helper that never
started is not reported either.

[ ... ]

[ ... ]

[Severity: Medium]
How are these checks meant to behave when a sniffer exited without
printing its SNIFF line?

do_sniff() prints SNIFF only at the very end. Every die() path leaves the
output file empty: socket(AF_PACKET), if_nametoindex, bind, poll and
recvfrom. So does a SIGTERM that arrives before the handler is installed.

run_burst() ignores both the busywait results and the exit status from
wait $pid_a $pid_g. In that case field() returns an empty string.

If gw.out has no SNIFF line, the first check_err expands to something
like:

	$((0 +  != 0))

That is a bash arithmetic syntax error. The over_mtu check on gw.out has
the same problem.

Bash then discards the whole top-level run_case command. log_test is never
reached, no TEST line is printed, and EXIT_STATUS is not updated.

With CONFIG_PACKET=n, both sniffers die in socket(AF_PACKET) and all six
cases are dropped this way. wait_tunnel() still succeeds because it does
not use the sniffer, so the script exits with 0.

If only amtr.out has no SNIFF line, the first expression is still valid
and gives 0, but big is empty. In the tx on GSO cases, this test is then
false:

	if [ "$big" = 0 ] && [ $RET -eq 0 ]; then

So the "no GSO skb reached amt_dev_xmit()" skip is never taken, and the
case can pass without showing that a GSO skb reached the driver.

In the other cases, $((big != 0)) treats the empty variable as 0 and
passes with no data behind it.

Would it make sense for run_burst() to check the busywait results and the
sniffers' exit status, and for run_case() to fail when a SNIFF line is
missing?

[ ... ]

[Severity: Low]
Should this be reported as a failure rather than a skip?

By this point the script has checked that iproute2 supports amt, and
setup_topology() has created both amt devices. If 40 probes then fail to
reach the listener, no prerequisite is missing. Discovery, request or
update is not working.

As written, an AMT forwarding regression shows up as SKIP and none of the
six cases run. amt.sh reports [FAIL] in the same situation.

[ ... ]

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