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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] MSG_PEEK uses a stale peek_seq after msk->copied_seq is resynced, causing it to skip newly received data and stall indefinitely.
- [High] In MPTCP active fallback scenarios, msk->copied_seq is never synchronized to the IASN, causing recvmsg to silently skip all received data.
--

--- Patch [3]: [PATCH mptcp-next v4 3/7] mptcp: remove CB offset field ---
commit 05518f1ca85e68c8cc86ae9b67a0c4d849dfd066
Author: Paolo Abeni <[email protected]>

mptcp: remove CB offset field

This commit removes the CB offset field and introduces a new msk-level
copied_seq field to track bytes already consumed inside each skb.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7944ee0f93b13..d52b1773fbb6b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -2199,33 +2192,24 @@ static void mptcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
>  }
>  
>  static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
> -				size_t len, int flags, int copied_total,
> +				size_t len, int flags, u64 *seq,
>  				struct scm_timestamping_internal *tss,
>  				int *cmsg_flags, struct sk_buff **last)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct sk_buff *skb, *tmp;
> -	int total_data_len = 0;
>  	int copied = 0;
>  
>  	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
> -		u32 delta, offset = MPTCP_SKB_CB(skb)->offset;
> +		u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
>  		u32 data_len = skb->len - offset;
>  		u32 count;
>  		int err;
>  
> -		if (flags & MSG_PEEK) {
> -			/* skip already peeked skbs */
> -			if (total_data_len + data_len <= copied_total) {
> -				total_data_len += data_len;
> -				*last = skb;
> -				continue;
> -			}
> -
> -			/* skip the already peeked data in the current skb */
> -			delta = copied_total - total_data_len;
> -			offset += delta;
> -			data_len -= delta;
> +		/* Skip the already peeked data. */
> +		if (offset >= skb->len) {
> +			*last = skb;
> +			continue;
> +		}

[Severity: High]
During an active connection fallback, could this offset calculation underflow?

If the server replies with a SYN-ACK lacking MPTCP options, msk->copied_seq
remains 0 because MPTCP_SYNC_SEQ is bypassed during fallback. When
__mptcp_recvmsg_mskq() processes the fallback SKBs, *seq is 0 but map_seq is
set to the initial ack sequence number.

Does this subtraction underflow to a large positive value, causing
offset >= skb->len to evaluate to true and silently skip all valid
payload data?

[ ... ]
> @@ -2449,6 +2430,11 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  
>  	len = min_t(size_t, len, INT_MAX);
>  	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
> +	seq = &msk->copied_seq;
> +	if (flags & MSG_PEEK) {
> +		peek_seq = msk->copied_seq;
> +		seq = &peek_seq;
> +	}
>  
>  	if (unlikely(msk->recvmsg_inq))
>  		cmsg_flags = MPTCP_CMSG_INQ;
> @@ -2458,7 +2444,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  		int err, bytes_read;
>  
>  		bytes_read = __mptcp_recvmsg_mskq(sk, msg, len - copied, flags,
> -						  copied, &tss, &cmsg_flags,
> +						  seq, &tss, &cmsg_flags,
>  						  &last);
>  		if (unlikely(bytes_read < 0)) {
>  			if (!copied)
> @@ -2468,6 +2454,10 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  
>  		copied += bytes_read;
>  
> +		/* Recompute peek offset after eventual seq resync. */
> +		if (flags & MSG_PEEK)
> +			peek_seq = msk->copied_seq + copied;
> +
>  		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk))
>  			continue;

[Severity: High]
When MSG_PEEK is used, could peek_seq become stale if the backlog
synchronizes the sequence?

If mptcp_move_skbs() processes the backlog and adds the initial ack sequence
number to msk->copied_seq via MPTCP_SYNC_SEQ, the loop executes a continue.

Since peek_seq was updated before mptcp_move_skbs() was called, it lacks the
new offset. On the next iteration, will calling __mptcp_recvmsg_mskq() with
this stale *seq value cause the offset calculation to underflow, skipping
the newly received data and stalling the receiver indefinitely?

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