Re: [PATCH mptcp-next v19 5/5] mptcp: defer read_sock cleanup to mptcp_worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: unsynchronized copied_seq when MPTCP_SYNC_SEQ is deferred
- [High] mptcp: wrong map_seq64 in mptcp_init_skb() on non-zero skb offset
--

commit 1033c608c403db5f17f6be77476b0bb66333000f
Author: Geliang Tang <geliang@kernel.org>

mptcp: defer read_sock cleanup to mptcp_worker

This series reworks MPTCP receive sequence tracking to use msk->copied_seq and
32-bit SKB control block sequence numbers aligned with TCP, and defers
__mptcp_read_sock() receive buffer autotuning and cleanup to mptcp_worker().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..13078f2f3c498 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -424,21 +423,21 @@ 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;
>  
>  	/* 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);
> -	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_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 + skb->len;
> +	MPTCP_SKB_CB(skb)->flags = 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
> -	MPTCP_SKB_CB(skb)->cant_coalesce = 0;
>  
>  	__skb_unlink(skb, &ssk->sk_receive_queue);
>  
> @@ -446,15 +445,40 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	skb_dst_drop(skb);
>  }

[ ... ]

>  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;
> +	u32 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct sk_buff *tail;
>  
>  	mptcp_borrow_fwdmem(sk, skb);
>  
> -	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
> +	/* Be sure to sync the eventual fastopen dummy mapping before any other
> +	 * skb lands into the msk.
> +	 */
> +	if (unlikely(msk->rcvd_dummy_seq))
> +		__mptcp_sync_rcv_sequence(sk);
> +
> +	if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
>  		/* in sequence */
>  insert:
>  		if (!mptcp_try_rmem_schedule(sk, skb)) {
> @@ -472,18 +496,14 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  		skb_set_owner_r(skb, sk);
>  		__skb_queue_tail(&sk->sk_receive_queue, skb);
>  		return true;
> -	} else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
> +	} else if (after64(MPTCP_SKB_CB(skb)->map_seq64, msk->ack_seq)) {
>  		mptcp_data_queue_ofo(msk, skb);
>  		return false;
>  	}
>  
>  	/* 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;
> +	if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
> +		copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
>  		goto insert;
>  	}

[Severity: High]
As reported by Sashiko on v18 [1], can subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) in mptcp_init_skb() corrupt the receive
stream when an out-of-order subflow skb has a non-zero offset?

In __mptcp_move_skbs_from_subflow(), offset = seq - TCP_SKB_CB(skb)->seq is
non-zero when the leading offset bytes of skb on ssk->sk_receive_queue were
already consumed or discarded (for instance, when skb spans the end of a
duplicate DSS mapping discarded by mptcp_subflow_discard_data() in
subflow_check_data_avail() and the start of a new DSS mapping at
tp->copied_seq).

Setting MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
offset assigns DSNs [mapped_dsn - offset, mapped_dsn) to those already
discarded leading bytes 0..offset of skb.

When the new DSS mapping starting at offset is out-of-order at the MPTCP level
(mapped_dsn > msk->ack_seq because the intervening DSN range
[msk->ack_seq, mapped_dsn) is in flight on another subflow):

1. If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb()
sees map_seq64 <= msk->ack_seq and map_seq64 + skb->len > msk->ack_seq. It
treats the out-of-order skb as in-sequence or a partial packet, queues it
onto sk->sk_receive_queue, and advances msk->ack_seq across the missing DSN
gap [msk->ack_seq, mapped_dsn), causing __mptcp_recvmsg_mskq() or
mptcp_recv_skb() to copy the stale leading bytes in 0..offset to user space.

2. If mapped_dsn - offset > msk->ack_seq, __mptcp_move_skb() queues skb into
msk->out_of_order_queue with map_seq = mapped_dsn - offset, so
mptcp_data_queue_ofo() and __mptcp_ofo_queue() treat bytes 0..offset as valid
data for [mapped_dsn - offset, mapped_dsn) and drop the real segments arriving
on other subflows as duplicate data.

[ ... ]

> @@ -2383,6 +2386,11 @@ static bool mptcp_move_skbs(struct sock *sk)
>  	bool enqueued = false;
>  	u32 moved = 0;
>  
> +	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		msk->copied_seq += msk->tfo_iasn;
> +		__mptcp_sync_rcv_sequence(sk);
> +	}
> +
>  	mptcp_data_lock(sk);
>  	while (mptcp_can_spool_backlog(sk, &skbs)) {
>  		mptcp_data_unlock(sk);
> @@ -2402,22 +2410,17 @@ static bool mptcp_move_skbs(struct sock *sk)
>  static unsigned int mptcp_inq_hint(const struct sock *sk)
>  {
>  	const struct mptcp_sock *msk = mptcp_sk(sk);
> -	const struct sk_buff *skb;
> +	u64 hint_val;
>  
> -	skb = skb_peek(&sk->sk_receive_queue);
> -	if (skb) {
> -		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +	hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> +	if (hint_val >= INT_MAX)
> +		return INT_MAX;
>  
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> -
> -		return (unsigned int)hint_val;
> -	}
> -
> -	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> +	if (!hint_val &&
> +	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
>  		return 1;
>  
> -	return 0;
> +	return (unsigned int)hint_val;
>  }
>  
>  static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,

[ ... ]

> @@ -2463,8 +2472,11 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  
>  		copied += bytes_read;
>  
> -		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk))
> +		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk)) {
> +			if (flags & MSG_PEEK)
> +				peek_seq = msk->copied_seq + copied;
>  			continue;
> +		}

[Severity: High]
As reported by Sashiko on v18 [2], can deferring the msk->copied_seq update
via MPTCP_SYNC_SEQ leave msk->copied_seq out of sync with msk->ack_seq and
sk->sk_receive_queue while the socket lock is held?

In subflow_set_remote_key() in net/mptcp/subflow.c, when the remote key and
IASN arrive in BH context while sock_owned_by_user(sk) is true, msk->ack_seq
is updated immediately to subflow->iasn while updating msk->copied_seq is
deferred:

	WRITE_ONCE(msk->ack_seq, subflow->iasn);
	WRITE_ONCE(msk->can_ack, true);
	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
	msk->tfo_iasn = subflow->iasn;

	if (!sock_owned_by_user(sk)) {
		msk->copied_seq += subflow->iasn;
		__mptcp_sync_rcv_sequence(sk);
	} else {
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}

This appears to leave several paths with an unsynchronized msk->copied_seq:

1. In mptcp_move_skbs(), test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) is
checked only once before mptcp_data_lock(sk) rather than inside the
while (mptcp_can_spool_backlog(sk, &skbs)) loop. If the 3rd ACK and payload
arrive in BH after that initial check or while mptcp_data_lock(sk) is dropped
around __mptcp_move_skbs(), the next iteration of the loop spools the new skb
via __mptcp_move_skb(), which calls __mptcp_sync_rcv_sequence(sk) (updating
the TFO skb in sk->sk_receive_queue to the IASN sequence space and clearing
msk->rcvd_dummy_seq) and queues the new IASN-based skb while msk->copied_seq
remains unsynchronized for the next iteration of mptcp_recvmsg().

2. While MPTCP_SYNC_SEQ is pending, mptcp_inq_hint() subtracts the
pre-IASN msk->copied_seq from the IASN-based msk->ack_seq and returns INT_MAX.
In mptcp_recvmsg(), if the 3rd ACK or SYN-ACK arrives without payload while
lock_sock(sk) is held, msk->backlog_list is empty so mptcp_move_skbs() is not
called; both mptcp_rcv_space_adjust() and the TCP_CM_INQ cmsg calculation run
before release_sock(sk) and observe INT_MAX from mptcp_inq_hint(sk). Similarly,
in mptcp_ioctl() for SIOCINQ, if the remote key arrives in BH after
mptcp_move_skbs() checks MPTCP_SYNC_SEQ and before mptcp_inq_hint(sk), SIOCINQ
returns INT_MAX.

3. In mptcp_recvmsg(), when flags & MSG_PEEK is set and mptcp_move_skbs(sk)
consumes MPTCP_SYNC_SEQ (updating msk->copied_seq and the TFO skb map_seq) but
returns false because the backlog skb was out-of-order or dropped, peek_seq is
only updated inside the if (mptcp_move_skbs(sk)) branch and remains stale if
the loop continues without sleeping.

[1] https://lore.kernel.org/r/b6e4ed70642e2f86c041381cea810db82116b836.1793349369.git.geliang@kernel.org
[2] https://lore.kernel.org/r/b6e4ed70642e2f86c041381cea810db82116b836.1793349369.git.geliang@kernel.org

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