Re: [PATCH mptcp-next v9 04/11] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Lockless check of MPTCP_SYNC_SEQ in mptcp_inq_hint() leads to a TOCTOU race condition that can incorrectly return a bogus TCP_CM_INQ value (often INT_MAX).
--

commit 6c708b896ef73852cc85c2a1debeca40b8a72f1c
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit replaces the offset field in MPTCP's skb control block with a
socket-level copied_seq field. This mirrors TCP's approach for tracking
bytes consumed and delegating IASN synchronization to the msk release CB
during Fastopen.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7d1422518aada..4cf7ad97ae0af 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -2414,27 +2397,27 @@ static bool mptcp_move_skbs(struct sock *sk)
>  	return enqueued;
>  }
>  
> -static unsigned int mptcp_inq_hint(const struct sock *sk)
> +static unsigned int mptcp_inq_hint(struct sock *sk)
>  {
>  	const struct mptcp_sock *msk = mptcp_sk(sk);
> -	const struct sk_buff *skb;
>  	u64 hint_val, ack_seq;
>  
> -	skb = skb_peek(&sk->sk_receive_queue);
> -	if (skb) {
> -		ack_seq = atomic64_read(&msk->ack_seq);
> -		hint_val = ack_seq - MPTCP_SKB_CB(skb)->map_seq;
> -
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> +	if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
> +		return 0;
>  
> -		return (unsigned int)hint_val;
> -	}
> +	/* Avoid races vs ack_seq updates. */
> +	mptcp_data_lock(sk);
> +	ack_seq = atomic64_read(&msk->ack_seq);
> +	hint_val = ack_seq - msk->copied_seq;
> +	mptcp_data_unlock(sk);

[Severity: Medium]
Could this lockless check of MPTCP_SYNC_SEQ lead to a Time-of-Check to
Time-of-Use (TOCTOU) race condition?

If an application calls recvmsg() with msg_control to read TCP_CM_INQ, and
concurrent to mptcp_inq_hint() executing, a softirq updates the socket's
ack_seq to the remote key (iasn) and sets the MPTCP_SYNC_SEQ flag:

mptcp_inq_hint() could test MPTCP_SYNC_SEQ outside of mptcp_data_lock(sk),
read 0 (false), wait for the lock, and then calculate hint_val using the
newly updated ack_seq but a stale copied_seq.

Since the new ack_seq is an absolute cryptographic sequence number and the
stale copied_seq is relative (negative), the difference could be larger
than INT_MAX, causing the function to incorrectly return INT_MAX.

> +	if (hint_val >= INT_MAX)
> +		return INT_MAX;
>  
> -	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.1787827525.git.tanggeliang@kylinos.cn?part=4