Re: [PATCH mptcp-next v20 2/6] mptcp: drop the cant_coalesce CB field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: mptcp_inq_hint() seq mismatch when MPTCP_SYNC_SEQ is deferred
--

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

mptcp: drop the cant_coalesce CB field

This commit removes the cant_coalesce field from struct mptcp_skb_cb and
synchronizes the passive Fast Open skb sequence numbers with the peer's
initial ACK sequence number once the 3rd ACK arrives. When the socket is
owned by user space at 3rd ACK arrival, synchronization is deferred via
the MPTCP_SYNC_SEQ flag in cb_flags.

> 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 [1], when the 3rd ACK arrives on a passive Fast
Open socket while user space holds the socket lock, subflow_set_remote_key()
immediately updates msk->ack_seq to subflow->iasn while deferring
__mptcp_sync_rcv_sequence() via MPTCP_SYNC_SEQ.

Until MPTCP_SYNC_SEQ is processed, the TFO skb in sk->sk_receive_queue still
carries its dummy sequence mapping (MPTCP_SKB_CB(skb)->map_seq = -skb->len
plus any consumed offset). If mptcp_inq_hint() is called under lock_sock(sk)
during that window, it subtracts the dummy map_seq from the newly updated
msk->ack_seq:

net/mptcp/protocol.c:mptcp_inq_hint() {
	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;
	}
}

This can happen before release_sock(sk) runs mptcp_release_cb() in three
paths:

1. mptcp_recvmsg() -> __mptcp_recvmsg_mskq() -> mptcp_rcv_space_adjust()
   calls mptcp_inq_hint(sk) before mptcp_move_skbs(sk) is invoked, which can
   underflow copied -= mptcp_inq_hint(sk).

2. mptcp_recvmsg() skips mptcp_move_skbs(sk) when msk->backlog_list is empty
   (for example when the 3rd ACK carries no payload) and then calls
   mptcp_inq_hint(sk) for TCP_CM_INQ:

net/mptcp/protocol.c:mptcp_recvmsg() {
		if (!list_empty(&msk->backlog_list) && mptcp_move_skbs(sk))
			continue;
		...
		if (cmsg_flags & MPTCP_CMSG_INQ) {
			unsigned int inq = mptcp_inq_hint(sk);

			put_cmsg(msg, SOL_TCP, TCP_CM_INQ, sizeof(inq), &inq);
		}
}

3. In mptcp_ioctl() for SIOCINQ, if the 3rd ACK arrives in BH after
   mptcp_move_skbs(sk) drops mptcp_data_lock(sk) and before
   mptcp_inq_hint(sk) reads msk->ack_seq:

net/mptcp/protocol.c:mptcp_ioctl() {
		lock_sock(sk);
		if (mptcp_move_skbs(sk))
			mptcp_cleanup_rbuf(msk, 0);
		*karg = mptcp_inq_hint(sk);
		release_sock(sk);
}

Could this cause mptcp_inq_hint() to return INT_MAX (or a bogus value derived
from subflow->iasn) to user space for TCP_CM_INQ and SIOCINQ, and corrupt the
receive buffer space calculation in mptcp_rcv_space_adjust()?

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

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