Re: [RFC PATCH mptcp-next v3 2/3] mptcp: pm: 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: duplicate MPTCP_EVENT_SUB_CLOSED event emitted on reconnect failure
--

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

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

This commit adds an opt-in sysctl to retry creating subflows closed by
TCP after an error. The PM immediately creates a new subflow using the
same addresses, and spaces out attempts via SYN retransmissions. A new
SubflowRetry counter tracks these attempts.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/440

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index d3014bf57bf3c..d020e5f69c013 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
[ ... ]
> @@ -1591,6 +1591,151 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
[ ... ]
> +/* Called with the msk socket lock held, instead of closing the subflow */
> +bool mptcp_pm_kernel_retry_reconnect(struct mptcp_sock *msk, struct sock *ssk,
> +				     const struct mptcp_pm_retry *retry)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	struct sock *sk = (struct sock *)msk;
> +
> +	/* like mptcp_close_ssk() does for the failed attempt */
> +	mptcp_event(MPTCP_EVENT_SUB_CLOSED, msk, ssk, GFP_KERNEL);
> +
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
> +	if (mptcp_subflow_reconnect(sk, ssk, &retry->local, &retry->remote))
> +		return false;

[Severity: Medium]
Will this sequence emit a duplicate MPTCP_EVENT_SUB_CLOSED event if the
reconnect attempt fails?

When mptcp_subflow_reconnect() is called, it clears the subflow context
using mptcp_subflow_ctx_reset(), which zeroes the close_event_done flag:

net/mptcp/subflow.c:mptcp_subflow_reconnect() {
...
	mptcp_subflow_ctx_reset(subflow);
...

If the underlying tcp connect call fails, mptcp_subflow_reconnect() returns
an error, causing mptcp_pm_kernel_retry_reconnect() to return false.

[ ... ]
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 60cbd91be6320..f29cfd2ad69b9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -2827,7 +2829,14 @@ static void __mptcp_close_subflow(struct sock *sk)
>  		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
>  			continue;
>  
> +		do_retry = mptcp_pm_kernel_retry_check(msk, subflow, &retry);
> +		if (do_retry && retry.reuse &&
> +		    mptcp_pm_kernel_retry_reconnect(msk, ssk, &retry))
> +			continue;
> +
>  		mptcp_close_ssk(sk, ssk, subflow);

[Severity: Medium]
When mptcp_pm_kernel_retry_reconnect() returns false, the fallback path
here calls mptcp_close_ssk().

Since the close_event_done flag was cleared to false by the failed
reconnect attempt, and the parent MPTCP socket is still in TCP_ESTABLISHED,
mptcp_close_ssk() will unconditionally emit a second MPTCP_EVENT_SUB_CLOSED
event for the exact same subflow socket.

Could this duplicate state notification confuse or desynchronize userspace
path managers?

> +		if (do_retry && !retry.reuse)
> +			mptcp_pm_kernel_retry(msk, &retry);
>  	}
>  
>  }

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