Re: [PATCH export v3 3/4] mptcp: reset subflow on MP_FAIL when OoO queue is non-empty

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] TOCTOU race due to lockless access of `msk->out_of_order_queue`.
--

--- Patch [3]: [PATCH export v3 3/4] mptcp: reset subflow on MP_FAIL when OoO queue is non-empty ---
commit fe6cef2f543d25c806aed2ee11c8a4d7a936c259
Author: Chenguang Zhao <[email protected]>

mptcp: reset subflow on MP_FAIL when OoO queue is non-empty

This commit updates the handling of an MP_FAIL response to reset the subflow
instead of falling back when the out-of-order queue is not empty. This is done
to ensure the contiguous data requirements for infinite mapping are met.

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index ba7c6f80a183c..8c263084db7be 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -882,6 +882,16 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
>  		spin_unlock_bh(&msk->fallback_lock);
>  		return;
>  	}
> +
> +	/* RFC8684 §3.7: Infinite mapping requires contiguous data */
> +	if (!subflow->fail_tout &&
> +	    !RB_EMPTY_ROOT(&msk->out_of_order_queue)) {

[Severity: High]
Can this lockless check of the out-of-order queue lead to a race condition?

Looking at mptcp_pm_mp_fail_received(), the out_of_order_queue is checked
for emptiness while only holding msk->fallback_lock, without the required
mptcp_data_lock.

If another active subflow concurrently receives out-of-order data, acquires
the msk data lock, and inserts data into the queue, this check might
incorrectly conclude the queue is empty.

If the queue is populated right after this lockless check but before
mptcp_try_fallback() is called, could this cause an infinite map TCP ACK to
be incorrectly transmitted before the fallback is ultimately aborted? 

Furthermore, if the queue is populated immediately after fallback occurs,
could this stranded MPTCP out-of-order data be improperly appended to the
plain TCP receive queue as the stream advances, resulting in data duplication
and stream corruption?

> +		spin_unlock_bh(&msk->fallback_lock);
> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_FALLBACKFAILED);
> +		mptcp_subflow_reset(sk);
> +		return;
> +	}
> +
>  	msk->allow_subflows = false;
>  	spin_unlock_bh(&msk->fallback_lock);

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