Re: [PATCH mptcp-next v10 2/9] mptcp: drop the cant_coalesce CB field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A race condition in `__mptcp_move_skb` causes premature consumption of the `rcvd_dummy_seq` flag and a stale read of `msk->ack_seq`, permanently corrupting the fastopen SKB sequence mapping.
--

commit 4b77d7da2be72a2e1f480d6176376b2e639e2faa
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: drop the cant_coalesce CB field

This patch removes the cant_coalesce field from the MPTCP SKB control block.
It replaces synchronous updates at 3rd ACK time by synchronizing the fastopen
skb sequence when the IASN becomes available, before the next segment enters
the msk receive queue.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7421b44934f05..0a90bed02b412 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -446,6 +464,12 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  
>  	mptcp_borrow_fwdmem(sk, skb);
>  
> +	/* Be sure to sync the eventual fastopen dummy mapping before any other
> +	 * skb lands into the msk.
> +	 */
> +	if (unlikely(msk->rcvd_dummy_seq))
> +		__mptcp_sync_rcv_sequence(sk);
> +
>  	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {

[Severity: High]
Does this execute the sync logic unconditionally without waiting for the
MPTCP_SYNC_SEQ flag or a memory barrier? 

If a concurrent mptcp_release_cb() processes the backlog while the 3rd ACK
is being processed, could this lead to a stale read of msk->ack_seq?

Since __mptcp_sync_rcv_sequence() reads the uninitialized ack_seq and clears
rcvd_dummy_seq, it appears this might permanently corrupt the fastopen SKB
sequence mapping, leading to data loss or connection stall during TCP Fast
Open.

Could this be triggered remotely by sending an MP_JOIN SYN and data before
the 3rd ACK of the initial MPC connection?

[ ... ]
> @@ -3895,6 +3919,13 @@ static void mptcp_release_cb(struct sock *sk)
>  			__mptcp_error_report(sk);
>  		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
>  			__mptcp_sync_sndbuf(sk);
> +		if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +			/* Ensure we see the updated ack_seq after seeing
> +			 * the flag
> +			 */
> +			smp_rmb();
> +			__mptcp_sync_rcv_sequence(sk);
> +		}
>  	}
>  }

[Severity: High]
While the correct synchronization with the memory barrier is placed here,
is it rendered ineffective because the rcvd_dummy_seq flag was already
prematurely consumed by the unguarded call in __mptcp_move_skb()?

Since __mptcp_sync_rcv_sequence() skips processing if rcvd_dummy_seq is
false, the proper sequence mapping might never occur if the flag is cleared
early.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787888115.git.tanggeliang@kylinos.cn?part=2