Re: [PATCH mptcp-next v5 3/7] mptcp: remove CB offset field

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing memory barrier before set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) causes a data race, allowing a concurrent lockless reader in __mptcp_move_skb() to read a stale msk->ack_seq.
--

--- Patch [3]: [PATCH mptcp-next v5 3/7] mptcp: remove CB offset field ---
commit 9bf62f08e2b81be277b483eb357610241e9d800b
Author: Paolo Abeni <[email protected]>
mptcp: remove CB offset field

This replaces the SKB CB offset field with a new msk-level copied_seq field
to track the bytes already consumed. It also introduces explicit synchronization
for the TFO SKB in the receive queue using the MPTCP_SYNC_SEQ flag.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -464,6 +460,9 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  
>  	mptcp_borrow_fwdmem(sk, skb);
>  
> +	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
> +		msk->copied_seq += mptcp_iasn(msk);
> +

[Severity: High]
Can this lockless reader observe the MPTCP_SYNC_SEQ flag as set but still read
a stale msk->ack_seq (via mptcp_iasn())?

If a Fast Open connection receives the remote key in BH context while a user
thread concurrently processes the backlog here, a missing memory barrier on the
writer side might cause mptcp_iasn() to calculate using a stale sequence
number. 

Could this corrupt copied_seq and lead to incorrect offset calculations for
subsequent packets?

>  	/* Be sure to sync the eventual fastopen dummy mapping before any other
>  	 * skb lands into the msk.
>  	 */
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
[ ... ]
> @@ -497,10 +497,13 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  	WRITE_ONCE(msk->can_ack, true);
>  	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
>  
> -	if (!sock_owned_by_user(sk))
> +	if (!sock_owned_by_user(sk)) {
> +		/* User space could have already read partially the TFO skb */
> +		msk->copied_seq += subflow->iasn;
>  		__mptcp_sync_rcv_sequence(sk);
> -	else
> +	} else {
>  		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
> +	}
>  }

[Severity: High]
Could the lack of a memory barrier here cause a data race on weakly-ordered
architectures?

The writer updates msk->ack_seq earlier in the function and then sets
MPTCP_SYNC_SEQ using set_bit(), which does not imply a memory barrier. 

Is it possible for the CPU to reorder these writes, making the flag visible to
__mptcp_move_skb() before the new sequence number is committed?

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