Re: [PATCH mptcp-next v15 07/11] mptcp: implement peek_len for proto_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: race and TFO byte loss on MPTCP_SYNC_SEQ in mptcp_inq()
- [High] mptcp: mptcp_inq() clamps wrapped u64 underflow to INT_MAX instead of 0
--

commit 4690b9f63adb950374a40e56669d9dcdd8ff2844
Author: Geliang Tang <geliang@kernel.org>

mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq() and mptcp_peek_len() to report the number of
readable bytes at the MPTCP socket level via proto_ops.peek_len for both IPv4
and IPv6 MPTCP stream operations.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7b0005d47bead..ad11a96b83eba 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -4848,6 +4848,43 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
>  	return ret;
>  }
>  
> +static int mptcp_inq(struct sock *sk)
> +{
> +	const struct mptcp_sock *msk = mptcp_sk(sk);
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) {
> +		answ = 0;
> +	} else if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		answ = 0;
> +	} else {
> +		bool sock_done = sock_flag(sk, SOCK_DONE);
> +		u64 hint_val;
> +
> +		/* Ensure we see updated ack_seq and copied_seq after
> +		 * checking MPTCP_SYNC_SEQ flag.
> +		 */
> +		smp_rmb();

[Severity: High]
Could the ordering of the MPTCP_SYNC_SEQ check and the sequence number reads
here race with subflow_set_remote_key() and mptcp_release_cb(), and also hide
queued TFO data?

On a passive MPTCP connection created before the third ACK arrives (such as a
passive TFO socket initialized via mptcp_sk_clone_init() and
mptcp_fastopen_subflow_synack_set_params()), mptcp_sk_clone_init() sets
sk->sk_state to TCP_ESTABLISHED while msk->ack_seq is 0 and msk->copied_seq
is -(u64)skb->len (or 0).

When the third ACK arrives in BH context while a reader holds the socket
lock, subflow_set_remote_key() writes msk->ack_seq before setting
MPTCP_SYNC_SEQ:

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)) {
		/* User space could have already read partially the TFO skb */
		msk->copied_seq += subflow->iasn;
		__mptcp_sync_rcv_sequence(sk);
	} else {
		/* Ensure ack_seq is visible before setting the flag */
		smp_wmb();
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}
    ...
}

And when the flag is cleared in mptcp_release_cb() (or __mptcp_move_skb()),
MPTCP_SYNC_SEQ is cleared before msk->copied_seq is updated:

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

Because mptcp_inq() tests MPTCP_SYNC_SEQ before reading msk->ack_seq and
msk->copied_seq, can it observe MPTCP_SYNC_SEQ == 0 and then read the new
msk->ack_seq (subflow->iasn) alongside the pre-IASN msk->copied_seq, causing
ack_seq - copied_seq to evaluate to subflow->iasn and clamp to INT_MAX?

In addition, when MPTCP_SYNC_SEQ is set on a passive TFO socket, returning 0
unconditionally ignores unread TFO payload already queued in
sk->sk_receive_queue, whereas mptcp_inq_hint() inspects the receive queue:

net/mptcp/protocol.c:mptcp_inq_hint() {
    ...
	if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
		skb = skb_peek(&sk->sk_receive_queue);
		return skb ? skb->len : 0;
	}
    ...
}

> +
> +		hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> +		if (hint_val >= INT_MAX)
> +			hint_val = INT_MAX;
> +
> +		answ = (unsigned int)hint_val;

[Severity: High]
What happens on 32-bit systems when a torn read of msk->ack_seq or
msk->copied_seq causes the unsigned subtraction to wrap below zero?

The commit message notes that on 32-bit systems a torn read can produce a
wrapped subtraction close to 2^64 which is caught by the INT_MAX clamp.
However, since hint_val is an unsigned u64, a wrapped negative difference
(near U64_MAX) satisfies hint_val >= INT_MAX and gets clamped to INT_MAX
rather than 0.

Would clamping a wrapped underflow to INT_MAX cause mptcp_peek_len() to
report 2 GiB of readable bytes when no data is queued?

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