Re: [PATCH mptcp-net 10/34] mptcp: don't leak an orphan when the PM closes its last subflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mptcp: skipped TCP_CLOSE cleanup for msk->first in
  mptcp_close_ssk() [1]
--

commit 9c49fccbfab6fbc60fdc19e95bb13b504edf3ebc
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: don't leak an orphan when the PM closes its last subflow

This commit expires the close timeout and schedules mptcp_worker() when
__mptcp_close_ssk() leaves an orphaned MPTCP socket with only a closed
initial subflow, instead of moving it directly to TCP_CLOSE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 16824bed85b42..6dc55a3f027bd 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2794,8 +2794,13 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	 */
>  	if (list_is_singular(&msk->conn_list) && msk->first &&
>  	    inet_sk_state_load(msk->first) == TCP_CLOSE) {
> -		if (sk->sk_state != TCP_ESTABLISHED ||
> -		    msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
> +		if (sock_flag(sk, SOCK_DEAD)) {
> +			/* let the worker close and destroy the orphan */
> +			mptcp_set_close_tout(sk, tcp_jiffies32 -
> +					     (mptcp_close_timeout(sk) + 1));
> +			mptcp_schedule_work(sk);
> +		} else if (sk->sk_state != TCP_ESTABLISHED ||
> +			   msk->in_accept_queue) {
>  			mptcp_set_state(sk, TCP_CLOSE);
>  			mptcp_close_wake_up(sk);
>  		} else {

[Severity: High]
This problem wasn't introduced by this patch, but can mptcp_close_ssk() skip
this TCP_CLOSE cleanup for msk->first when it is half-closed first [1]?

When msk->first is gracefully closed before reaching TCP_CLOSE (for example,
on a subflow-level FIN without DATA_FIN or via mptcp_pm_rm_addr_or_subflow())
while msk->free_first and msk->fastclosing are false, mptcp_close_ssk() sets
subflow->close_event_done = true and __mptcp_close_ssk() calls
__mptcp_subflow_disconnect(), which only runs tcp_shutdown(ssk, SEND_SHUTDOWN)
before jumping to out: while msk->first is still not in TCP_CLOSE:

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);
	} else {
		tcp_shutdown(ssk, SEND_SHUTDOWN);
	}
}

net/mptcp/protocol.c:mptcp_close_ssk() {
	if (subflow->close_event_done || READ_ONCE(subflow->local_id) < 0)
		return;

	subflow->close_event_done = true;
    ...
	__mptcp_close_ssk(sk, ssk, subflow, MPTCP_CF_PUSH);
}

When the FIN handshake later completes and msk->first reaches TCP_CLOSE,
__mptcp_close_subflow() calls mptcp_close_ssk() again, which returns early
because subflow->close_event_done is already set.  As a result, msk->first is
never disconnected or reset, and if it is the last subflow in msk->conn_list,
the parent msk never starts its close timeout timer or transitions to
TCP_CLOSE.

[1] https://sashiko.dev/#/bug/linux-47cd2e29-62e6-4ad6-9246-4044e6d4b0e9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=10