Re: [PATCH mptcp-next v7 4/8] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Spurious INT_MAX returned by SIOCINQ and CMSG_INQ due to deferred `copied_seq` synchronization window, causing potential application OOMs.
--

--- Patch [4]: [PATCH mptcp-next v7 4/8] mptcp: remove CB offset field ---
commit 9bec5b4a8e5f51b288f8330e08515f372476e6e8
Author: Paolo Abeni <pabeni@redhat.com>
Subject: mptcp: remove CB offset field

This patch replaces the offset field in mptcp_skb_cb with a socket-level
copied_seq field. This tracks bytes consumed and delegates synchronization
to the release callback when userspace holds the socket lock during remote
key reception.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c23046b1e51b5..a9faae858a21c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -2391,26 +2391,23 @@ static unsigned int mptcp_inq_hint(struct sock *sk)
>  {
>  	const struct mptcp_sock *msk = mptcp_sk(sk);
>  	u64 hint_val;
>  
> -	skb = skb_peek(&sk->sk_receive_queue);
> -	if (skb) {
> -		u64 hint_val = atomic64_read(&msk->ack_seq) -
> -			       MPTCP_SKB_CB(skb)->map_seq;
> -
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> -
> -		return (unsigned int)hint_val;
> -	}
> +	/* Avoid races vs ack_seq updates. */
> +	mptcp_data_lock(sk);
> +	hint_val = atomic64_read(&msk->ack_seq) - msk->copied_seq;
> +	mptcp_data_unlock(sk);
> +	if (hint_val >= INT_MAX)
> +		return INT_MAX;

[Severity: High]
Can mptcp_inq_hint() return a spurious INT_MAX here when userspace calls
SIOCINQ during connection establishment?

If the user application concurrently holds the socket lock while polling,
subflow_set_remote_key() will immediately update msk->ack_seq to the
cryptographic sequence number (iasn), but defer the msk->copied_seq
synchronization by setting MPTCP_SYNC_SEQ:

net/mptcp/subflow.c:subflow_set_remote_key() {
    ...
	atomic64_set(&msk->ack_seq, subflow->iasn);
    ...
	if (!sock_owned_by_user(sk)) {
    ...
	} else {
		/* Ensure ack_seq is visible before setting the flag */
		smp_wmb();
		set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
	}
}

Because mptcp_inq_hint() unconditionally reads the updated ack_seq and the
not-yet-synchronized copied_seq, their difference will be a huge value
causing this to return INT_MAX. Is there a way to force synchronization
or check MPTCP_SYNC_SEQ before computing this hint? Applications often use
this value to dynamically allocate buffers, which could lead to memory
exhaustion and crashes.

>  
> -	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> +	if (!hint_val &&
> +	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
>  		return 1;
>  
> -	return 0;
> +	return (unsigned int)hint_val;
>  }

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