Re: [PATCH mptcp-next RFC v5 2/3] mptcp: fallback immediately on MP_FAIL reception

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] State Machine Violation: Executing receive logic and queueing data on a closed socket
- [High] Logic Bug: Premature fallback evaluation uses stale MPTCP queue state
--

--- Patch [2]: [PATCH mptcp-next RFC v5 2/3] mptcp: fallback immediately on MP_FAIL reception ---
commit 293c264f04a7af047f95b678acf86f3fe06f4d7d
Author: Chenguang Zhao <zhaochenguang@kylinos.cn>

mptcp: fallback immediately on MP_FAIL reception
    
Call mptcp_try_fallback() right after the MP_FAIL response
ACK, and simplify mptcp_update_infinite_map() accordingly.

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d7c5b50b34cc9..351f9c998d7b6 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -895,6 +895,11 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
>  		pr_debug("MP_FAIL response received\n");
>  		WRITE_ONCE(subflow->fail_tout, 0);
>  	}
> +
> +	if (!mptcp_try_fallback(sk, MPTCP_MIB_MPFAILFALLBACK)) {
> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_FALLBACKFAILED);
> +		mptcp_subflow_reset(sk);

[Severity: High]
Does this synchronously reset the socket to TCP_CLOSE during the RX path?

If mptcp_try_fallback() fails, we call mptcp_subflow_reset() which sets
the socket state to TCP_CLOSE. However, this is called from
mptcp_incoming_options() when parsing the MP_FAIL option:

net/mptcp/options.c:mptcp_incoming_options() {
    ...
		if (mp_opt.suboptions & OPTION_MPTCP_FAIL) {
			mptcp_pm_mp_fail_received(sk, mp_opt.fail_seq);
			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILRX);
		}
    ...
		if (!(mp_opt.suboptions & OPTION_MPTCP_DSS))
			return true;
}

If mptcp_incoming_options() then returns true without checking if the socket
was closed, won't the TCP stack continue normal packet processing? This could
cause tcp_data_queue() to queue the packet's payload onto the closed socket,
and tcp_rcv_established() to proceed with an invalid state.

[Severity: High]
Does premature fallback evaluation here use stale MPTCP queue state?

Evaluating mptcp_try_fallback() synchronously in the softirq receive path
creates a race condition with the asynchronous MPTCP worker.
mptcp_try_fallback() checks if msk->out_of_order_queue is empty, but because
MPTCP queues are populated asynchronously by the MPTCP worker, this state is
inherently stale.

Since the current packet hasn't even been added to the TCP receive queue
yet, couldn't a gap be correctly filled by data sitting in the subflow's TCP
queue that hasn't been pulled into the MPTCP queue yet? This would cause
fallback to erroneously fail and lead to unwarranted subflow resets.

> +	}
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819112112.1985121-1-chenguang.zhao@linux.dev?part=2