Re: [PATCH net-next 6/6] selftests: net: add kselftest for IEEE 802.1CB FRER tc action

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260717095549.10565-3-xiaoliang.yang_1@nxp.com?part=6
---
--- Patch [6]: [PATCH net-next 6/6] selftests: net: add kselftest for IEEE 802.1CB FRER tc action ---
commit af0b752d09a70dfd74c1c5880e3ee38044b284c6
Author: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>

selftests: net: add kselftest for IEEE 802.1CB FRER tc action

Add a TAP-format kselftest script covering the FRER tc action. The tests
exercise shared and individual recover modes using bond, point-to-point,
and relay topologies.

> diff --git a/tools/testing/selftests/net/frer_test.sh b/tools/testing/selftests/net/frer_test.sh
> new file mode 100755
> index 0000000000000..ecd88952f4958
> --- /dev/null
> +++ b/tools/testing/selftests/net/frer_test.sh

[ ... ]

> +cleanup()
> +{
> +	for n in "$NS_TALKER" "$NS_LISTENER" \
> +		"$P2P_NS_SRC" "$P2P_NS_DST" \
> +		"$R_NS_TALKER" "$R_NS_BRIDGE0" "$R_NS_BRIDGE1" "$R_NS_LISTENER"; do
> +		ip netns del "$n" 2>/dev/null || true
> +	done
> +}
> +trap cleanup EXIT

[Severity: Low]
Does this cleanup handler leak temporary packet capture files?

The test allocates files via mktemp /tmp/frer_... earlier, but this trap
handler only removes network namespaces and does not issue an rm for the
pcap files.

[ ... ]

> +capture_start_on()
> +{

[ ... ]

> +	# Wait until tcpdump opens a packet socket (max ~2.5 s).
> +	local tries=0
> +	while [ $tries -lt 50 ]; do
> +		ip netns exec "$ns" grep -q "$iface" /proc/net/packet 2>/dev/null && break

Simon says: I believe the common approach for the issue flagged below
            is to simply "sleep 1".
            Which has the advantage of being simple.

            Otherwise the index can be looked up from sysfs:
	    /sys/class/net/$iface/ifindex

[Severity: Medium]
Will this grep always fail and exhaust the 50-iteration retry limit?

/proc/net/packet exposes the integer ifindex of the interface, not the
string name.

This could cause a 2.5-second block per capture, wasting most of the 4-second
tcpdump timeout window. This leaves only 1.5s for the ping command and could
potentially cause test flakiness.

> +		sleep 0.05
> +		tries=$((tries + 1))
> +	done
> +}
> +
> +capture_stop()
> +{
> +	[ -n "$_CAP_PID" ] || return 0
> +	wait "$_CAP_PID" 2>/dev/null || true

Simon says: The issue flagged below like a design decision,
            I think we can pass on it.

[Severity: Medium]
Could this cause the script to idly block for the remainder of the 4-second
tcpdump timeout?

Because capture_start_on() spawns tcpdump using timeout 4 tcpdump &, running
wait without first sending a termination signal means it waits for the
timeout to expire naturally.

This could add significant delay across the full test suite.

> +	_CAP_PID=""
> +}

[ ... ]

> +test_individual_recover_bond()
> +{

[ ... ]

> +	# Per-slave capture A: verify veth_a1 passes frames; also use this run
> +	# for the overall ping_rc check (do_ping targets bond0->bond1).
> +	pcap_a=$(mktemp /tmp/frer_bond_indv_a_XXXXXX.pcap)
> +	capture_start_on "$NS_LISTENER" "$VETH_A1" "$pcap_a" "icmp"

Simon says: The issue flagged below seems to be a false-positive.

[Severity: Medium]
Will this BPF filter drop the ingress R-TAG frames?

Since tcpdump opens an AF_PACKET socket, it taps ingress frames in
__netif_receive_skb_core() before sch_handle_ingress() is called.

The ingress frame still contains the FRER R-TAG (EtherType 0xF1C1) and would
bypass the icmp (0x0800) filter, potentially leading to 0 captured ingress
ICMP frames and a deterministic test failure.

> +	do_ping || ping_rc=$?
> +	capture_stop

[ ... ]

> +main()
> +{

[ ... ]

> +	ksft_print_cnts
> +
> +	[ "$_ksft_fail" -eq 0 ] && ksft_exit_pass || ksft_exit_fail

[Severity: High]
Can this unconditionally fail the test with a bash syntax error?

When run in the kselftest tree, the script sources kselftest/lib.sh, which
skips the else block that initializes _ksft_fail=0.

Evaluating this with an unset variable triggers an integer expression
expected error and unconditionally takes the ksft_exit_fail path.

> +}
> +
> +main "$@"