Re: [PATCH mptcp-next v12 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:
- [High] mptcp: unclosed non-SYN_SENT retry subflows in mptcp_close_retry_subflows()
--

commit de5632976a598bbd4a3db378be502a28582f26e1
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 an established
subflow initiated by the local host after TCP closes it due to a path timeout
while another subflow can still send data. It also adds cleanup logic to
close re-created subflows that are not yet fully established when no other
subflow can send data or when the MPTCP socket is shut down or closed.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d8c..257059ffdc7e9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -736,6 +736,10 @@ static void mptcp_check_data_fin(struct sock *sk)
>  		if (!__mptcp_check_fallback(msk))
>  			mptcp_send_ack(msk);
>  		mptcp_close_wake_up(sk);
> +
> +		/* not always called from the worker, which closes them */
> +		if (mptcp_retry_subflows_stale(msk))
> +			mptcp_schedule_work(sk);
>  	}
>  }

[ ... ]

> @@ -2788,6 +2795,49 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	}
>  }
>  
> +/* Re-created subflows not established yet are useless once the msk is
> + * closed or shut down. When no other subflow can send data, see
> + * mptcp_retry_subflows_stale(), only the ones still connecting are: a joined
> + * one is fully established soon. Not called while iterating on the subflows.
> + */
> +static void mptcp_close_retry_subflows(struct sock *sk, bool connecting)
> +{
> +	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 || READ_ONCE(subflow->fully_established))
> +			continue;
> +
> +		if (connecting) {
> +			if (inet_sk_state_load(ssk) != TCP_SYN_SENT)
> +				continue;
> +			/* another one got established in the meantime */
> +			if (!mptcp_retry_subflows_stale(msk))
> +				break;
> +		}

[ ... ]

> @@ -3185,6 +3246,9 @@ static void mptcp_worker(struct work_struct *work)
>  	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
>  		__mptcp_close_subflow(sk);
>  
> +	if (mptcp_retry_subflows_stale(msk))
> +		mptcp_close_retry_subflows(sk, true);

[Severity: High]
Can this leave unestablished retry subflows open when the MPTCP socket is no
longer established or when all other subflows have closed?

When mptcp_check_data_fin() transitions sk->sk_state out of TCP_ESTABLISHED
(for example to TCP_CLOSE_WAIT or TCP_CLOSING), mptcp_retry_subflows_stale()
returns true for any retry subflow with !subflow->fully_established and
schedules mptcp_worker().

However, mptcp_worker() unconditionally calls
mptcp_close_retry_subflows(sk, true) with connecting set to true rather than
checking sk->sk_state == TCP_ESTABLISHED. Inside
mptcp_close_retry_subflows(), when connecting is true:

	if (connecting) {
		if (inet_sk_state_load(ssk) != TCP_SYN_SENT)
			continue;
		...
	}

This skips any retry subflow with !subflow->fully_established that is already
in TCP_SYN_RECV or TCP_ESTABLISHED (such as one that received the MP_JOIN
SYN-ACK and is waiting for the 4th ACK).

Similarly, if sk->sk_state is TCP_ESTABLISHED and the last active subflow
closes via mptcp_close_ssk(), leaving only the closed msk->first and a
joined (!subflow->fully_established) retry subflow in TCP_ESTABLISHED or
TCP_SYN_RECV, mptcp_close_retry_subflows(sk, true) also skips that retry
subflow.

Because the unestablished retry subflow remains in msk->conn_list alongside
msk->first, list_is_singular(&msk->conn_list) at the end of
__mptcp_close_ssk() evaluates to false:

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

As a result, neither mptcp_set_state(sk, TCP_CLOSE) nor
mptcp_start_tout_timer(sk) is called. Meanwhile, on the joined retry subflow
with !subflow->fully_established, __mptcp_subflow_active(subflow) is false,
so no data is queued on its TCP write queue and tcp_write_err() never fires.
If the 4th ACK never arrives, schedule_3rdack_retransmission() re-arms
icsk_delack_timer to send pure 0-byte 3rd ACKs indefinitely without a retry
limit, keeping the connection hung.

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