Re: [PATCH mptcp-next v5 7/7] selftests: mptcp: join: validate subflow retry after link failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/mptcp: sf_from() extracts Send-Q instead of local address and port
- [Medium] selftests/mptcp: hand-rolled busy-wait polling loops instead of shared helpers
- [Medium] selftests: mptcp: background file generation race causes flaky timeout
--

commit f8121441934e1bbc20d48bed94f57afe881c5d82
Author: Anil Kaushik <anilkaushikwireless@gmail.com>

selftests: mptcp: join: validate subflow retry after link failure

Test the subflow-retries limit, with its default value, and a long
connection in the background.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f4d8afaeda4f3..521f95d3027fa 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2151,6 +2151,102 @@ wait_attempt_fail()
>  	return 1
>  }
>  
> +# $1: ns ; $2: counter ; $3: expected minimum value
> +wait_counter()
> +{
> +	local cnt
> +
> +	for _ in $(seq $((timeout_poll * 10))); do
> +		cnt=$(mptcp_lib_get_counter "${1}" "${2}")
> +		[ "${cnt:-0}" -ge "${3}" ] && return 0
> +		sleep 0.1
> +	done
> +
> +	return 1
> +}

[Severity: Medium]
Does this hand-roll a busy-wait polling loop instead of using the shared
helpers?

The networking selftests library (tools/testing/selftests/net/lib.sh) already
provides busywait_for_counter specifically to handle this pattern robustly.

> +
> +# $1: client address ; prints the local address and port of its subflow
> +sf_from()
> +{
> +	ip netns exec $ns2 ss -tnH state established src "${1}" |
> +		awk '{ print $3; exit }'
> +}

[Severity: Medium]
Does this extract the Send-Q column instead of the Local Address:Port column?

The command ss -tnH state established outputs columns: State, Recv-Q, Send-Q,
Local Address:Port, Peer Address:Port. So the 3rd column is Send-Q, rather than
Local Address:Port.

Because the previously blocked subflow has queued, dropped packets, its Send-Q
is non-zero. The newly created subflow has an empty Send-Q (0). The test logic
later evaluates [ "${after}" = "${before}" ] as [ "0" = "<non-zero>" ],
which evaluates to false, causing the test to pass by coincidence without ever
verifying that the new subflow originated from a different port.

[ ... ]

> +
> +# $1: result of the wait for the subflow being re-created before the close
> +chk_no_sf_connecting()
> +{
> +	print_check "no subflow connecting after close"
> +	if [ "${1}" -ne 0 ]; then
> +		fail_test "no subflow re-created before the close"
> +		return
> +	fi
> +
> +	# closed with the MPTCP socket
> +	for _ in $(seq $((timeout_poll * 10))); do
> +		if ! ip netns exec $ns2 ss -tnH state syn-sent | grep -q .; then
> +			print_ok
> +			return
> +		fi
> +		sleep 0.1
> +	done
> +
> +	fail_test "subflow still connecting after close"
> +}

[Severity: Medium]
Does this hand-roll a busy-wait polling loop instead of using the shared
helpers?

The networking selftests library (tools/testing/selftests/net/lib.sh) already
provides busywait to handle this pattern robustly.

[ ... ]

> @@ -2585,6 +2681,62 @@ link_failure_tests()
>  		chk_stale_nr $ns2 1 -1 2
>  		chk_link_usage $ns2 ns2eth3 $cinsent 50
>  	fi
> +
> +	# subflows closed after a silent link failure, then re-created: the
> +	# additional one, then the initial one. Then close the connection
> +	# while a subflow is being re-created.
> +	if reset_with_events "subflow retry after link failure" &&
> +	   continue_if has_subflow_retries; then
> +		init_nftables
> +		# slow links, short queues: the transfer lasts the whole test on
> +		# any machine, and the blocked subflow always has data in flight
> +		local i
> +		for i in 1 3; do
> +			tc -n $ns1 qdisc add dev ns1eth$i root netem \
> +				rate 4mbit limit 30 delay 1ms
> +			tc -n $ns2 qdisc add dev ns2eth$i root netem \
> +				rate 4mbit limit 30 delay 1ms
> +		done
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_linear_timeouts=0
> +		pm_nl_set_limits $ns1 0 4
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
> +		# created here, not in the background job, for cleanup()
> +		if [ -z "$cinfail" ]; then
> +			cinfail=$(mktemp)
> +		fi
> +		if [ -z "$sinfail" ]; then
> +			sinfail=$(mktemp)
> +		fi
> +		# killed at the end: the timeout only stops a stuck test
> +		{ timeout_test=600 test_linkfail=131072 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_counter $ns1 "MPTcpExtMPJoinAckRx" 1

[Severity: Medium]
Could this foreground polling loop time out before the background file
generation completes?

The test case sets test_linkfail=131072 and launches run_tests in the
background. Inside run_tests, it sequentially generates 128MB of random data
per file via dd, blocking the network transfer from starting until both files
are fully generated.

Meanwhile, the foreground script immediately calls wait_counter, which polls
for the MPTcpExtMPJoinAckRx counter to increment with a 30-second timeout.
If the dd commands take longer than 30 seconds on a slow test runner, the
foreground script will time out and fail the test before the actual connection
has even started.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016160926.1155262-1-anilkaushikwireless@gmail.com?part=7