Re: [PATCH net-next v7 2/2] selftests: net: add local ECMP rehash test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The test `test_ecmp_dst_rebuild_syncookie_consistency` fails to verify its claim that both endpoints select the same ECMP path.
--

commit f06e95b4d5669d963a1fb7ffc9820d130b26b945
Author: Neil Spring <ntspring@meta.com>
Subject: selftests: net: add local ECMP rehash test

This commit introduces a new selftest script, ecmp_rehash.sh, to verify
TCP local ECMP path re-selection. It includes nine test scenarios covering
various network conditions like SYN/ACK retransmissions, midstream RTOs,
PLB, and dst rebuild consistency with and without syncookies.

> The dst rebuild tests stream data, replace the ECMP route with
> identical nexthops, and verify that traffic stays on the same path.
> The route replace creates a new fib6_info, causing ip6_dst_check()
> to fail on the next transmit and trigger a fresh route lookup via
> inet6_csk_route_socket().  The syncookie variant verifies that the
> server's initial mp_hash (set in cookie_v6_check()) is consistent
> with the client's, so both endpoints select the same ECMP path.

[ ... ]

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

[ ... ]

> +ecmp_dst_rebuild_check()
> +{
> +	local ns_client=$1; shift
> +	local ns_server=$1; shift
> +	local port=$1; shift
> +	local rc=0

[ ... ]

> +	local base0 base1
> +	base0=$(link_tx_packets_get "$ns_client" veth0a)
> +	base1=$(link_tx_packets_get "$ns_client" veth1a)
> +
> +	ip netns exec "$ns_client" timeout 15 socat -u \
> +		OPEN:/dev/zero \
> +		"TCP6:[fd00:ff::2]:$port,bind=[fd00:ff::1]" \
> +		&>/dev/null &
> +	local client_pid=$!
> +
> +	# Wait for enough packets to identify the active path.
> +	# Return 2 for setup failure (distinct from 1 = path changed).
> +	if ! busywait "$BUSYWAIT_TIMEOUT" until_counter_is \
> +			">= $((base0 + base1 + 50))" \
> +		link_tx_packets_total "$ns_client" > /dev/null; then

[ ... ]

> +	local mid0 mid1 active_dev inactive_dev
> +	mid0=$(link_tx_packets_get "$ns_client" veth0a)
> +	mid1=$(link_tx_packets_get "$ns_client" veth1a)

[ ... ]

> +	local active_after inactive_after
> +	active_after=$(link_tx_packets_get "$ns_client" "$active_dev")
> +	inactive_after=$(link_tx_packets_get "$ns_client" "$inactive_dev")

[Severity: Medium]
Does this test actually verify that both endpoints select the same ECMP
path as described in the commit message?

The test implementation in ecmp_dst_rebuild_check() only monitors the
client's TX statistics (veth0a and veth1a in $ns_client). It never appears
to inspect the server's TX statistics (veth0b and veth1b in $ns_server)
or the client's RX statistics to determine which path the server used
to send ACKs.

Since both ECMP paths are open during this test, the connection will
succeed regardless of the server's path choice. Should this test also
verify the server's TX statistics to ensure the syncookie consistency
claim is actually validated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520064310.4154268-1-ntspring@meta.com?part=2