Re: [PATCH mptcp-next v9 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] Lockless read of 64-bit `copied_seq` causes torn reads on 32-bit systems and concurrent update races, leading to spurious INT_MAX return values.
- [High] Subtracting 1 for a FIN flag breaks EOF detection for stream consumers by reporting 0 instead of 1.
--

commit 7df02914f0a616d203067af77b84f00d4f6bc119
Author: Geliang Tang <geliang@kernel.org>
mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq() to compute the readable bytes at the MPTCP
level and derives the count from mptcp_inq_hint(). It wires mptcp_inq()
into mptcp_peek_len() and assigns .peek_len in the stream operations
so upper layers get the in-queue byte count.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index cc8747162cca9..9b9ace7cb977d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -4814,6 +4814,37 @@ 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 {
> +		u64 hint_val;
> +
> +		hint_val = atomic64_read(&msk->ack_seq) - msk->copied_seq;
> +		if (hint_val >= INT_MAX)
> +			hint_val = INT_MAX;

[Severity: High]
Could this unprotected 64-bit read of copied_seq cause torn reads on 32-bit
systems?

If a concurrent caller invokes poll or a vhost loop calls .peek_len while
another thread is calling recvmsg(), the unsynchronized read of ack_seq and
copied_seq can result in an inconsistent state and cause an underflow.

This would then trigger the INT_MAX clamp, leading .peek_len to incorrectly
report a massive amount of queued data (up to 2GB). Is there a risk that this
causes stream consumers to allocate excessive memory or loop endlessly
expecting data?

> +
> +		answ = (unsigned int)hint_val;
> +		if (answ &&
> +		    (sk->sk_state == TCP_CLOSE ||
> +		     (sk->sk_shutdown & RCV_SHUTDOWN)))
> +			answ--;

[Severity: High]
Does subtracting 1 here break EOF detection for stream consumers?

When the peer sends a FIN and the local MPTCP stack sets RCV_SHUTDOWN, this
logic actively suppresses the 1-byte EOF signal by reporting 0 instead of 1.

Callers relying on .peek_len for stream parsing (like strparser or KCM) might
enter an infinite wait or hang because they will see 0 data available and
wait for more data that will never arrive, instead of issuing the final
recvmsg() to observe EOF.

[ ... ]

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