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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] Setup for the new "subflow retry after link failure" test in
  link_failure_tests() depends on timing, and its first gate is never…
- [Low] chk_no_sf_connecting() cannot reliably show that the new
  mptcp_close_retry_subflows() helper works.

--

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

This adds a test to mptcp_join.sh for the subflow-retries limit. While a
long transfer runs in the background, nftables drops the client packets
sent from a subflow address. Once the re-created subflow's SYN has been
retransmitted more times than tcp_syn_retries allows, the packets are let
through and the test checks that the subflow comes back. This is done for
the additional subflow and then the initial one. Finally the connection is
closed while a subflow is being re-created, and the netlink events are
checked.

> 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

[ ... ]

> @@ -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

[ ... ]

> +		# 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]

Can the setup for this test take longer than this first wait_counter()
allows?

With test_linkfail=131072, run_tests() always regenerates both files,
because test_linkfail > 2. Only after that does it call do_transfer():

tools/testing/selftests/net/mptcp/mptcp_join.sh:run_tests() {
    ...
	if [ "$test_linkfail" -gt 2 ]; then
		size=$test_linkfail
		...
		make_file "$cinfail" "client" $size
    ...
	if [ "$test_linkfail" -gt 2 ]; then
		size=$test_linkfail
		...
		make_file "$sinfail" "server" $size
    ...
	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP ${connect_addr}
}

That reads 2 x 128 MiB from /dev/urandom in 1 KiB blocks
(mptcp_lib_make_file() uses dd with bs=1024). All of this runs in the
background subshell before the listener and connector start. The mktemp
calls above only create the file names.

The return value of this wait_counter() call is ignored. File generation
plus connection setup might not fit in the timeout_poll window, for
example on a KASAN or lockdep debug kernel, or under emulation. In that
case sf_retry_after_block 10.0.3.2 fails with "no established subflow
from 10.0.3.2 before the block".

The later phases would then fail in turn. The absolute
wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4 waits out its full timeout,
chk_no_sf_connecting() reports "no subflow re-created before the close",
and chk_join_nr and chk_evt_nr fail.

The commit message says the transfer lasts the whole test "on any
machine", but that covers the transfer, not this setup time.

Would it help to check the result of this first wait, or to generate the
files in the foreground before starting the background job?

> +		sf_retry_after_block 10.0.3.2
> +		sf_retry_after_block 10.0.1.2
> +
> +		ip netns exec $ns2 nft add rule ip filter OUTPUT \
> +			ip saddr 10.0.3.2 drop
> +		wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4
> +		local retrying=$?
> +		mptcp_lib_kill_group_wait $tests_pid
> +		chk_no_sf_connecting ${retrying}

[Severity: Low]

Does chk_no_sf_connecting() actually exercise
mptcp_close_retry_subflows()?

mptcp_lib_kill_group_wait() closes the connection by killing both
mptcp_connect processes in the middle of a bidirectional transfer. If the
client msk still has unread data at that point, __mptcp_close() takes the
fastclose path:

net/mptcp/protocol.c:__mptcp_close() {
    ...
	if (mptcp_data_avail(msk) || timeout < 0 ||
	    (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime)) {
		...
		mptcp_do_fastclose(sk);
		timeout = 0;
	} else if (mptcp_close_state(sk)) {
		__mptcp_wr_shutdown(sk);
	}

	mptcp_close_retry_subflows(sk);
    ...
}

mptcp_do_fastclose() moves the msk to TCP_CLOSE. __mptcp_destroy_sock()
then closes every subflow, including the SYN_SENT retry subflow, whatever
the new helper does.

The same happens if the server is killed first with unread data. It sends
MP_FASTCLOSE, and on the client mptcp_check_fastclose() closes every
subflow not already in TCP_CLOSE, including SYN_SENT ones:

	if (tcp_sk->sk_state != TCP_CLOSE) {
		mptcp_send_active_reset_reason(tcp_sk);
		tcp_set_state(tcp_sk, TCP_CLOSE);
	}

The helper only seems to matter on the graceful close path. Which path a
run takes depends on timing that the test doesn't control.

Could the "no subflow connecting after close" check pass even if
mptcp_close_retry_subflows() were broken or removed? Would a drained,
graceful close make this check deterministic?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016160926.1155262-1-anilkaushikwireless%40gmail.com