Re: [PATCH mptcp-next v19 2/5] mptcp: drop the cant_coalesce CB field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mptcp: unlocked MPTCP_SYNC_SEQ bitop on cb_flags in mptcp_move_skbs()
- [Medium] mptcp: mptcp_inq_hint() seq mismatch when MPTCP_SYNC_SEQ is deferred
- [Medium] mptcp: unreset tfo_iasn in mptcp_disconnect() corrupts passive TFO fallback
--

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

mptcp: drop the cant_coalesce CB field

This commit removes the cant_coalesce skb control buffer field and instead
synchronizes the passive Fast Open skb sequence numbers with msk->tfo_iasn
via __mptcp_sync_rcv_sequence() when the 3rd ACK arrives or before the next
segment moves into the msk receive queue.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 528a72da646cc..e855aaa59da29 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -438,6 +436,24 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	skb_dst_drop(skb);
>  }
>  
> +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_seq = msk->tfo_iasn - skb->len;
> +	MPTCP_SKB_CB(skb)->end_seq = msk->tfo_iasn;
> +}

[ ... ]

> @@ -2374,6 +2396,9 @@ 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))
> +		__mptcp_sync_rcv_sequence(sk);
> +
>  	mptcp_data_lock(sk);
>  	while (mptcp_can_spool_backlog(sk, &skbs)) {
>  		mptcp_data_unlock(sk);

[Severity: Medium]
As reported by Sashiko on v15 [1], can calling test_and_clear_bit() on
msk->cb_flags and __mptcp_sync_rcv_sequence() here in mptcp_move_skbs()
without holding mptcp_data_lock(sk) race with concurrent non-atomic updates
to msk->cb_flags in BH or timer contexts?

While user space holds lock_sock(sk) and runs mptcp_move_skbs() (from
mptcp_recvmsg(), mptcp_ioctl(), or mptcp_recv_skb()), the 3rd ACK can arrive
in softirq on another CPU:

  check_fully_established()
    mptcp_propagate_state()
      mptcp_data_lock(sk);
      subflow_set_remote_key()
        msk->tfo_iasn = subflow->iasn;
        set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
      __set_bit(MPTCP_SYNC_STATE, &msk->cb_flags);
      mptcp_data_unlock(sk);

Other BH and timer paths (__mptcp_data_acked(), __mptcp_check_push(),
mptcp_retransmit_timer(), mptcp_finish_join(), and subflow_error_report())
also modify msk->cb_flags with non-atomic __set_bit() under
mptcp_data_lock(sk). If a BH handler loads msk->cb_flags while
MPTCP_SYNC_SEQ is set and mptcp_move_skbs() executes unlocked
test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) before the BH CPU stores
the modified word back, the store can resurrect MPTCP_SYNC_SEQ. That causes
MPTCP_SYNC_SEQ to be processed a second time in mptcp_release_cb(), which in
the subsequent patch ("mptcp: remove CB offset field") adds msk->tfo_iasn to
msk->copied_seq twice.

In addition, because set_bit() is a relaxed atomic operation without release
semantics and mptcp_move_skbs() does not hold mptcp_data_lock(sk) here,
could __mptcp_sync_rcv_sequence() observe a stale msk->tfo_iasn on weakly
ordered architectures?

Finally, the commit message states that mptcp_move_skbs() checks
MPTCP_SYNC_SEQ at each iteration before spooling the backlog, but the check
is placed before mptcp_data_lock(sk) outside the while
(mptcp_can_spool_backlog(sk, &skbs)) loop. Should the MPTCP_SYNC_SEQ check
be moved inside the loop under mptcp_data_lock(sk) (using
__test_and_clear_bit() across all msk->cb_flags accessors)?

> @@ -3680,6 +3705,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
>  	msk->bytes_retrans = 0;
>  	msk->rcvspace_init = 0;
>  	msk->fastclosing = 0;
> +	msk->rcvd_dummy_seq = false;
>  	mptcp_init_rtt_est(msk);
>  
>  	/* for fallback's sake */

[Severity: Medium]
Does mptcp_disconnect() also need to reset msk->tfo_iasn = 0 (or initialize
it in __mptcp_init_sock())?

If a client MPTCP socket connects and receives an MP_CAPABLE SYN-ACK,
subflow_set_remote_key() sets msk->tfo_iasn to subflow->iasn. If the socket
then disconnects via mptcp_disconnect() and is reused as a listener via
mptcp_listen(), child sockets created by mptcp_sk_clone_init() inherit the
non-zero msk->tfo_iasn via sk_clone_lock().

If a passive TFO connection on that listener receives a SYN with MP_CAPABLE
and data (setting msk->rcvd_dummy_seq = true in
mptcp_fastopen_subflow_synack_set_params()) and then falls back to TCP on
the 3rd ACK in check_fully_established() without calling
subflow_set_remote_key(), the next incoming segment in __mptcp_move_skb()
calls __mptcp_sync_rcv_sequence(sk). Because msk->tfo_iasn still holds the
previous connection's IASN instead of 0, __mptcp_sync_rcv_sequence() will
overwrite the queued TFO skb's map_seq and end_seq with the stale
msk->tfo_iasn, breaking skb coalescing and mptcp_inq_hint() (and causing
__mptcp_recvmsg_mskq() and mptcp_recv_skb() to skip the TFO payload after
the subsequent commit removing the CB offset field).

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..8a3ba490b5772 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,12 @@ 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);
> +	msk->tfo_iasn = subflow->iasn;
> +
> +	if (!sock_owned_by_user(sk))
> +		__mptcp_sync_rcv_sequence(sk);
> +	else
> +		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
>  }

[Severity: Medium]
As reported by Sashiko on v15 [2], when the 3rd ACK arrives in BH context
while user space holds lock_sock(sk), subflow_set_remote_key() updates
msk->ack_seq to subflow->iasn immediately while deferring
__mptcp_sync_rcv_sequence() via MPTCP_SYNC_SEQ. Can this cause
mptcp_inq_hint() to compute a bogus value before MPTCP_SYNC_SEQ is
processed?

In net/mptcp/protocol.c, mptcp_inq_hint() computes:

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

While MPTCP_SYNC_SEQ is still pending, MPTCP_SKB_CB(skb)->map_seq on the
queued TFO skb is still the dummy value (-skb->len), whereas msk->ack_seq
 has already been updated to subflow->iasn (and in the later commit
"mptcp: remove CB offset field", msk->copied_seq remains -(u64)skb->len
until MPTCP_SYNC_SEQ is processed).

This state is reachable under lock_sock(sk) in three places before
release_sock(sk) runs mptcp_release_cb():

1. mptcp_recvmsg() -> __mptcp_recvmsg_mskq() -> mptcp_rcv_space_adjust()
   calls mptcp_inq_hint(sk) before mptcp_move_skbs(sk), underflowing
   copied -= mptcp_inq_hint(sk).
2. In mptcp_recvmsg(), mptcp_move_skbs(sk) is only called when
   !list_empty(&msk->backlog_list); if the 3rd ACK carries no data payload,
   backlog_list is empty and the TCP_CM_INQ cmsg path calls
   mptcp_inq_hint(sk) with MPTCP_SYNC_SEQ still pending, reporting INT_MAX
   (or a corrupted value based on subflow->iasn) to user space.
3. In mptcp_ioctl(SIOCINQ), if the 3rd ACK arrives in BH after
   mptcp_move_skbs(sk) checks MPTCP_SYNC_SEQ and before mptcp_inq_hint(sk)
   reads msk->ack_seq, SIOCINQ likewise returns a bogus value.

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

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