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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mptcp: sk_forward_alloc corruption in mptcp_init_skb() on paged
  pull
- [Medium] mptcp: rewound map_seq in mptcp_init_skb() exposes
  unvalidated prefix
--

commit 53ea4c3d9a0507effa8276bb08b2e103414e8c2d
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 counter and handles already-consumed skb prefixes in
mptcp_init_skb().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ce0f9c2e6fd1a..30933dd9f0b1d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -375,6 +375,11 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  				 *  |     skb1    |
>  				 * continue traversing
>  				 */
> +			} else if (MPTCP_SKB_CB(skb)->flags &
> +				   MPTCP_SKB_CB_F_OFFSET) {
> +				/* skb covers skb1 but has an unvalidated
> +				 * prefix; keep skb1, continue traversing.
> +				 */
>  			} else {
>  				/* skb's seq == skb1's seq and skb covers skb1.
>  				 * Replace skb1 with skb.
> @@ -415,25 +420,54 @@ 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 bool 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 &&
> +		     before(TCP_SKB_CB(skb)->seq - subflow->ssn_offset,
> +			    subflow->map_subflow_seq))) {
> +		if (offset >= skb->len) {
> +			sk_eat_skb(ssk, skb);
> +			return false;
> +		}
> +
> +		if (offset > skb_headlen(skb)) {
> +			skb_orphan(skb);
> +			if (!pskb_pull(skb, offset)) {
> +				sk_eat_skb(ssk, skb);
> +				return false;
> +			}
> +			skb->truesize = SKB_TRUESIZE(skb_end_offset(skb)) +
> +					skb->data_len;
> +			skb_set_owner_r(skb, ssk);
> +		} else {
> +			if (!skb_pull(skb, offset)) {
> +				sk_eat_skb(ssk, skb);
> +				return false;
> +			}
> +		}
> +		offset = 0;
> +	}

[Severity: High]
Does orphaning and re-charging skb here corrupt ssk->sk_forward_alloc and
subflow->lent_mem_frag?

skb_orphan(skb) invokes sock_rfree() -> sk_mem_uncharge(ssk, orig_truesize),
which calls sk_mem_reclaim() and returns whole pages from
ssk->sk_forward_alloc to tcp_memory_allocated.  skb_set_owner_r(skb, ssk)
then calls sk_mem_charge(ssk, skb->truesize) without sk_rmem_schedule(),
subtracting the new truesize from ssk->sk_forward_alloc without allocating
backing pages, and altering skb->truesize breaks the PAGE_SIZE alignment
that mptcp_subflow_lend_fwdmem() and __mptcp_close_ssk() rely on.  When the
subflow closes, ssk->sk_forward_alloc is left negative (triggering
WARN_ON_ONCE(sk->sk_forward_alloc) in inet_sock_destruct()) while msk
reclaims the transferred pages a second time.

Note that pskb_expand_head() only adjusts skb->truesize when !skb->sk or
skb->destructor == sock_edemux, so calling pskb_pull() directly while skb is
still owned by ssk avoids changing skb->truesize or touching
ssk->sk_forward_alloc.

> +
>  	/* the skb map_seq accounts for the skb 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)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
> -	MPTCP_SKB_CB(skb)->offset = offset;
> +	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 + skb->len;
> +	MPTCP_SKB_CB(skb)->flags = offset ? MPTCP_SKB_CB_F_OFFSET : 0;

[Severity: Medium]
Can rewinding MPTCP_SKB_CB(skb)->map_seq by offset expose the unvalidated
prefix to the receive and out-of-order queues?

When an incoming subflow segment overlaps tp->copied_seq within the current
DSS mapping, validate_data_csum() starts at tp->copied_seq +
subflow->map_csum_len and does not validate the first offset bytes of skb.
Because mptcp_init_skb() rewinds map_seq to include that prefix and only
records a single bit in MPTCP_SKB_CB_F_OFFSET:

In mptcp_data_queue_ofo(), if the already-validated range in
msk->out_of_order_queue spans more than one skb (for example skb1a covering
[1000, 1050) and skb1b covering [1050, 1100)) and a segment arrives with
mapped_dsn 1100 and offset 100, traversal skips replacing skb1a via the
MPTCP_SKB_CB_F_OFFSET check, moves right to skb1b, sees seq (1000) <
skb1b->map_seq (1050), inserts skb to the left of skb1b, and then drops the
checksum-validated skb1b in merge_right, replacing [1050, 1100) with skb's
unvalidated prefix.  Similarly, mptcp_prune_ofo_queue() compares the rewound
in_skb->map_seq against the validated ooo_last_skb and can prune it, and in
__mptcp_move_skb(), if the earlier segment that advanced tp->copied_seq was
dropped at the msk level due to backlog or rmem limits before advancing
msk->ack_seq, the unvalidated prefix is accepted as in-sequence data.

Pulling offset bytes in mptcp_init_skb() whenever offset > 0 (not only
across mapping boundaries) would strip the unvalidated prefix before map_seq
is set.

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