Re: [PATCH mptcp-next v9 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 2 potential issue(s) to consider:
- [High] mptcp: close timeout from mptcp_retry_check_tout() not stopped on retry join
- [Medium] mptcp: pm: retry subflows keep half-closed sockets open when all subflows close
--

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

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

This commit updates the in-kernel path manager to re-create established
subflows initiated by the local host when TCP closes them after a
retransmission or keepalive timeout, using the subflow-retries limit for SYN
retransmissions. It also cleans up connecting retry subflows on socket close
and starts the close timeout timer if no active subflows remain while a retry
subflow is connecting.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..25fd0628686c3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -2788,6 +2791,32 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	}
>  }
>  
> +/* With no subflow left to send data, a re-created subflow still connecting
> + * must not keep the connection open longer than the close timeout, as when
> + * all the subflows are closed, see __mptcp_close_ssk(). A join stops it.
> + */
> +static void mptcp_retry_check_tout(struct sock *sk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	bool connecting = false;
> +
> +	if (sk->sk_state != TCP_ESTABLISHED ||
> +	    inet_csk(sk)->icsk_mtup.probe_timestamp)
> +		return;

[Severity: Medium]
What happens if sk transitions out of TCP_ESTABLISHED while a re-created
subflow is still connecting in TCP_SYN_SENT, and the remaining active
subflows subsequently close before close() is called?

For example, if subflow 1 times out while sk->sk_state is TCP_ESTABLISHED,
__mptcp_close_subflow() -> mptcp_pm_subflow_retry() creates a retry subflow
in TCP_SYN_SENT while subflow 2 is still active.

If the peer then sends DATA_FIN on subflow 2 (moving sk->sk_state to
TCP_CLOSE_WAIT in mptcp_check_data_fin()), or the application calls
shutdown(fd, SHUT_WR) with unsent data still queued (moving sk->sk_state to
TCP_FIN_WAIT1 in mptcp_shutdown() while mptcp_check_send_data_fin() returns
before calling mptcp_subflow_shutdown()), mptcp_close_retry_subflows() is not
called because it only runs from __mptcp_close(). At that point,
mptcp_is_fully_established(sk) is false, so mptcp_finish_join() will reject
the connecting retry subflow even if the path recovers.

If subflow 2 later closes via mptcp_close_ssk(), __mptcp_close_ssk() checks
list_is_singular(&msk->conn_list), which is false because the connecting
retry subflow is still in msk->conn_list alongside msk->first:

net/mptcp/protocol.c:__mptcp_close_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)) {
			mptcp_set_state(sk, TCP_CLOSE);
			mptcp_close_wake_up(sk);
		} else {
			mptcp_start_tout_timer(sk);
		}
	}
}

Then mptcp_retry_check_tout() returns early because sk->sk_state is no longer
TCP_ESTABLISHED. Doesn't this leave the half-closed MPTCP socket open without
a close timeout timer until all 127 SYN retries on the retry subflow expire?

> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		if (__mptcp_subflow_active(subflow))
> +			return;
> +
> +		connecting |= subflow->retry &&
> +			      !READ_ONCE(subflow->fully_established);
> +	}
> +
> +	if (connecting)
> +		mptcp_start_tout_timer(sk);
> +}
>  
>  void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		     struct mptcp_subflow_context *subflow)
> @@ -2806,6 +2835,7 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	mptcp_pm_subflow_check_next(mptcp_sk(sk), subflow);
>  
>  	__mptcp_close_ssk(sk, ssk, subflow, MPTCP_CF_PUSH);
> +	mptcp_retry_check_tout(sk);
>  }
>  
>  static unsigned int mptcp_sync_mss(struct sock *sk, u32 pmtu)
> @@ -2823,6 +2853,8 @@ static void __mptcp_close_subflow(struct sock *sk)
>  	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>  		int ssk_state = inet_sk_state_load(ssk);
> +		struct mptcp_pm_retry retry;
> +		bool do_retry;
>  
>  		if (ssk_state != TCP_CLOSE &&
>  		    (ssk_state != TCP_CLOSE_WAIT ||
> @@ -2834,7 +2866,13 @@ static void __mptcp_close_subflow(struct sock *sk)
>  		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
>  			continue;
>  
> +		do_retry = mptcp_pm_subflow_retry_check(msk, subflow, &retry);
>  		mptcp_close_ssk(sk, ssk, subflow);
> +		if (do_retry) {
> +			mptcp_pm_subflow_retry(msk, &retry);
> +			/* creating a subflow stopped the close timeout */
> +			mptcp_retry_check_tout(sk);
> +		}

[Severity: High]
Where is the close timeout timer stopped once the re-created subflow finishes
connecting and joins the connection?

When the last active subflow closes and is retried here in
__mptcp_close_subflow(), mptcp_pm_subflow_retry() -> mptcp_pm_kernel_retry()
calls __mptcp_subflow_connect(), which adds subflow->node to msk->conn_list,
starts the non-blocking connect, and calls mptcp_stop_tout_timer(sk).

Immediately after mptcp_pm_subflow_retry() returns (or in mptcp_close_ssk()
when another last active subflow closes while a retry subflow is connecting),
mptcp_retry_check_tout(sk) sees no active subflows and calls
mptcp_start_tout_timer(sk), setting inet_csk(sk)->icsk_mtup.probe_timestamp
and arming the close timeout timer.

The comment in mptcp_retry_check_tout() notes "A join stops it", but when the
outgoing retry subflow receives SYN+ACK and subflow_finish_connect() calls
mptcp_finish_join(ssk), subflow->node is already in msk->conn_list, so
mptcp_finish_join() takes the early return path without calling
__mptcp_finish_join() or mptcp_stop_tout_timer(sk):

net/mptcp/protocol.c:mptcp_finish_join() {
	...
	if (!list_empty(&subflow->node)) {
		spin_lock_bh(&msk->fallback_lock);
		if (!msk->allow_subflows) {
			spin_unlock_bh(&msk->fallback_lock);
			return false;
		}
		mptcp_subflow_joined(msk, ssk);
		spin_unlock_bh(&msk->fallback_lock);
		mptcp_propagate_sndbuf(parent, ssk);
		return true;
	}
}

Later, when the 4th ACK arrives in check_fully_established() ->
__mptcp_subflow_fully_established(), mptcp_stop_tout_timer(sk) is not called
there either.

Because inet_csk(sk)->icsk_mtup.probe_timestamp remains non-zero, won't
mptcp_worker() see mptcp_close_tout_expired(sk) as true once close_timeout
elapses and abort the recovered connection with mptcp_do_fastclose(sk) and
__mptcp_close_ssk() (also leaving msk->pm.extra_subflows incremented)?

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