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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mptcp: mptcp_inq_hint() seq mismatch when subflow_set_remote_key() defers sync
- [Medium] mptcp: off-by-one in __mptcp_sync_rcv_sequence() after mptcp_check_data_fin()
--

commit f64a32260362fb1c8168b46cd47f03ba4852fb6c
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 sequence numbers of a passive TCP Fast Open skb once the
initial ACK sequence number becomes available on the third ACK.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 528a72da646cc..fe154b1816834 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -438,6 +436,26 @@ 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;
> +	u32 offset;
> +
> +	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;
> +
> +	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;
> +}

[Severity: Medium]
As reported by Sashiko on v15 [1], can __mptcp_sync_rcv_sequence() compute
an off-by-one map_seq and end_seq if mptcp_check_data_fin() runs before
mptcp_release_cb()?

When mptcp_worker() holds lock_sock(sk) on a passive TFO socket with SYN data
queued in sk->sk_receive_queue (msk->rcvd_dummy_seq is true), the peer's third
ACK carrying MP_CAPABLE and a zero-payload DATA_FIN (with data_seq equal to
subflow->iasn and data_len equal to 1) can arrive in BH context.

In that case, subflow_set_remote_key() sets msk->ack_seq to subflow->iasn and
sets MPTCP_SYNC_SEQ in msk->cb_flags, while mptcp_incoming_options() /
get_mapping_status() calls mptcp_update_rcv_data_fin() to record
msk->rcv_data_fin_seq = subflow->iasn and msk->rcv_data_fin = 1 without
queuing an skb to msk->backlog_list.

Before release_sock(sk) invokes mptcp_release_cb() in net/mptcp/protocol.c,
mptcp_worker() calls mptcp_check_data_fin():

net/mptcp/protocol.c:mptcp_worker()
	mptcp_check_send_data_fin(sk);
	mptcp_check_data_fin_ack(sk);
	mptcp_check_data_fin(sk);

which sees msk->ack_seq == rcv_data_fin_seq and advances msk->ack_seq by 1:

net/mptcp/protocol.c:mptcp_check_data_fin()
	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
		WRITE_ONCE(msk->rcv_data_fin, 0);

When release_sock(sk) subsequently runs mptcp_release_cb() ->
__mptcp_sync_rcv_sequence(sk), msk->ack_seq is already subflow->iasn + 1, so
both MPTCP_SKB_CB(skb)->map_seq and MPTCP_SKB_CB(skb)->end_seq on the TFO skb
will be shifted forward by 1 byte.

[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..49a000b8dcb8e 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,11 @@ 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))
> +		__mptcp_sync_rcv_sequence(sk);
> +	else
> +		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
>  }

[Severity: Medium]
As reported by Sashiko on v15 [2], does deferring __mptcp_sync_rcv_sequence()
here while immediately updating msk->ack_seq cause mptcp_inq_hint() to return
a bogus value when sock_owned_by_user(sk) is true?

In mptcp_fastopen_subflow_synack_set_params() in net/mptcp/fastopen.c, the TFO
skb is queued onto sk->sk_receive_queue with MPTCP_SKB_CB(skb)->map_seq set to
-skb->len and end_seq set to 0 while msk->ack_seq is 0.

If user space holds lock_sock(sk) in mptcp_ioctl() (for SIOCINQ) or
mptcp_recvmsg() when the peer's third ACK arrives in BH context,
subflow_set_remote_key() updates msk->ack_seq to subflow->iasn immediately,
but defers __mptcp_sync_rcv_sequence(sk) via MPTCP_SYNC_SEQ until
release_sock(sk) runs mptcp_release_cb().

When the third ACK carries no payload (or arrives after mptcp_move_skbs() has
checked msk->backlog_list), __mptcp_move_skb() is not called before
mptcp_inq_hint(sk) runs under lock_sock(sk):

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

and similarly in mptcp_recvmsg() (for TCP_CM_INQ) and
__mptcp_recvmsg_mskq() -> mptcp_rcv_space_adjust().

In mptcp_inq_hint() in net/mptcp/protocol.c:

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

hint_val subtracts the unsynchronized dummy map_seq (-skb->len + offset) from
subflow->iasn, evaluating to subflow->iasn + skb->len - offset and returning
INT_MAX (or a corrupted value) instead of the unread TFO payload length.

[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.1793349369.git.geliang@kernel.org?part=2