Re: [PATCH nf-next v5 3/3] selftests: netfilter: nft_flowtable.sh: roam a host between two bridge ports

[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] In test_bridge_roam() the two readiness barriers are decision
  points whose failure is ignored: `busywait 1000 roam_listener_ready`…
- [Medium] Both offload assertions in test_bridge_roam() are one-sided
  upper bounds on the reply-direction forward-chain counter (`[…
- [Low] The listener socat started in $rns2 (nft_flowtable.sh:944-945) is
  backgrounded but its PID is never captured, so no exit path of…

--

selftests: netfilter: nft_flowtable.sh: roam a host between two bridge ports

This adds test_bridge_roam() to nft_flowtable.sh, which moves a host
bridge between two ports of a router bridge while a tcp flow is
offloaded, and checks that the transfer resumes and stays on the fast
path.  It is run twice, once with plain veth bridge ports and once with
vlan devices, and it adds CONFIG_NET_SWITCHDEV to the netfilter config.

A few questions about the new test below.

> diff --git a/tools/testing/selftests/net/netfilter/nft_flowtable.sh b/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> index 449c518bd947..0d9f7d771fac 100755
> --- a/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> +++ b/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> @@ -851,6 +851,146 @@ test_ipip
>  
>  test_bridge
>  
> +# Roam a host between two ports of a bridge on the router while its flow is
> +# offloaded, as a station moving between two access points attached to ports
> +# of one bridge. The host keeps its address and mac on a bridge of its own
> +# and roams by moving that bridge's port, so only the fdb changes. With a vid
> +# the bridge ports are vlan devices, whose ifindex differs from the device
> +# the flow transmits on. A flow that stays on the fast path leaves no more
> +# than a few packets in the forward chain, the counter tells offloaded from
> +# slow path.
> +test_bridge_roam()
> +{

[ ... namespace, veth, vlan, bridge and ruleset setup snipped ... ]

> +	# the host discards what it receives, its socket byte counter is read
> +	rx_bytes() { ip netns exec "$rns1" ss -tin "dst 10.9.2.99" 2>/dev/null |
> +		grep -o 'bytes_received:[0-9]*' | head -1 | cut -d: -f2; }
> +	rx_started() { local n; n=$(rx_bytes); [ "${n:-0}" -gt 0 ]; }
> +	# forward chain packets of the reply direction since the last call
> +	slow_pkts() { ip netns exec "$rnsr" nft reset counter inet filter roam_repl |
> +		grep -o 'packets [0-9]*' | cut -d' ' -f2; }
> +	roam_listener_ready() { ss -N "$rns2" -lnt -o "sport = :12345" | grep -q 12345; }
> +
> +	timeout "$SOCAT_TIMEOUT" ip netns exec "$rns2" socat -u \
> +		OPEN:/dev/zero TCP-LISTEN:12345,reuseaddr &

[Severity: Low]
The listener socat in $rns2 is backgrounded but its pid is not saved, so
no exit path of test_bridge_roam() can stop it.  Only the client pid is
kept in $cpid, and neither child is ever waited for.  Elsewhere in this
same file the listener pid is captured and reaped:

	timeout "$SOCAT_TIMEOUT" ip netns exec "$nsb" socat ... &
	lpid=$!
	...
	wait $lpid

Would it be worth following that pattern here?

The leftover process does get reclaimed, since cleanup_ns() in
tools/testing/selftests/net/lib.sh does

	ip netns pids "${ns}" 2> /dev/null | xargs -r kill || true

and that runs both from setup_ns() when the second call test_bridge_roam 100
reuses the names, and from the EXIT trap via cleanup_all_ns().  Each socat
is also bounded by timeout "$SOCAT_TIMEOUT".  So this is hygiene rather
than a leak.

> +	busywait 1000 roam_listener_ready
> +	timeout "$SOCAT_TIMEOUT" ip netns exec "$rns1" socat -u \
> +		TCP:10.9.2.99:12345 OPEN:/dev/null &
> +	cpid=$!
> +
> +	busywait "$BUSYWAIT_TIMEOUT" rx_started

[Severity: Medium]
Can a setup failure here be reported as a flowtable regression?  The exit
status of both readiness barriers is discarded.

busywait() in tools/testing/selftests/net/lib.sh does return the status of
loopy_wait():

	busywait()
	{
		local timeout_ms=$1; shift

		loopy_wait : "$timeout_ms" "$@"
	}

If the listener is not bound within the hard-coded 1000 ms, the client
socat is started anyway and exits immediately on ECONNREFUSED with no
retry.  busywait "$BUSYWAIT_TIMEOUT" rx_started then also times out
unnoticed, and the test continues to measure a connection that never
existed.

With no traffic at all, slow_pkts() returns 0, so the pre-roam check
[ "${slow:-0}" -gt 100 ] is false and control falls through to the roam.
Afterwards old=$(rx_bytes) and new=$(rx_bytes) are both the empty string,
so [ "${new:-0}" -le "${old:-0}" ] evaluates 0 -le 0 and the script sets
ret=1 with "FAIL: ... transfer stalled after the roam".

Would it be better to check these two busywait calls and report an error
or a skip when the stream never comes up, rather than letting a broken
precondition turn into a fail attributed to the roam path?

> +	sleep 2
> +	slow_pkts > /dev/null
> +	sleep 2
> +	slow=$(slow_pkts)
> +	if [ "${slow:-0}" -gt 100 ]; then
> +		echo "FAIL: $what: $slow forward chain packets before the roam" 1>&2
> +		ret=1
> +		kill "$cpid" 2>/dev/null
> +		return
> +	fi
> +
> +	# roam: move the host bridge to the other port and send on it
> +	ip -net "$rns1" link set "$p0" nomaster
> +	ip -net "$rns1" link set "$p1" master stbr
> +	ip netns exec "$rns1" ping -c 2 -W 1 -q 10.9.1.1 >/dev/null 2>&1
> +
> +	# the bytes in flight at the roam drain within the first seconds; a
> +	# stale flow delivers nothing after that, a torn down one resumes and
> +	# is offloaded again.
> +	sleep 4
> +	old=$(rx_bytes)
> +	sleep 4
> +	new=$(rx_bytes)
> +	slow_pkts > /dev/null
> +	sleep 2
> +	slow=$(slow_pkts)
> +	kill "$cpid" 2>/dev/null
> +
> +	if [ "${new:-0}" -le "${old:-0}" ]; then
> +		echo "FAIL: $what: transfer stalled after the roam" 1>&2
> +		ret=1
> +	elif [ "${slow:-0}" -gt 100 ]; then
> +		echo "FAIL: $what: $slow forward chain packets after the roam" 1>&2
> +		ret=1
> +	else
> +		echo "PASS: flow offload for $what"
> +	fi

[Severity: Medium]
Both offload assertions in test_bridge_roam() are one-sided upper bounds
on the reply-direction counter, [ "${slow:-0}" -gt 100 ], with nothing
tying them to traffic actually having been observed.  A reading of 0
packets is accepted as evidence of offload.

slow_pkts() is a pipeline and the script does not set pipefail:

	slow_pkts() { ip netns exec "$rnsr" nft reset counter inet filter roam_repl |
		grep -o 'packets [0-9]*' | cut -d' ' -f2; }

so a failing nft reset counter, or a grep that matches nothing, yields an
empty string which ${slow:-0} silently turns into 0.

The progress check is similarly weak: [ "${new:-0}" -le "${old:-0}" ] is
satisfied by a single byte of advance over the eight seconds around the
roam.

The commit message says the test "checks that the flow was offloaded
before the roam".  Since that check passes when no flow exists and when
the counter read fails, could a lower bound be added, for example
requiring that the pre-roam byte counter advanced by a meaningful amount
and that the counter read succeeded, before concluding the flow was on the
fast path?

> +}
> +
> +test_bridge_roam ""
> +test_bridge_roam 100

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914072541.897040-1-julius%40bairaktaris.de