Re: [PATCH mptcp-next v6 4/7] mptcp: sync mptcp skb cb layout with tcp one

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Torn read/write of 64-bit `msk->ack_seq` on 32-bit architectures, leading to corrupted MPTCP Data ACKs.
--

--- Patch [4]: [PATCH mptcp-next v6 4/7] mptcp: sync mptcp skb cb layout with tcp one ---
commit d6240ab2e98090a0998cc27a5a2729d562ab8ffd
Author: Paolo Abeni <[email protected]>

mptcp: sync mptcp skb cb layout with tcp one

This commit limits the MPTCP-level sequence number to 32 bits after
validating against the receive window. This allows the MPTCP CB layout
to align with TCP-level fields inside the control buffer, and adds
build time checks to enforce this invariant.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0700cdab7f2ed..2e0e8e85b4d0f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -892,40 +899,40 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
>  {
[ ... ]
> -		msk->bytes_received += end_seq - msk->ack_seq;
> -		WRITE_ONCE(msk->ack_seq, end_seq);
> +		msk->bytes_received += seq_delta;
> +		WRITE_ONCE(msk->ack_seq, msk->ack_seq + seq_delta);
>  		moved = true;
>  	}
>  	return moved;

[Severity: High]
This is a pre-existing issue, but can this lead to torn reads on 32-bit
architectures?

Because msk->ack_seq is a 64-bit value, updating it with WRITE_ONCE() in
process context here (and similarly in __mptcp_move_skb()) without holding
mptcp_data_lock(msk) might race with concurrent softirq readers.

If incoming subflow packets are processed on another CPU, they generate
TCP ACKs via:

tcp_transmit_skb()
  mptcp_established_options()
    ack_seq = READ_ONCE(msk->ack_seq);

Since this read only holds the subflow lock (bh_lock_sock(ssk)) and not the
parent MPTCP socket lock, a concurrent 64-bit WRITE_ONCE() on 32-bit systems
compiles into a non-atomic dual 32-bit memory access.

Could this torn read result in sending a corrupted MPTCP Data ACK to the
peer? If the sequence number jumps forward or backward unexpectedly, might
the peer incorrectly assume data was successfully received or have its
connection state disrupted?

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