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

[netdev-bot+sashiko@kernel.org]

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

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

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] In link_failure_tests(), the third new test ("subflow retry,
  no other subflow") does not reliably check what it claims, that the…

--

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

This adds three tests to link_failure_tests() for the subflow-retries
limit. Netfilter drops the client packets so the subflows are closed and
re-created. The tests check that the re-created subflows connect again,
that a subflow still connecting goes away when the connection is closed,
and that a connection whose only subflow is re-created is still closed
after the close timeout.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c3ccce96b45b1..e80f3f5f99973 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh

[ ... ]

> @@ -2585,6 +2784,119 @@ link_failure_tests()

[ ... ]

> +	# the only subflow is re-created: the connection is still closed after
> +	# the close timeout, the new subflow cannot connect
> +	if reset_with_events "subflow retry, no other subflow" &&
> +	   continue_if has_subflow_retries && init_nft_filter; then
> +		sf_retry_init
> +		pm_nl_flush_endpoint $ns2
> +		ip netns exec $ns2 sysctl -q net.mptcp.close_timeout=5
> +		{ timeout_test=600 test_linkfail=131072 \
> +			run_tests $ns1 $ns2 10.0.1.1 >/dev/null & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		print_check "closed after the close timeout"
> +		if ! wait_counter_ge $((timeout_poll * 4)) \
> +		     $ns1 "MPTcpExtMPCapableACKRX" 1; then
> +			fail_test "connection not established"
> +		elif ! nft_drop_from 10.0.1.2; then
> +			fail_test "unable to drop the packets"
> +		# no other subflow: the first MP_JOIN SYN is the re-created one
> +		elif ! wait_counter_ge "${timeout_poll}" \
> +		     $ns2 "MPTcpExtMPJoinSynTx" 1; then
> +			fail_test "no subflow re-created"
> +		# closed with the connection, after 5 seconds
> +		elif slowwait "${timeout_poll}" no_sf_connecting; then
> +			print_ok

[Severity: Medium]
Could this check pass even when the close timeout never fires?

do_transfer() starts the background client with the usual 30 s I/O
timeout. Setting timeout_test=600 only changes the outer watchdog:

    ./mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \

After 10.0.1.2 is blocked, the client gets no more events. When poll()
times out, copyfd_io_poll() returns 2 and mptcp_connect exits, which
closes the MPTCP socket. __mptcp_close() then calls
mptcp_close_retry_subflows(), and that closes the SYN_SENT retry subflow
without any help from the close timeout:

net/mptcp/protocol.c:mptcp_close_retry_subflows() {
    ...
		if (state != TCP_SYN_SENT && state != TCP_CLOSE)
			continue;
    ...
		__mptcp_close_ssk(sk, ssk, subflow, 0);
}

The client's last event happens at or before the re-created SYN. Its 30 s
poll timeout therefore starts around the time of the block.

The slowwait window here is also timeout_poll (30 s). It only starts once
MPTcpExtMPJoinSynTx >= 1 is seen, about 3-5 s after the block with
tcp_retries2=3. So the client's own close falls inside this window, or
races with its end.

Suppose the re-arm in mptcp_retry_check_tout() were missing or broken:

	if (connecting)
		mptcp_start_tout_timer(sk);

__mptcp_subflow_connect() stops the timer, so the subflow would stay
connecting until the client exits on its own. Wouldn't this check still
print ok, or pass or fail depending on a sub-second race?

The final checks don't tell the two close paths apart either.
__mptcp_close_ssk() emits no SUB_CLOSED event, and the exit status of the
background transfer is never checked.

Could the wait be bounded to close_timeout plus a margin, well below
timeout_poll? Alternatively, the test could check that the client is still
running when the subflow goes away.

> +		else
> +			fail_test "subflow still connecting"
> +		fi
> +		mptcp_lib_kill_group_wait $tests_pid
> +		kill_events_pids
> +
> +		join_syn_tx=1 chk_join_nr 0 0 0
> +		chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1 error 1
> +		sf_retry_cleanup
> +	fi
>  }

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