Re: [PATCH mptcp-next v26 6/8] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] mptcp: subflow sk_rmem_alloc leak in __mptcp_move_skb() on
  rcv_data_fin drop
- [High] mptcp: rcv_data_fin guard bypass in __mptcp_move_skb()
  underflows offset
- [High] mptcp: out-of-order queue corruption and stall on non-zero skb
  offset
- [High] mptcp: wrong partial-packet copy_len in __mptcp_move_skb() when
  offset > 0
- [High] mptcp: __mptcp_try_coalesce() clobbers encoded offset on target
  skb
- [Medium] mptcp: commit message describes unimplemented
  mptcp_init_skb() changes
--

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

mptcp: remove CB offset field

This commit replaces the per-skb MPTCP_SKB_CB(skb)->offset field with an
msk-level copied_seq field to track the read offset in the MPTCP receive
queue.

> In mptcp_init_skb(), map_seq is computed as mptcp_subflow_get_mapped_dsn()
> minus offset. A mapping boundary check using before() for proper 32-bit
> modular arithmetic detects when the skb spans multiple DSS mappings
> (offset > 0 and skb starts before the current mapping). In that case,
> the consumed prefix bytes belong to an old mapping and must be removed
> before setting map_seq. The function now returns bool to indicate success.
>
> When removing the prefix, we handle linear and paged data separately.
> For offset within linear data where offset <= skb_headlen, use skb_pull()
> which is safe and doesn't modify truesize. For offset in paged data where
> offset > skb_headlen, use pskb_pull() which may expand the skb head. To
> avoid truesize corruption in pskb_expand_head(), we first orphan the skb
> to disconnect it from the socket, perform the pull, recompute truesize
> manually, then reattach with skb_set_owner_r(). This ensures proper memory
> accounting.
>
> When mptcp_init_skb() fails, the caller increments seq to skip the
> problematic skb, records the failure via MPTCP_MIB_RCVPRUNED, and jumps
> to cleanup to ensure tp->copied_seq and more_data_avail are properly
> updated. This maintains TCP/MPTCP state consistency, and MPTCP-level
> retransmission will fill any resulting gaps.
>
> When the skb stays within a single mapping and offset > 0, map_seq is
> rewound to cover the already-consumed prefix. A new MPTCP_SKB_CB_F_OFFSET
> flag marks such skbs. __mptcp_try_coalesce() refuses to coalesce when the
> from skb carries this flag, preventing an unvalidated prefix from being
> merged into a validated buffer. mptcp_data_queue_ofo() skips the
> rb_replace_node() replacement when the new skb covers an existing OFO
> entry and carries the flag, so that previously validated data is not
> overwritten by an unvalidated prefix; the new skb is inserted after the
> existing entry instead, and the prefix is naturally skipped when the OFO
> queue is later processed.

[Severity: Medium]
Does the commit message match the actual code changes in this patch?
mptcp_init_skb() still returns void and does not check mapping boundaries
or call skb_pull() / pskb_pull(), __mptcp_move_skbs_from_subflow() has no
failure handling or MPTCP_MIB_RCVPRUNED accounting, and
mptcp_data_queue_ofo() does not check MPTCP_SKB_CB_F_OFFSET before calling
rb_replace_node().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 41fc2429d2fed..99dfcd0488a45 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -167,8 +167,9 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
>  {
>  	int limit = READ_ONCE(sk->sk_rcvbuf);
>  
> -	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
> -	    MPTCP_SKB_CB(from)->offset ||
> +	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->map_seq +
> +					   to->len ||
> +	    MPTCP_SKB_CB(from)->flags & MPTCP_SKB_CB_F_OFFSET ||
>  	    ((to->len + from->len) > (limit >> 3)) ||
>  	    !skb_try_coalesce(to, from, fragstolen, delta))
>  		return false;

[Severity: High]
Does coalescing into a target skb that had offset > 0 lose its encoded
offset?

When to has offset > 0, mptcp_init_skb() sets to->map_seq = mapped_dsn -
offset and to->end_seq = to->map_seq + to->len - offset, so __mptcp_move_skb()
and __mptcp_ofo_queue() can recover offset from to->len - (to->end_seq -
to->map_seq).  When from (with offset == 0 and from->end_seq ==
from->map_seq + from->len) is coalesced into to in __mptcp_add_backlog()
or mptcp_data_queue_ofo(), __mptcp_try_coalesce() does:

net/mptcp/protocol.c:__mptcp_try_coalesce() {
    ...
	MPTCP_SKB_CB(to)->end_seq = MPTCP_SKB_CB(from)->end_seq;
	return true;
}

That sets to->end_seq to to->map_seq + to->len, wiping out the offset difference.
When to is later spooled from the backlog or out-of-order queue, dsn is
computed as mapped_dsn - offset instead of mapped_dsn, causing the stale
prefix bytes in to->data[0..offset - 1] to be treated as valid unconsumed
payload.

> @@ -306,7 +307,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  	struct sk_buff *skb1;
>  
>  	seq = MPTCP_SKB_CB(skb)->map_seq;
> -	end_seq = MPTCP_SKB_CB(skb)->end_seq;
> +	end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
>  	max_seq = atomic64_read(&msk->rcv_wnd_sent);

[ ... ]

> @@ -346,7 +347,8 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  	}
>  
>  	/* Can avoid an rbtree lookup if we are adding skb after ooo_last_skb */
> -	if (!before64(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq)) {
> +	if (!before64(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->map_seq +
> +		      msk->ooo_last_skb->len)) {
>  		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUETAIL);
>  		parent = &msk->ooo_last_skb->rbnode;
>  		p = &parent->rb_right;
> @@ -362,8 +364,9 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  			p = &parent->rb_left;
>  			continue;
>  		}
> -		if (before64(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
> -			if (!after64(end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
> +		if (before64(seq, MPTCP_SKB_CB(skb1)->map_seq + skb1->len)) {
> +			if (!after64(end_seq,
> +				     MPTCP_SKB_CB(skb1)->map_seq + skb1->len)) {
>  				/* All the bits are present. Drop. */
>  				mptcp_drop(sk, skb);
>  				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
> @@ -400,7 +403,8 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  merge_right:
>  	/* Remove other segments covered by skb. */
>  	while ((skb1 = skb_rb_next(skb)) != NULL) {
> -		if (before64(end_seq, MPTCP_SKB_CB(skb1)->end_seq))
> +		if (before64(end_seq,
> +			     MPTCP_SKB_CB(skb1)->map_seq + skb1->len))
>  			break;
>  		rb_erase(&skb1->rbnode, &msk->out_of_order_queue);
>  		mptcp_drop(sk, skb1);
> @@ -425,9 +429,10 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
>  	 * value
>  	 */
> -	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
> +	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> +				     offset;
>  	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
> -	MPTCP_SKB_CB(skb)->offset = offset;
> +	MPTCP_SKB_CB(skb)->flags = offset ? MPTCP_SKB_CB_F_OFFSET : 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;

[Severity: High]
Can an out-of-order skb with offset > 0 cause mptcp_data_queue_ofo() to
drop valid segments and stall the receive queue?

When offset > 0, mptcp_init_skb() rewinds MPTCP_SKB_CB(skb)->map_seq to
mapped_dsn - offset while the valid unconsumed payload only starts at
mapped_dsn.  mptcp_data_queue_ofo() uses seq = MPTCP_SKB_CB(skb)->map_seq
(mapped_dsn - offset) to position skb and deduplicate overlapping ranges,
so any valid out-of-order segment skb1 covering [mapped_dsn - offset,
mapped_dsn) is either replaced via rb_replace_node() / merge_right if it
was queued first, or dropped as MPTCP_MIB_DUPDATA if it arrives after skb.

When msk->ack_seq later reaches mapped_dsn - offset, __mptcp_ofo_queue()
checks dsn = mapped_dsn > msk->ack_seq and stops draining the RB-tree
because the segment covering [mapped_dsn - offset, mapped_dsn) was
dropped, stalling the connection.

[ ... ]

> @@ -450,17 +455,24 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)

[ ... ]

>  static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  {
>  	u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
> +	u64 dsn = MPTCP_SKB_CB(skb)->map_seq + skb->len - copy_len;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct sk_buff *tail;
>  
> +	if (READ_ONCE(msk->rcv_data_fin) &&
> +	    !before64(MPTCP_SKB_CB(skb)->map_seq,
> +		      READ_ONCE(msk->rcv_data_fin_seq))) {
> +		mptcp_drop(sk, skb);
> +		return false;
> +	}
> +
>  	mptcp_borrow_fwdmem(sk, skb);

[Severity: High]
Does calling mptcp_drop() before mptcp_borrow_fwdmem() leak
ssk->sk_rmem_alloc on the subflow socket?

Both callers of __mptcp_move_skb() call mptcp_subflow_lend_fwdmem() first,
which clears skb->destructor = NULL while leaving skb->truesize charged to
ssk->sk_rmem_alloc until mptcp_borrow_fwdmem() transfers the forward
memory to sk and subtracts skb->truesize from ssk->sk_rmem_alloc.
Dropping skb before mptcp_borrow_fwdmem() skips that uncharge on ssk (so
closing the subflow triggers the sk_rmem_alloc warning in
inet_sock_destruct()) and calls sk_mem_reclaim(sk) without having credited
sk->sk_forward_alloc.

[Severity: High]
Can data arriving after DATA_FIN has been processed bypass this
rcv_data_fin check and underflow the receive offset?

Once mptcp_check_data_fin() processes the DATA_FIN, it increments
msk->ack_seq to rcv_data_fin_seq + 1 and clears msk->rcv_data_fin to 0:

net/mptcp/protocol.c:mptcp_check_data_fin() {
    ...
	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
		WRITE_ONCE(msk->rcv_data_fin, 0);

		WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
    ...
}

If a peer then sends a segment at dsn = rcv_data_fin_seq + 1 on an open
subflow, READ_ONCE(msk->rcv_data_fin) is 0, so __mptcp_move_skb() accepts
the skb as in-sequence (dsn == msk->ack_seq) and queues it with map_seq =
rcv_data_fin_seq + 1 while msk->copied_seq is at most rcv_data_fin_seq.
In __mptcp_recvmsg_mskq(), offset = *seq - MPTCP_SKB_CB(skb)->map_seq
underflows and skips the skb without consuming or freeing it.  In
addition, while msk->rcv_data_fin is still set, checking map_seq instead
of map_seq + skb->len misses segments that straddle rcv_data_fin_seq (or
have map_seq < rcv_data_fin_seq <= dsn due to offset > 0) and lets them
advance msk->ack_seq past rcv_data_fin_seq.

[ ... ]

> @@ -495,10 +507,6 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  	/* Partial packet */
>  	if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
>  		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;
> -		MPTCP_SKB_CB(skb)->map_seq += msk->ack_seq -
> -					      MPTCP_SKB_CB(skb)->map_seq;
>  		goto insert;
>  	}

[Severity: High]
Does the partial-packet check use the wrong end sequence when offset > 0?

In mptcp_init_skb(), MPTCP_SKB_CB(skb)->end_seq is set to map_seq +
copy_len (= dsn + copy_len - offset), which is offset bytes less than the
actual end of the segment (map_seq + skb->len).  When before64(dsn,
msk->ack_seq) and offset > 0, using MPTCP_SKB_CB(skb)->end_seq here
falsely drops the skb as duplicate data if msk->ack_seq falls in
[end_seq, map_seq + skb->len), or under-increments msk->ack_seq and
msk->bytes_received by offset bytes while __mptcp_recvmsg_mskq() and
__mptcp_read_sock() still copy up to skb->len (advancing msk->copied_seq
past msk->ack_seq).

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