Re: [PATCH mptcp-next v15 04/11] mptcp: sync mptcp skb cb layout with tcp one

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] mptcp: subtracting offset in mptcp_init_skb() corrupts OoO and coalescing
- [High] mptcp: double offset subtraction in mptcp_init_skb() on TCP fallback
- [High] mptcp: unlocked cb_flags and sequence sync in __mptcp_move_skb()
- [Medium] mptcp: race and TFO byte overcount in mptcp_inq_hint()
- [Medium] mptcp: missing rcvd_dummy_seq reset in mptcp_disconnect()
--

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

mptcp: sync mptcp skb cb layout with tcp one

This series removes the cant_coalesce and offset fields from struct
mptcp_skb_cb, tracks consumed receive bytes via msk->copied_seq, and
aligns the 32-bit sequence number and flags layout of struct mptcp_skb_cb
with struct tcp_skb_cb while keeping the 64-bit sequence number in
map_seq64.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..01eb7fb314e76 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -167,13 +167,14 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
>  {
>  	int limit = READ_ONCE(sk->sk_rcvbuf);
>  
> -	if (unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
> -	    MPTCP_SKB_CB(from)->offset ||
> +	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
> +	    MPTCP_SKB_CB(from)->map_seq64 != MPTCP_SKB_CB(to)->map_seq64 +
> +					     to->len ||
>  	    ((to->len + from->len) > (limit >> 3)) ||
>  	    !skb_try_coalesce(to, from, fragstolen, delta))
>  		return false;

[ ... ]

> @@ -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;

[Severity: Critical]
When __mptcp_move_skbs_from_subflow() passes a non-zero offset to
mptcp_init_skb() (for example, after mptcp_subflow_discard_data() advances
tp->copied_seq past an already-received mapping in the prefix of a
multi-mapping skb, or when a partial-overlap subflow segment has
TCP_SKB_CB(skb)->seq < tp->copied_seq), the valid mapped payload starts at
skb->data + offset with data sequence number dsn =
mptcp_subflow_get_mapped_dsn(subflow), while skb->data[0 .. offset - 1]
contains discarded or unrelated subflow bytes.

By shifting map_seq64 and map_seq backwards to dsn - offset and removing
the offset check from __mptcp_try_coalesce(), what prevents the following
regressions when offset > 0?

1. If a preceding skb in backlog_list, out_of_order_queue, or
sk_receive_queue ends at dsn - offset, __mptcp_try_coalesce() sees
MPTCP_SKB_CB(from)->map_seq == MPTCP_SKB_CB(to)->end_seq and
MPTCP_SKB_CB(from)->map_seq64 == MPTCP_SKB_CB(to)->map_seq64 + to->len, and
skb_try_coalesce() appends the entire from buffer (including the leading
bytes in from->data[0 .. offset - 1]) onto to.

2. If the mapping at skb->data + offset is out-of-order (dsn >
msk->ack_seq) with dsn - offset <= msk->ack_seq, __mptcp_move_skb() sees
MPTCP_SKB_CB(skb)->map_seq64 <= msk->ack_seq and treats the out-of-order
skb as in-sequence or a partial packet, queuing it to sk->sk_receive_queue
and advancing msk->ack_seq so __mptcp_recvmsg_mskq() copies stale prefix
bytes to user space in place of [msk->ack_seq, dsn).

3. If dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the skb
into msk->out_of_order_queue at map_seq = (u32)(dsn - offset), where it can
drop or overwrite legitimate out-of-order segments in [dsn - offset, dsn)
as MPTCP_MIB_DUPDATA, and __mptcp_ofo_queue() will dequeue it as soon as
msk->ack_seq reaches dsn - offset.

[Severity: High]
Also in mptcp_init_skb(), doesn't subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) subtract offset twice when the
connection is in fallback mode?

In net/mptcp/subflow.c:subflow_check_data_avail():

	skb = skb_peek(&ssk->sk_receive_queue);
	subflow->map_valid = 1;
	subflow->map_data_len = skb->len;
	subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
					      subflow->iasn +
					      TCP_SKB_CB(skb)->seq -
					      subflow->ssn_offset - 1);

Because subflow->map_subflow_seq is based on tcp_sk(ssk)->copied_seq,
mptcp_subflow_get_map_offset(subflow) is 0, while subflow->map_seq is set
from TCP_SKB_CB(skb)->seq (which is tcp_sk(ssk)->copied_seq - offset).
Thus mptcp_subflow_get_mapped_dsn(subflow) is already msk->ack_seq -
offset.

When mptcp_init_skb() subtracts offset again, map_seq64 becomes
msk->ack_seq - 2 * offset and end_seq becomes (u32)msk->ack_seq + skb->len
- 2 * offset. In __mptcp_move_skb(), won't this drop the skb as
MPTCP_MIB_DUPDATA when skb->len <= 2 * offset, or compute copy_len =
skb->len - 2 * offset and skip 2 * offset bytes in __mptcp_recvmsg_mskq()
while leaving msk->ack_seq behind tp->copied_seq?

>  	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,47 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	skb_dst_drop(skb);
>  }
>  
> +/* Caller must hold the msk data lock */
> +void __mptcp_sync_rcv_sequence(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sk_buff *skb;
> +
> +	if (likely(!msk->rcvd_dummy_seq))
> +		return;
> +
> +	/* User space can have already received the TFO skb. */
> +	msk->rcvd_dummy_seq = false;
> +	skb = skb_peek_tail(&sk->sk_receive_queue);
> +	if (!skb)
> +		return;
> +
> +	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_iasn(msk) - skb->len;
> +	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;
> +}
> +
>  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) {
> +	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		/* Ensure we see the updated ack_seq after seeing the flag */
> +		smp_rmb();
> +		msk->copied_seq += mptcp_iasn(msk);
> +
> +		/* 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);
> +	}

[Severity: High]
Is it safe for __mptcp_move_skb() to run test_and_clear_bit(MPTCP_SYNC_SEQ,
&msk->cb_flags) and call __mptcp_sync_rcv_sequence(sk) without holding
mptcp_data_lock(sk)?

When __mptcp_move_skb() is called from mptcp_move_skbs() or
mptcp_release_cb() -> __mptcp_move_skbs(), sk->sk_lock.slock has been
dropped while sock_owned_by_user(sk) is true:

net/mptcp/protocol.c:mptcp_move_skbs() {
	mptcp_data_lock(sk);
	while (mptcp_can_spool_backlog(sk, &skbs)) {
		mptcp_data_unlock(sk);
		enqueued |= __mptcp_move_skbs(sk, &skbs, &moved);

		mptcp_data_lock(sk);
	}
}

At the same time, other paths holding mptcp_data_lock(sk) while
sock_owned_by_user(sk) is true update msk->cb_flags using non-atomic
__set_bit() or |= operations, such as __mptcp_data_acked(),
__mptcp_check_push(), and mptcp_propagate_state():

net/mptcp/protocol.c:__mptcp_data_acked() {
	if (!sock_owned_by_user(sk))
		__mptcp_clean_una(sk);
	else
		__set_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->cb_flags);
}

If a concurrent softirq path loads msk->cb_flags (while MPTCP_SYNC_SEQ is
set) for a non-atomic __set_bit() and stores the word back after
test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) in __mptcp_move_skb()
clears it, won't MPTCP_SYNC_SEQ be re-set and cause a subsequent
__mptcp_move_skb() or mptcp_release_cb() call to add mptcp_iasn(msk) to
msk->copied_seq a second time? Calling __mptcp_sync_rcv_sequence(sk) here
without mptcp_data_lock(sk) also violates the lock requirement noted above
__mptcp_sync_rcv_sequence().

[ ... ]

> @@ -2403,21 +2411,22 @@ 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;
> -
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> -
> -		return (unsigned int)hint_val;
> +	if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		skb = skb_peek(&sk->sk_receive_queue);
> +		return skb ? skb->len : 0;
>  	}
>  
> -	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> +	hint_val = READ_ONCE(msk->ack_seq) - msk->copied_seq;

[Severity: Medium]
Can mptcp_inq_hint() return an inflated byte count in two cases here?

First, in subflow_set_remote_key() when sock_owned_by_user(sk) is true,
msk->ack_seq is written before MPTCP_SYNC_SEQ is set:

net/mptcp/subflow.c:subflow_set_remote_key() {
	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)) {
		...
	} else {
		/* Ensure ack_seq is visible before setting the flag */
		smp_wmb();
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}
}

Because mptcp_inq_hint() tests MPTCP_SYNC_SEQ first and then reads
READ_ONCE(msk->ack_seq) without holding mptcp_data_lock(sk), it can observe
MPTCP_SYNC_SEQ == 0 and then read the updated msk->ack_seq == subflow->iasn
before msk->copied_seq has been synced, causing READ_ONCE(msk->ack_seq) -
msk->copied_seq to wrap or clamp to INT_MAX.

Second, when test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) is true and user
space has already partially read from the fastopen skb (advancing
msk->copied_seq), returning skb->len does not subtract the already-copied
offset ((u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq).

[ ... ]

> @@ -3689,11 +3711,13 @@ static int mptcp_disconnect(struct sock *sk, int flags)
>  	msk->bytes_retrans = 0;
>  	msk->rcvspace_init = 0;
>  	msk->fastclosing = 0;
> +	msk->tfo_skb_len = 0;
>  	mptcp_init_rtt_est(msk);
>  
>  	/* for fallback's sake */
>  	WRITE_ONCE(msk->ack_seq, 0);
>  	atomic64_set(&msk->rcv_wnd_sent, 0);
> +	msk->copied_seq = 0;

[Severity: Medium]
Should mptcp_disconnect() also reset msk->rcvd_dummy_seq = false (and clear
MPTCP_SYNC_SEQ)?

If a passive fastopen socket sets msk->rcvd_dummy_seq = true in
mptcp_fastopen_subflow_synack_set_params() and is then disconnected via
mptcp_disconnect() before __mptcp_sync_rcv_sequence() runs,
msk->rcvd_dummy_seq remains true and a subsequent connection on the same
socket can invoke __mptcp_sync_rcv_sequence() and overwrite the tail skb's
sequence numbers in sk->sk_receive_queue.

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