Re: [PATCH mptcp-next RFC 3/3] mptcp: pm: kernel: skip operating on closing connections

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PM endpoint operations silently ignore half-closed connections, leaving stale PM state and breaking MPTCP resilience.
- [High] Lockless connection state check introduces a Time-of-Check to Time-of-Use race that causes transitioning sockets to permanently miss endpoint removals.
--

--- Patch [3]: [PATCH mptcp-next RFC 3/3] mptcp: pm: kernel: skip operating on closing connections ---
commit d802951fdbb5f62840e58ea428dab6858edf0731
Author: Matthieu Baerts (NGI0) <[email protected]>

mptcp: pm: kernel: skip operating on closing connections

When iterating over each MPTCP connection after having manipulated MPTCP
endpoints, there is no need to operate on closing (or not ready)
connections. We can then safely skip those.

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index d3014bf57bf3c..c185e53fe42b1 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c

[ ... ]

> @@ -1095,7 +1095,8 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
>  		struct sock *sk = (struct sock *)msk;
>  		bool remove_subflow;
>  
> -		if (mptcp_pm_is_userspace(msk))
> +		if (!mptcp_is_fully_established(sk) ||
> +		    mptcp_pm_is_userspace(msk))
>  			goto next;
>  
>  		lock_sock(sk);

[Severity: High]
Are half-closed connections skipped by this check?

mptcp_is_fully_established() requires the socket to be in the TCP_ESTABLISHED
state. If a connection is transferring data while in a half-closed state
like TCP_CLOSE_WAIT or TCP_FIN_WAIT1, the PM endpoint operations will silently
skip it.

If an interface goes down, could this cause dead subflows to remain alive,
breaking failover because data continues to stall over removed interfaces
instead of seamlessly switching to active paths?

[Severity: High]
Can this lockless state check cause transitioning connections to miss
endpoint removals?

Because the check happens before lock_sock(sk) is called, a connection
transitioning from TCP_SYN_RECV to TCP_ESTABLISHED could be skipped.

If the socket transitions to TCP_ESTABLISHED immediately after being skipped
here, the state machine triggers mptcp_pm_nl_fully_established(). However,
that function only synchronizes additions from the PM database, not removals.

Will this result in the endpoint removal event being lost permanently,
leaving stale subflows active indefinitely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3