Re: [PATCH mptcp-next v6 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:
- [Medium] mptcp: pm: TOCTOU race on sk_state in mptcp_pm_rm_addr_or_subflow()
- [Medium] mptcp: pm: in-kernel: missing subflow->resetting check in mptcp_pm_kernel_retry_check()
--

commit 8d1998d3ad0845aa492d3b66abe99c472d868e05
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 initiated by this peer when TCP closes them after an error, while
avoiding retries when the peer sent an non-transient MP_RST, when the
subflow was aborted locally, or when its address was removed.

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d7c5b50b34cc9..54d94bc729630 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c

[ ... ]

> @@ -762,6 +782,24 @@ static void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk)
>  	mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
>  }
>  
> +/* A subflow closed by TCP but not collected yet must not be re-created with
> + * a removed address, see mptcp_pm_kernel_retry_check().
> + */
> +static void mptcp_pm_rm_no_retry(struct mptcp_subflow_context *subflow,
> +				 enum linux_mptcp_mib_field rm_type, u8 rm_id)
> +{
> +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +	u8 id;
> +
> +	if (rm_type == MPTCP_MIB_RMADDR)
> +		id = READ_ONCE(subflow->remote_id);
> +	else
> +		id = subflow_get_local_id(subflow);
> +
> +	if (id == rm_id && inet_sk_state_load(ssk) == TCP_CLOSE)
> +		subflow->no_retry = 1;
> +}
> +
>  static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
>  					const struct mptcp_rm_list *rm_list,
>  					enum linux_mptcp_mib_field rm_type)
> @@ -794,6 +832,7 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
>  			int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
>  			u8 id = subflow_get_local_id(subflow);
>  
> +			mptcp_pm_rm_no_retry(subflow, rm_type, rm_id);
>  			if ((1 << inet_sk_state_load(ssk)) &
>  			    (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
>  				continue;

[Severity: Medium]
Can a subflow whose address is being removed still be retried if its
subflow socket transitions to TCP_CLOSE concurrently?

mptcp_pm_rm_addr_or_subflow() runs under the msk socket lock and
msk->pm.lock without holding the subflow socket lock (ssk), and reads
ssk->sk_state twice without synchronization.

If ssk is not yet in TCP_CLOSE when mptcp_pm_rm_no_retry() checks
inet_sk_state_load(ssk) == TCP_CLOSE, subflow->no_retry stays 0. If a
concurrent TCP timer or softirq (e.g., tcp_write_err() or tcp_reset()
under bh_lock_sock(ssk)) moves ssk to TCP_CLOSE before the second
inet_sk_state_load(ssk) in mptcp_pm_rm_addr_or_subflow(), or if ssk was
in TCP_FIN_WAIT1, TCP_FIN_WAIT2, or TCP_CLOSING and later closes with an
error, the loop takes the continue branch without calling
mptcp_close_ssk().

When __mptcp_close_subflow() later processes ssk, both
subflow->close_event_done and subflow->no_retry are 0, so
mptcp_pm_kernel_retry_check() will re-create the subflow using the
removed address. Would setting subflow->no_retry = 1 whenever id == rm_id
(regardless of whether ssk has already reached TCP_CLOSE) avoid this race?

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 0379e1495cf4d..e88c236b62e9f 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1610,6 +1610,106 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
>  	return true;
>  }
>  
> +/* Called with the msk socket lock held, before the subflow is closed */
> +bool mptcp_pm_kernel_retry_check(struct mptcp_sock *msk,
> +				 const struct mptcp_subflow_context *subflow,
> +				 struct mptcp_pm_retry *retry)
> +{

[ ... ]

> +	/* The peer asked not to use this path again (MP_RST without the T
> +	 * flag), the subflow has been aborted locally, or it hit an MPTCP
> +	 * protocol error.
> +	 */
> +	if ((subflow->reset_seen && !subflow->reset_transient) ||
> +	    err == ECONNABORTED || err == EBADMSG)
> +		return false;

[Severity: Medium]
Should this check also reject subflows where subflow->resetting is set?

When a subflow is locally aborted due to an MPTCP protocol error via
mptcp_subflow_reset() (for example from mptcp_mp_fail_no_response(),
mptcp_update_infinite_map(), or mptcp_dss_corruption()), it sets
subflow->resetting = 1 and calls tcp_done(ssk) without setting
ssk->sk_err:

net/mptcp/subflow.c:mptcp_subflow_reset() {
	...
	subflow->resetting = 1;

	/* No need to delay the actual close for to-be discarded data. */
	__skb_queue_purge(&ssk->sk_receive_queue);
	mptcp_send_active_reset_reason(ssk);
	tcp_done(ssk);
	...
}

If IP_RECVERR or IPV6_RECVERR was enabled on the socket, an earlier
non-fatal ICMP error on the established subflow leaves a non-zero
ssk->sk_err (such as EHOSTUNREACH or EPROTO) untouched because
__mptcp_subflow_error_report() skips sock_error(ssk) on established
non-fallback sockets:

net/mptcp/protocol.c:__mptcp_subflow_error_report() {
	...
	if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
		goto out;

	err = sock_error(ssk);
	...
}

If mptcp_subflow_reset() later closes that subflow,
mptcp_pm_kernel_retry_check() will see the stale ssk->sk_err (which is
neither ECONNABORTED nor EBADMSG) and re-create a subflow that was reset
locally for an MPTCP protocol error.

[ ... ]

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