Re: [PATCH v2] mptcp: do not drop partial packets

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
mptcp: do not drop partial packets

This commit modifies the handling of out-of-order packets where
map_seq < ack_seq < end_seq. Instead of dropping the entire packet,
it skips the already-acknowledged bytes and enqueues the remainder.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/600

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index fbffd3a43fe86..4e9fba692a701 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -397,12 +397,24 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)

[ ... ]

> +	/* Partial packet: map_seq < ack_seq < end_seq.
> +	 * Skip the already-acked bytes and enqueue the new data.
> +	 */
> +	copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
> +	MPTCP_SKB_CB(skb)->offset += msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
> +	msk->bytes_received += copy_len;
> +	WRITE_ONCE(msk->ack_seq, msk->ack_seq + copy_len);

Should MPTCP_SKB_CB(skb)->map_seq also be advanced here to reflect the newly
adjusted offset?

If map_seq remains stale after skipping the already-acked bytes, does this
cause mptcp_inq_hint() to overestimate the amount of unread data? 

Looking at the hint calculation, it calculates the unread bytes using
READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq for the first
enqueued SKB.

If inq_hint is artificially large, could this corrupt the ioctl(FIONREAD)
and TCP_INQ cmsg values returned to userspace?

Furthermore, could this suppress receive window auto-tuning? In
mptcp_rcv_space_adjust(), an artificially large inq_hint might cause
the calculation copied -= mptcp_inq_hint(sk) to become negative, which
would stall window growth.

> +
> +	skb_set_owner_r(skb, sk);
> +	__skb_queue_tail(&sk->sk_receive_queue, skb);
> +	return true;
>  }