Re: [PATCH mptcp-next v21 7/9] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mptcp: kernel BUG in mptcp_init_skb() when pulling nonlinear skb
- [High] mptcp: 32-bit sequence wrap bug in mptcp_init_skb() boundary check
--

commit 1950bbe60e98f6cc91ceb0674f4d63a6b8018ce9
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit removes the offset field from struct mptcp_skb_cb and replaces it
with a new copied_seq field in struct mptcp_sock to track bytes already
consumed inside each skb. It also adds a mapping boundary check in
mptcp_init_skb() to strip already-consumed prefix bytes belonging to a
previous DSS mapping via skb_pull().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index dd612e9b2b262..0666bbfdf2cb8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -415,19 +414,34 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  	skb_set_owner_r(skb, sk);
>  }
>  
> -static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
> -			   int copy_len)
> +static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	bool has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
>  
> +	/* Detect mapping boundary: skb spans multiple mappings */
> +	if (unlikely(offset > 0 &&
> +		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
> +		     subflow->map_subflow_seq)) {

[Severity: High]
As reported by Sashiko on v20 [1], should this comparison use before()
instead of raw unsigned < for 32-bit modular relative subflow sequence
numbers, matching validate_mapping() in net/mptcp/subflow.c?

net/mptcp/subflow.c:validate_mapping() {
	u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;

	if (unlikely(before(ssn, subflow->map_subflow_seq))) {
		...
}

TCP_SKB_CB(skb)->seq, subflow->ssn_offset, and subflow->map_subflow_seq are
all u32 values. Using unsigned < causes regressions in two scenarios:

First, on passive TFO subflows, mptcp_fastopen_subflow_synack_set_params()
in net/mptcp/fastopen.c advances subflow->ssn_offset past the TFO SYN data:

	tp->copied_seq += skb->len;
	subflow->ssn_offset += skb->len;

If a peer sends an overlapping segment starting in the TFO SYN data range
where TCP_SKB_CB(skb)->seq < subflow->ssn_offset < TCP_SKB_CB(skb)->end_seq,
or when relative subflow sequence numbers wrap around 2^32 across two
mappings, TCP_SKB_CB(skb)->seq - subflow->ssn_offset wraps near U32_MAX while
subflow->map_subflow_seq is small. The unsigned < check evaluates to false,
leaving the old prefix bytes in skb and setting MPTCP_SKB_CB(skb)->map_seq to
mptcp_subflow_get_mapped_dsn(subflow) - offset, which corrupts the receive
stream.

Second, if a single DSS mapping starts near U32_MAX and wraps past 0, an skb
within that same mapping whose relative sequence number has wrapped past 0
with offset > 0 will satisfy unsigned < against subflow->map_subflow_seq and
falsely trigger the boundary branch.

> +		pr_debug("spans mappings seq=%u map_subflow_seq=%u offset=%d ssn_offset=%u\n",
> +			 TCP_SKB_CB(skb)->seq, subflow->map_subflow_seq,
> +			 offset, subflow->ssn_offset);
> +
> +		/* prefix bytes belong to old mapping, remove them and start
> +		 * from current mapping
> +		 */
> +		WARN_ON_ONCE(offset >= skb->len);
> +		skb_pull(skb, offset);
> +		offset = 0;
> +	}

[Severity: Critical]
Can calling skb_pull(skb, offset) in __mptcp_move_skbs_from_subflow() ->
mptcp_init_skb() trigger a kernel BUG() when the subflow receive skb is
non-linear?

When TCP queues incoming segments onto ssk->sk_receive_queue in
tcp_data_queue(), the TCP header has already been pulled via __skb_pull().
For non-linear skbs where payload bytes reside in skb_shinfo(skb)->frags[]
(such as packets sent by mptcp_sendmsg_frag() over loopback or veth, GRO
packets, page_pool RX packets, or segments coalesced via tcp_try_coalesce()),
skb_headlen(skb) is 0 or smaller than offset.

Calling skb_pull() invokes __skb_pull() in include/linux/skbuff.h:

static inline void *__skb_pull(struct sk_buff *skb, unsigned int len)
{
	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
#if defined(CONFIG_DEBUG_NET)
		skb->len += len;
		pr_err("__skb_pull(len=%u)\n", len);
		skb_dump(KERN_ERR, skb, false);
#endif
		BUG();
	}
	return skb->data += len;
}

When offset > skb_headlen(skb), skb->len becomes smaller than skb->data_len
and __skb_pull() hits BUG(). Would pskb_pull() or __pskb_pull_tail() be
needed here, or can the offset be handled without pulling paged data?

[1] https://lore.kernel.org/r/866f86ad2ea894fc21af7546ed0cf6f8b9d816de.1793695518.git.tanggeliang@kylinos.cn

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