Re: [PATCH mptcp-next v18 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 68366c7579c4f5b2392dcf76c7e487086dcb48df
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: defer read_sock cleanup to mptcp_worker

This series replaces the MPTCP SKB control buffer offset tracking with
msk->copied_seq, aligns struct mptcp_skb_cb with struct tcp_skb_cb using
32-bit sequence numbers, and defers __mptcp_read_sock() receive buffer
cleanup to mptcp_worker().

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

[ ... ]

> @@ -424,21 +421,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 +443,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 +494,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]
When __mptcp_move_skbs_from_subflow() passes a non-zero offset to
mptcp_init_skb() (for example, when the leading offset bytes of skb on
ssk->sk_receive_queue belonged to a prior or duplicate DSS mapping that was
already consumed or discarded via subflow_check_data_avail() ->
mptcp_subflow_discard_data()), mptcp_init_skb() subtracts offset from
mptcp_subflow_get_mapped_dsn(subflow):

	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;

If the new DSS mapping starting at offset has mapped_dsn > msk->ack_seq
(out-of-order at the MPTCP level because the range [msk->ack_seq, mapped_dsn)
is still in flight on another subflow), doesn't subtracting offset assign the
DSN range [mapped_dsn - offset, mapped_dsn) to the already-discarded bytes
0..offset of skb?

Specifically, in __mptcp_move_skb():

1. When 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.
Instead of queueing skb to msk->out_of_order_queue, it takes the in-sequence
or partial-packet insert path, advances msk->ack_seq across the unreceived
DSN gap, and causes __mptcp_recvmsg_mskq() to copy the stale subflow bytes at
0..offset to user-space.

2. When 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.

Can this corrupt the MPTCP receive stream when an out-of-order subflow SKB has
a non-zero offset?

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..899ffaba0b2b6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -515,6 +515,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  				   struct mptcp_subflow_context *subflow,
>  				   const struct mptcp_options_received *mp_opt)
>  {
> +	struct sock *sk = (struct sock *)msk;
> +
>  	/* active MPC subflow will reach here multiple times:
>  	 * at subflow_finish_connect() time and at 4th ack time
>  	 */
> @@ -533,6 +535,13 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  	WRITE_ONCE(msk->ack_seq, subflow->iasn);
>  	WRITE_ONCE(msk->can_ack, true);
>  	atomic64_set(&msk->rcv_wnd_sent, 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);
> +	}
>  }

[Severity: High]
When subflow_set_remote_key() runs 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 via MPTCP_SYNC_SEQ until mptcp_release_cb().
Could leaving msk->copied_seq unsynchronized while the user holds the socket
lock cause several issues?

First, in net/mptcp/protocol.c:mptcp_recvmsg():

		bytes_read = __mptcp_recvmsg_mskq(sk, msg, len - copied, flags,
						  seq, &tss, &cmsg_flags,
						  &last);
		...
		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk)) {
			if (flags & MSG_PEEK)
				peek_seq = msk->copied_seq + copied;
			continue;
		}

If payload arrives with or right after the MP_CAPABLE packet while the user
holds lock_sock(sk) in mptcp_recvmsg(), mptcp_data_ready() queues the
IASN-based SKBs onto msk->backlog_list. Before release_sock(sk) and
mptcp_release_cb() run, mptcp_recvmsg() calls mptcp_move_skbs(sk) and
continues to the next iteration of __mptcp_recvmsg_mskq(). Inside
mptcp_move_skbs() -> __mptcp_move_skb(), __mptcp_sync_rcv_sequence(sk) updates
any TFO SKB in sk->sk_receive_queue to the IASN sequence space and clears
msk->rcvd_dummy_seq, and queues the new IASN-based SKBs onto
sk->sk_receive_queue, while MPTCP_SYNC_SEQ is still pending and
msk->copied_seq has not been advanced by subflow->iasn.

On the next iteration, __mptcp_recvmsg_mskq() computes:

		u32 offset = (u32)(*seq) - MPTCP_SKB_CB(skb)->map_seq;

using the unsynchronized msk->copied_seq against the IASN-based map_seq. When
offset >= skb->len, __mptcp_recvmsg_mskq() treats the unread SKB as
already-peeked data, sets *last = skb, and skips it; sk_wait_data(sk, &timeo,
last) then blocks waiting for skb_peek_tail(&sk->sk_receive_queue) != last
even after release_sock(sk) runs MPTCP_SYNC_SEQ, stalling the reader with
unread data in the queue. When offset < skb->len, __mptcp_recvmsg_mskq()
copies from a bogus offset inside skb and eats the remainder.

Second, any call to mptcp_inq_hint(sk) while MPTCP_SYNC_SEQ is pending (via
mptcp_ioctl() for SIOCINQ, mptcp_recvmsg() for TCP_CM_INQ, or
mptcp_rcv_space_adjust()) subtracts the unsynchronized msk->copied_seq from
the IASN-based msk->ack_seq and returns INT_MAX.

Third, in net/mptcp/protocol.c:mptcp_release_cb():

	do {
		...
		if (__mptcp_move_skbs(sk))
			need_push = true;
		...
	} while (!list_empty(&msk->backlog_list));

	if (need_push)
		__mptcp_push_pending(sk, 0);

	if (msk->cb_flags) {
		...
		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
			msk->copied_seq += mptcp_iasn(msk);
			__mptcp_sync_rcv_sequence(sk);
		}
	}

Because __mptcp_move_skbs(sk) runs before MPTCP_SYNC_SEQ is handled, if a
spooled backlog SKB carries a DATA_FIN (arriving with or right after the 3rd
ACK while the user holds lock_sock(sk)), __mptcp_move_skbs() ->
mptcp_check_data_fin() increments msk->ack_seq by 1 without incrementing
msk->bytes_received, causing mptcp_iasn(msk) to shift msk->copied_seq one byte
past the real IASN.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793349369.git.geliang@kernel.org?part=5