Re: [PATCH mptcp-net 2/2] selftests: mptcp: join: re-add the initial subflow endp with fullmesh

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: pm: fill_remote_addresses_fullmesh() fails to skip reset initial subflow
--

commit 788b7ee55348ab3d2eca027c9a9093fa76b13dc4
Author: Abdul Wasey <w453y.me@gmail.com>

selftests: mptcp: join: re-add the initial subflow endp with fullmesh

This commit adds a selftest in mptcp_join.sh to validate deleting and
re-adding the endpoint used by the initial subflow when the fullmesh flag
is set.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 18ce7136a2b0e..4a7c5f31fc6e2 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4445,6 +4445,36 @@ endpoint_tests()
>  		chk_rm_nr 4 4
>  	fi
>  
> +	# remove and re-add the endpoint of the initial subflow, with fullmesh
> +	if reset_with_events "delete and re-add, fullmesh" &&
> +	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow,fullmesh
> +		{ timeout_test=120 test_linkfail=128 speed=5 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_event ns2 MPTCP_LIB_EVENT_ESTABLISHED 1
> +		wait_mpj 1
> +		chk_subflow_nr "before delete id 0" 2
> +
> +		pm_nl_del_endpoint $ns2 1 10.0.1.2
> +		sleep 0.5
> +		chk_subflow_nr "after delete id 0" 1
> +
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		wait_mpj 2
> +		chk_subflow_nr "after re-add id 0" 2

[Severity: Medium]
Does fill_remote_addresses_fullmesh() still fail to skip the initial subflow
when msk->first has been disconnected via tcp_disconnect() and reset via
mptcp_subflow_ctx_reset()?

The preceding commit ("mptcp: pm: in-kernel: fullmesh: skip closed subflows")
added subflow->closing checks in fill_remote_addresses_fullmesh() so that
closed subflows are skipped:

net/mptcp/pm_kernel.c:fill_remote_addresses_fullmesh() {
	mptcp_for_each_subflow(msk, subflow) {
		/* The initial subflow stays in the list once closed */
		if (subflow->closing)
			continue;

		if (READ_ONCE(subflow->local_id) == local->id)
			__set_bit(subflow->remote_id, unavail_id);
	}

	mptcp_for_each_subflow(msk, subflow) {
		if (subflow->closing)
			continue;

		ssk = mptcp_subflow_tcp_sock(subflow);
		mptcp_remote_address((struct sock_common *)ssk, &addrs[i]);
...
}

In this test, deleting the endpoint while msk->first is in TCP_ESTABLISHED
causes __mptcp_close_ssk() -> __mptcp_subflow_disconnect() to take the
tcp_shutdown() branch, which leaves subflow->closing set to 1.

However, when the initial subflow (msk->first) reaches TCP_CLOSE prior to
__mptcp_close_ssk() (for example, due to a peer RST, peer FIN closure,
timeout, or TCP_SYN_SENT shutdown in mptcp_subflow_shutdown()) and
!dispose_it holds (!msk->free_first && ssk == msk->first),
__mptcp_close_ssk() sets subflow->closing = 1 and then calls
__mptcp_subflow_disconnect():

net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
	    fastclosing) {
		/* The MPTCP code never wait on the subflow sockets, TCP-level
		 * disconnect should never fail
		 */
		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
		mptcp_subflow_ctx_reset(subflow);
	} else {
		tcp_shutdown(ssk, SEND_SHUTDOWN);
	}
}

Because ssk->sk_state is TCP_CLOSE, tcp_disconnect(ssk, 0) zeroes inet_dport
and inet_daddr, and mptcp_subflow_ctx_reset() is called immediately after:

net/mptcp/protocol.h:mptcp_subflow_ctx_reset() {
	memset(&subflow->reset, 0, sizeof(subflow->reset));
	subflow->request_mptcp = 1;
	WRITE_ONCE(subflow->local_id, -1);
}

Since subflow->closing is inside struct_group(reset, ...),
mptcp_subflow_ctx_reset() zeroes subflow->closing back to 0 and zeroes
subflow->remote_id to 0 while setting subflow->local_id to -1.

When fill_remote_addresses_fullmesh() runs afterward (such as when re-adding
an endpoint with the fullmesh flag in
mptcp_pm_create_subflow_or_signal_addr()), checking if (subflow->closing)
evaluates to false for the disconnected msk->first still in msk->conn_list.
The second loop in fill_remote_addresses_fullmesh() then copies the wiped
remote address (port 0, address 0.0.0.0, id 0) from msk->first, sets bit 0
in unavail_id (causing any subsequent active subflow with remote_id 0 in
msk->conn_list to be skipped), and passes the port-0 address to
__mptcp_subflow_connect(), where kernel_connect() fails with -EINVAL.

> +
> +		mptcp_lib_kill_group_wait $tests_pid
> +		kill_events_pids
> +
> +		chk_join_nr 2 2 2
> +		chk_rm_nr 1 1
> +	fi
> +
>  	# remove and re-add
>  	if reset_with_events "delete re-add signal" &&
>  	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019170751.200446-1-w453y.me@gmail.com?part=2