Re: [PATCH mptcp-next v27 7/8] mptcp: sync mptcp skb cb layout with tcp one

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mptcp: use of offset-shifted map_seq instead of mptcp_dsn() in
  OoO queue
- [Medium] mptcp: copied_seq offset underflow in recv path for data
  after DATA_FIN
--

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

mptcp: sync mptcp skb cb layout with tcp one

This commit aligns the MPTCP skb control buffer layout with TCP's by
shrinking map_seq and end_seq to 32-bit values, adding a 64-bit map_seq64
field, and switching the receive and out-of-order paths to 32-bit sequence
arithmetic where possible.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f4ab3bdd368f2..44f4a680a4bdb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -256,8 +256,8 @@ static void mptcp_prune_ofo_queue(struct sock *sk,
>  		struct sk_buff *skb = rb_to_skb(node);
>  
>  		/* Stop pruning if the incoming skb would land in OoO tail. */
> -		if (after64(MPTCP_SKB_CB(in_skb)->map_seq,
> -			    MPTCP_SKB_CB(skb)->map_seq))
> +		if (after(MPTCP_SKB_CB(in_skb)->map_seq,
> +			  MPTCP_SKB_CB(skb)->map_seq))
>  			break;
>  
>  		pruned = true;

[ ... ]

> @@ -359,18 +360,18 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  	while (*p) {
>  		parent = *p;
>  		skb1 = rb_to_skb(parent);
> -		if (before64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
> +		if (before(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
>  			p = &parent->rb_left;
>  			continue;
>  		}
> -		if (before64(seq, mptcp_end_dsn(skb1))) {
> -			if (!after64(end_seq, mptcp_end_dsn(skb1))) {
> +		if (before(seq, (u32)mptcp_end_dsn(skb1))) {
> +			if (!after((u32)end_seq, (u32)mptcp_end_dsn(skb1))) {
>  				/* All the bits are present. Drop. */
>  				mptcp_drop(sk, skb);
>  				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
>  				return;
>  			}
> -			if (after64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
> +			if (after(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
>  				/* partial overlap:
>  				 *     |     skb      |
>  				 *  |     skb1    |

[ ... ]

> @@ -424,10 +425,11 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  
>  	/* the skb map_seq accounts for the skb offset:
>  	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
> -	 * value
> +	 * value; note that end seq number is only available in 32bits format.
>  	 */
> -	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> -				     offset;
> +	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
> +				       offset;
> +	MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
>  	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
>  	MPTCP_SKB_CB(skb)->flags = offset ? MPTCP_SKB_CB_F_OFFSET : 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;

[Severity: High]
Can comparing against MPTCP_SKB_CB(skb)->map_seq instead of
(u32)mptcp_dsn(skb) in mptcp_data_queue_ofo() and mptcp_prune_ofo_queue()
drop valid out-of-order segments or corrupt the RB-tree ordering?

mptcp_init_skb() subtracts offset from map_seq64 and map_seq, so for any
out-of-order skb with a non-zero subflow offset (for example, when a
subflow skb spans two DSS mappings and the first mapping was already
discarded), MPTCP_SKB_CB(skb1)->map_seq is (u32)mptcp_dsn(skb1) - offset
rather than the starting DSN of the mapped payload.

When a subsequent segment arrives covering the preceding range
[mptcp_dsn(skb1) - offset, mptcp_dsn(skb1)), its seq ((u32)mptcp_dsn(skb))
is not before MPTCP_SKB_CB(skb1)->map_seq.  In mptcp_data_queue_ofo(), it
misses the rb_left branch and is either falsely dropped as duplicate data
(if end_seq <= mptcp_end_dsn(skb1)) or inserted into the right subtree of
skb1 despite having a smaller DSN (if end_seq > mptcp_end_dsn(skb1)),
stalling the connection once msk->ack_seq reaches the missing range.
mptcp_prune_ofo_queue() likewise compares raw map_seq values instead of
(u32)mptcp_dsn().

[ ... ]

> @@ -2212,7 +2214,7 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
>  	int copied = 0;
>  
>  	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
> -		u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
> +		u32 offset = (u32)(*seq) - MPTCP_SKB_CB(skb)->map_seq;
>  		u32 data_len = skb->len - offset;
>  		u32 count;
>  		int err;
> @@ -4635,7 +4637,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
>  		mptcp_move_skbs(sk);
>  
>  	while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
> -		offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
> +		offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
>  		if (offset < skb->len) {
>  			*off = offset;
>  			return skb;

[Severity: Medium]
Can offset underflow in __mptcp_recvmsg_mskq() and mptcp_recv_skb() if a
peer sends mapped data at or after DATA_FIN?

When mptcp_check_data_fin() processes an incoming DATA_FIN at
rcv_data_fin_seq, it advances msk->ack_seq to rcv_data_fin_seq + 1 without
advancing msk->copied_seq:

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);
    ...
}

Because __mptcp_move_skb() does not reject segments after DATA_FIN, a
subsequent segment with dsn == rcv_data_fin_seq + 1 matches
dsn == msk->ack_seq, increments msk->bytes_received, and is queued to
sk->sk_receive_queue with map_seq == (u32)(rcv_data_fin_seq + 1).  When
__mptcp_recvmsg_mskq() or mptcp_recv_skb() inspects that skb with
msk->copied_seq == rcv_data_fin_seq, offset underflows to 0xffffffff >=
skb->len and mptcp_eat_recv_skb() frees the skb without updating
msk->bytes_consumed, leaving mptcp_data_avail(msk) permanently non-zero
and causing __mptcp_close() to trigger mptcp_do_fastclose().

If the post-DATA_FIN segment instead starts at rcv_data_fin_seq with its
first byte discarded by mptcp_subflow_discard_data() (offset == 1 in
mptcp_init_skb()), map_seq equals (u32)rcv_data_fin_seq, so offset in
__mptcp_recvmsg_mskq() evaluates to 0 instead of 1 and copies the
discarded prefix byte to userspace.

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