Re: [PATCH mptcp-next v7 4/6] mptcp: pm: in-kernel: retry subflows closed after an error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: __mptcp_close() disconnects retry subflows before closing them
--

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

mptcp: pm: in-kernel: retry subflows closed after an error

This commit teaches the in-kernel path manager to re-create established
subflows that were initiated locally and closed after a path error such
as a retransmission or keepalive timeout. It also closes any re-created
subflows that are still connecting when the MPTCP socket is closed.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 60cbd91be6320..5cee1ed788caa 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -3494,6 +3499,26 @@ static void mptcp_check_listen_stop(struct sock *sk)
>  	release_sock(ssk);
>  }
>  
> +/* Re-created subflows still connecting are useless once the msk is closed,
> + * and their SYN retransmissions could keep them around for a long time.
> + */
> +static void mptcp_close_retry_subflows(struct sock *sk)
> +{
> +	struct mptcp_subflow_context *subflow, *tmp;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +
> +	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		if (!subflow->retry || inet_sk_state_load(ssk) != TCP_SYN_SENT)
> +			continue;
> +
> +		/* it never carried data: nothing to push or re-inject */
> +		mptcp_pm_subflow_check_next(msk, subflow);
> +		__mptcp_close_ssk(sk, ssk, subflow, 0);
> +	}
> +}
> +
>  bool __mptcp_close(struct sock *sk, long timeout)
>  {
>  	struct mptcp_subflow_context *subflow;
> @@ -3520,6 +3545,8 @@ bool __mptcp_close(struct sock *sk, long timeout)
>  		__mptcp_wr_shutdown(sk);
>  	}
>  
> +	mptcp_close_retry_subflows(sk);
> +
>  	sk_stream_wait_close(sk, timeout);
>  
>  cleanup:

[Severity: Medium]
Should mptcp_close_retry_subflows() be called before mptcp_do_fastclose()
and __mptcp_wr_shutdown(), or also handle retry subflows that have already
been disconnected?

When closing an established or TCP_CLOSE_WAIT MPTCP socket that has no
unsent data queued in mptcp_send_head(sk), __mptcp_wr_shutdown() calls
mptcp_check_send_data_fin(), which iterates over msk->conn_list and calls
mptcp_subflow_shutdown(sk, tcp_sk, SEND_SHUTDOWN) on every subflow:

net/mptcp/protocol.c:mptcp_subflow_shutdown() {
	...
	switch (ssk->sk_state) {
	case TCP_LISTEN:
		break;
	case TCP_SYN_SENT:
		WARN_ON_ONCE(tcp_disconnect(ssk, O_NONBLOCK));
		break;
	...
}

For a retry subflow in TCP_SYN_SENT, tcp_disconnect() sets ssk->sk_state to
TCP_CLOSE without invoking sk_state_change, unlinking the subflow from
msk->conn_list, or decrementing msk->pm.extra_subflows.

When mptcp_close_retry_subflows() runs right after __mptcp_wr_shutdown() (or
during close() after an earlier shutdown(fd, SHUT_WR)), it checks for
TCP_SYN_SENT:

	if (!subflow->retry || inet_sk_state_load(ssk) != TCP_SYN_SENT)
		continue;

Because ssk->sk_state is already TCP_CLOSE, mptcp_close_retry_subflows()
skips the retry subflow. This leaves msk->pm.extra_subflows incremented and
leaves the disconnected retry subflow on msk->conn_list, which can also
prevent sk->sk_state from transitioning to TCP_CLOSE before
sk_stream_wait_close(sk, timeout) when all other subflows are already
closed.

Similarly, on the fastclose path, calling mptcp_do_fastclose() before
mptcp_close_retry_subflows() calls tcp_send_active_reset() on the
TCP_SYN_SENT retry subflow before mptcp_close_retry_subflows() can close it.

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