Re: [PATCH mptcp-next v18 3/5] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mptcp: unsynchronized copied_seq when MPTCP_SYNC_SEQ is deferred
- [High] mptcp: off-by-one in mptcp_iasn() when DATA_FIN precedes MPTCP_SYNC_SEQ
- [High] mptcp: stream corruption in mptcp_init_skb() on non-zero skb offset
--

commit 1889ab1df71af9d2613dfc80ed132ac8e6d02071
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 to track consumed bytes in the MPTCP receive
queue. It also adds deferred sequence synchronization via MPTCP_SYNC_SEQ and
mptcp_iasn() for passive FastOpen sockets when the remote key arrives while
the socket is owned by user space.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index fe154b1816834..792147b5279f9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -168,7 +168,6 @@ 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 ||
>  	    ((to->len + from->len) > (limit >> 3)) ||
>  	    !skb_try_coalesce(to, from, fragstolen, delta))
>  		return false;
> @@ -415,8 +414,7 @@ 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;
> @@ -425,9 +423,9 @@ 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)->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)->has_rxtstamp = has_rxtstamp;
>  
>  	__skb_unlink(skb, &ssk->sk_receive_queue);

[Severity: High]
As reported by Sashiko on v17 [1], can subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) in mptcp_init_skb() and dropping the
offset check in __mptcp_try_coalesce() corrupt the MPTCP data stream when an
skb has a non-zero subflow offset?

An skb at the head of ssk->sk_receive_queue can reach
__mptcp_move_skbs_from_subflow() with offset = tp->copied_seq -
TCP_SKB_CB(skb)->seq > 0 while carrying a new DSS mapping M2 starting at
tp->copied_seq (skb->data[offset..skb->len)) with mapped_dsn =
mptcp_subflow_get_mapped_dsn(subflow):

1. In subflow_check_data_avail() -> get_mapping_status(), a single skb can
   span the tail of a previous DSS mapping M1 in skb->data[0..offset) and the
   start of a new DSS mapping M2 in skb->data[offset..skb->len):

   net/mptcp/subflow.c:get_mapping_status() {
       ...
       if (!skb_is_fully_mapped(ssk, skb)) {
           /* will validate the next map after consuming the current one */
           goto validate_csum;
       }
   }

   If M1 was already received on another subflow (ack_seq < msk->ack_seq),
   mptcp_subflow_discard_data() advances tp->copied_seq by offset without
   eating the skb and resets subflow->map_valid = 0. On the next iteration,
   get_mapping_status() installs M2 starting offset bytes into the skb.

2. Similarly, tcp_data_queue() or tcp_ofo_queue() can queue a partially
   overlapping TCP segment with TCP_SKB_CB(skb)->seq < tp->copied_seq <
   TCP_SKB_CB(skb)->end_seq at a DSS mapping boundary.

Because MPTCP stripes mappings across subflows, M1 and M2 on the same subflow
have contiguous SSNs but non-contiguous MPTCP DSNs, so skb->data[0..offset)
does not belong to the DSN range [mapped_dsn - offset, mapped_dsn). Setting
MPTCP_SKB_CB(skb)->map_seq = mapped_dsn - offset (and map_seq64 = mapped_dsn -
offset in the follow-up commit syncing the skb cb layout with tcp) causes
several problems when M2 is out of order (mapped_dsn > msk->ack_seq) or
queued to msk->backlog_list:

- If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb()
  treats the out-of-order skb as in-sequence or partially overlapping,
  advances msk->ack_seq and msk->bytes_received across the unreceived gap
  [msk->ack_seq, mapped_dsn), coalesces skb->data[0..skb->len) onto the
  receive queue tail via mptcp_try_coalesce() if mapped_dsn - offset ==
  MPTCP_SKB_CB(tail)->end_seq, delivers the stale M1 bytes in
  skb->data[0..offset) to user space, and causes the genuine data for
  [msk->ack_seq, mapped_dsn) to be dropped later as MPTCP_MIB_DUPDATA.

- If mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts or
  coalesces the skb in msk->out_of_order_queue under the fabricated range
  [mapped_dsn - offset, end_seq), dropping or replacing legitimate
  out-of-order segments covering [mapped_dsn - offset, mapped_dsn) and
  delivering skb->data[0..offset) to user space when __mptcp_ofo_queue()
  dequeues it.

- In __mptcp_add_backlog(), if mapped_dsn - offset ==
  MPTCP_SKB_CB(tail)->end_seq for a prior backlog skb, __mptcp_try_coalesce()
  coalesces skb->data[0..skb->len) including the stale prefix
  skb->data[0..offset) into tail.

> @@ -440,7 +438,6 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct sk_buff *skb;
> -	u32 offset;
>  
>  	if (likely(!msk->rcvd_dummy_seq))
>  		return;
> @@ -451,9 +448,8 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
>  	if (!skb)
>  		return;
>  
> -	offset = MPTCP_SKB_CB(skb)->offset;
> -	MPTCP_SKB_CB(skb)->map_seq = msk->ack_seq - skb->len + offset;
> -	MPTCP_SKB_CB(skb)->end_seq = msk->ack_seq;
> +	MPTCP_SKB_CB(skb)->map_seq = mptcp_iasn(msk) - skb->len;
> +	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
>  }

[ ... ]

> @@ -2209,33 +2198,24 @@ static void mptcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
>  }
>  
>  static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
> -				size_t len, int flags, int copied_total,
> +				size_t len, int flags, u64 *seq,
>  				struct scm_timestamping_internal *tss,
>  				int *cmsg_flags, struct sk_buff **last)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct sk_buff *skb, *tmp;
> -	int total_data_len = 0;
>  	int copied = 0;
>  
>  	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
> -		u32 delta, offset = MPTCP_SKB_CB(skb)->offset;
> +		u64 offset = *seq - MPTCP_SKB_CB(skb)->map_seq;
>  		u32 data_len = skb->len - offset;
>  		u32 count;
>  		int err;
>  
> -		if (flags & MSG_PEEK) {
> -			/* skip already peeked skbs */
> -			if (total_data_len + data_len <= copied_total) {
> -				total_data_len += data_len;
> -				*last = skb;
> -				continue;
> -			}
> -
> -			/* skip the already peeked data in the current skb */
> -			delta = copied_total - total_data_len;
> -			offset += delta;
> -			data_len -= delta;
> +		/* Skip the already peeked data. */
> +		if (offset >= skb->len) {
> +			*last = skb;
> +			continue;
>  		}

[ ... ]

> @@ -2417,22 +2395,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;
> -
> -	skb = skb_peek(&sk->sk_receive_queue);
> -	if (skb) {
> -		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +	u64 hint_val;
>  
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> +	hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> +	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;
>  }

[ ... ]

> @@ -2478,8 +2457,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;
> +		}

[ ... ]

> @@ -3955,8 +3943,10 @@ static void mptcp_release_cb(struct sock *sk)
>  			__mptcp_error_report(sk);
>  		if (__test_and_clear_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags))
>  			__mptcp_sync_sndbuf(sk);
> -		if (__test_and_clear_bit(MPTCP_SYNC_SEQ, &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);
> +		}
>  	}
>  }
>  
> @@ -4628,7 +4618,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 = MPTCP_SKB_CB(skb)->offset;
> +		offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
>  		if (offset < skb->len) {
>  			*off = offset;
>  			return skb;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index c317d8b59fffb..985ef15275eb0 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h

[ ... ]

> @@ -884,6 +885,11 @@ struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk);
>  int mptcp_sched_get_send(struct mptcp_sock *msk);
>  int mptcp_sched_get_retrans(struct mptcp_sock *msk);
>  
> +static inline u64 mptcp_iasn(const struct mptcp_sock *msk)
> +{
> +	return msk->ack_seq - msk->bytes_received + msk->tfo_skb_len;
> +}
> +
>  static inline u64 mptcp_data_avail(const struct mptcp_sock *msk)
>  {
>  	return READ_ONCE(msk->bytes_received) - READ_ONCE(msk->bytes_consumed);

[Severity: High]
As reported by Sashiko on v15 [2], won't mptcp_iasn() be off by one if a
DATA_FIN is processed before the deferred MPTCP_SYNC_SEQ runs in
mptcp_release_cb()?

The commit message notes that mptcp_check_data_fin() incrementing ack_seq
before MPTCP_SYNC_SEQ runs would require a DATA_FIN to arrive before the MPC
third ACK on a passive TFO socket. However, subflow_set_remote_key() sets
MPTCP_SYNC_SEQ at the moment the MPC third ACK (or SYN-ACK on an active
socket) arrives while sock_owned_by_user(sk) is true, which also establishes
the connection and sets msk->ack_seq = subflow->iasn.

If the peer sends a payload with DATA_FIN (or a subflow closes immediately)
together with or right after the MPC third ACK while lock_sock(sk) is still
held:

1. When data + DATA_FIN is queued to msk->backlog_list while lock_sock(sk) is
   held, release_sock(sk) invokes mptcp_release_cb(sk), which spools the
   backlog via __mptcp_move_skbs() in its loop before checking MPTCP_SYNC_SEQ
   at the end of the function:

   net/mptcp/protocol.c:__mptcp_move_skbs() {
       ...
       __mptcp_ofo_queue(msk);
       if (moved)
           mptcp_check_data_fin((struct sock *)msk);
       return moved;
   }

   Inside __mptcp_move_skbs(), __mptcp_move_skb() syncs any TFO skb using
   mptcp_iasn(msk) == subflow->iasn and queues the new skb at map_seq =
   subflow->iasn, and then mptcp_check_data_fin() increments msk->ack_seq by 1
   without incrementing msk->bytes_received. When mptcp_release_cb() then
   processes MPTCP_SYNC_SEQ, msk->copied_seq += mptcp_iasn(msk) adds
   subflow->iasn + 1, advancing msk->copied_seq one byte past the queued skb's
   map_seq and skipping the first byte of unread payload.

2. Similarly, if a 0-payload DATA_FIN arrives after the third ACK while
   mptcp_worker() holds lock_sock(sk), mptcp_worker() calls
   mptcp_check_data_fin(sk) and increments msk->ack_seq to subflow->iasn + 1
   before release_sock(sk) runs mptcp_release_cb(sk).

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 49a000b8dcb8e..899ffaba0b2b6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -536,10 +536,12 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  	WRITE_ONCE(msk->can_ack, true);
>  	atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
>  
> -	if (!sock_owned_by_user(sk))
> +	if (!sock_owned_by_user(sk)) {
> +		msk->copied_seq += subflow->iasn;
>  		__mptcp_sync_rcv_sequence(sk);
> -	else
> +	} else {
>  		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
> +	}
>  }

[Severity: High]
As reported by Sashiko on v15 [2], when subflow_set_remote_key() runs in BH
context while sock_owned_by_user(sk) is true, it immediately updates
msk->ack_seq to subflow->iasn and sets msk->can_ack = true, but defers
updating msk->copied_seq via MPTCP_SYNC_SEQ until mptcp_release_cb(). What
happens if the user context holding lock_sock(sk) accesses msk->copied_seq or
spools the backlog before release_sock(sk) runs?

Specifically, if the peer's MP_CAPABLE third ACK (on a passive TFO socket) or
SYN-ACK (on an active client socket) arrives with or followed by data while
user space holds lock_sock(sk):

1. In mptcp_recvmsg(), mptcp_recv_skb(), and mptcp_ioctl(SIOCINQ),
   mptcp_move_skbs(sk) is called while holding lock_sock(sk). Inside
   mptcp_move_skbs() -> __mptcp_move_skbs() -> __mptcp_move_skb(),
   __mptcp_sync_rcv_sequence(sk) rebases any queued TFO skb's map_seq from
   -(u64)skb->len to mptcp_iasn(msk) - skb->len and clears
   msk->rcvd_dummy_seq = false, and __mptcp_move_skb() moves the IASN-mapped
   backlog skbs into sk->sk_receive_queue. However, MPTCP_SYNC_SEQ is still
   pending, so msk->copied_seq (and peek_seq = msk->copied_seq + copied in
   mptcp_recvmsg()) remains in the pre-IASN sequence space.

2. In mptcp_recv_skb(), the loop then computes:

   offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
   if (offset < skb->len) {
       *off = offset;
       return skb;
   }
   mptcp_eat_recv_skb(sk, skb);

   Because msk->copied_seq has not been rebased by subflow->iasn, offset
   underflows to a value >= skb->len, and mptcp_eat_recv_skb() frees and drops
   every unread skb in sk->sk_receive_queue without advancing msk->copied_seq
   or msk->bytes_consumed. Even after the later commit in this series ("mptcp:
   defer read_sock cleanup to mptcp_worker") removes mptcp_move_skbs() from
   mptcp_recv_skb(), mptcp_release_cb() still runs __mptcp_move_skbs() and
   calls sk->sk_data_ready(sk) in its loop before processing MPTCP_SYNC_SEQ,
   so kTLS RX synchronously invokes __mptcp_read_sock() -> mptcp_recv_skb()
   with an unsynchronized msk->copied_seq and drops the queued skbs.

3. In mptcp_recvmsg(), the next iteration of __mptcp_recvmsg_mskq() computes
   offset = *seq - MPTCP_SKB_CB(skb)->map_seq, which similarly underflows by
   subflow->iasn; when offset >= skb->len, it treats all unread skbs in
   sk->sk_receive_queue as already-peeked data, sets *last = skb, and blocks
   in sk_wait_data() (or copies from a corrupted mid-skb offset if a 32-bit
   offset wraps below skb->len in the follow-up commit).

4. In mptcp_inq_hint() (called under lock_sock(sk) by mptcp_ioctl(SIOCINQ),
   TCP_CM_INQ, and mptcp_rcv_space_adjust()), READ_ONCE(msk->ack_seq) -
   READ_ONCE(msk->copied_seq) subtracts the pre-IASN msk->copied_seq from the
   post-IASN msk->ack_seq, returning INT_MAX and corrupting receive buffer
   autotuning.

[1] https://lore.kernel.org/r/9a98772b14e535454de5bafbf113aa72a3ee6f44.1793326853.git.tanggeliang@kylinos.cn
[2] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn

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