Re: [PATCH mptcp-net 1/2] mptcp: pm: in-kernel: fullmesh: skip closed subflows

[sashiko-bot@kernel.org]

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

commit 4834733b36c6118707c74ccafb4f5dae74a92bbf
Author: Abdul Wasey <w453y.me@gmail.com>

mptcp: pm: in-kernel: fullmesh: skip closed subflows

This commit updates fill_remote_addresses_fullmesh() to skip subflows that
have subflow->closing set when marking remote IDs as unavailable and when
selecting remote addresses for fullmesh endpoints. This aims to avoid
relying on closed subflows kept in conn_list, such as the initial subflow.

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 1a77508132354..b7d7520a3290a 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -216,11 +216,19 @@ fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
>  	 * already created by incoming ADD_ADDR
>  	 */
>  	bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
> -	mptcp_for_each_subflow(msk, subflow)
> +	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]);
>  		addrs[i].id = READ_ONCE(subflow->remote_id);

[Severity: Medium]
Is checking subflow->closing sufficient to skip the initial subflow after it
has been disconnected via tcp_disconnect()?

When the initial subflow is closed via __mptcp_close_ssk(), subflow->closing
is set to 1, the subflow remains in conn_list, and
__mptcp_subflow_disconnect() is called. Whenever ssk->sk_state is TCP_CLOSE
(or fastclosing is set), __mptcp_subflow_disconnect() disconnects the socket
and resets the subflow context:

net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
	    fastclosing) {
		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
		mptcp_subflow_ctx_reset(subflow);
	}
}

Because subflow->closing and subflow->remote_id are inside the reset
struct_group, mptcp_subflow_ctx_reset() zeroes both of them back to 0 and
sets local_id to -1:

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);
}

(This is also why mptcp_close_ssk() checks READ_ONCE(subflow->local_id) < 0
in addition to subflow->close_event_done to detect an already disconnected
initial subflow.)

Additionally, if a subflow transitions to TCP_CLOSE (for example via peer RST
or timeout), subflow->closing is not set until __mptcp_close_subflow() runs,
which in mptcp_worker() executes after mptcp_pm_worker().

When fill_remote_addresses_fullmesh() runs after the initial subflow has been
disconnected and reset, won't subflow->closing be 0, causing the second loop
to read the zeroed remote address and port (skc_dport == 0) from msk->first,
mark remote_id 0 as unavailable in unavail_id (skipping subsequent active
subflows with remote_id 0), increment msk->pm.extra_subflows, and pass port 0
to __mptcp_subflow_connect()?

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