Re: [PATCH net-next v4] mptcp: normalize seq numbers reported in mptcp_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Changing the semantics of UAPI fields `mptcpi_write_seq`, `mptcpi_snd_una`, and `mptcpi_rcv_nxt` from absolute MPTCP sequence numbers to relative zero-based offsets breaks userspace tools.
- [Medium] Unsynchronized access to the 64-bit `subflow->iasn` and bitfield `subflow->remote_key_valid` causes a data race, torn reads, and memory reordering bugs.
- [Medium] A cross-lock Time-of-Check to Time-of-Use (TOCTOU) race between reading `remote_key_valid` and `msk->ack_seq` causes the diagnostic tool to leak the raw, unnormalized sequence number.
--

commit b74b908d7d9e7114635fdc56f70e62e97e495292
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

mptcp: normalize seq numbers reported in mptcp_info

This commit attempts to normalize sequence numbers reported in mptcp_info by
subtracting the initial sequence numbers from the raw values. It calculates
these offsets using the local and remote IDSN from the subflow context.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/445

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index fcf6feb2a9eb5..5bcad54e909d1 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
[ ... ]
> @@ -1008,9 +1009,29 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	info->mptcpi_flags = flags;
>  
>  	slow = lock_sock_fast(sk);
> +	/* msk->first is only ever NULL once the whole msk is already in
> +	 * TCP_CLOSE (see __mptcp_close_ssk()); mptcp_close() holds the same
> +	 * sk lock this function acquires via lock_sock_fast(), so no caller
> +	 * can observe that transition mid-flight. If it does happen, the 0
> +	 * fallback below just leaves write_seq/snd_una/rcv_nxt unnormalized,
> +	 * which is harmless since the socket is already gone.
> +	 */
> +
> +	if (msk->first) {
> +		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
> +
> +		local_idsn = subflow->idsn;
> +		/* subflow->iasn is incremented once in subflow_set_remote_key()
> +		 * to account for the peer's virtual SYN; undo that here so
> +		 * rcv_nxt normalizes against the same baseline write_seq and
> +		 * snd_una use.
> +		 */
> +		remote_idsn = subflow->remote_key_valid ? subflow->iasn - 1 : 0;

[Severity: Medium]
Could this result in a data race? In mptcp_diag_fill_info(), we are only
holding the socket lock via lock_sock_fast(). However, the writer in
subflow_set_remote_key() holds only the data lock.

On 32-bit systems, a lockless read of the 64-bit subflow->iasn can tear.
Additionally, since there are no memory barriers (e.g. smp_load_acquire() and
smp_store_release()), the CPU could reorder the reads and observe
remote_key_valid == 1 while iasn is still uninitialized.

Could this happen when getsockopt(MPTCP_INFO) is called concurrently with the
softirq processing the remote key via mptcp_propagate_state()?

> +	}
> +
>  	info->mptcpi_csum_enabled = READ_ONCE(msk->csum_enabled);
>  	info->mptcpi_token = msk->token;
> -	info->mptcpi_write_seq = msk->write_seq;
> +	info->mptcpi_write_seq = msk->write_seq - local_idsn;

[Severity: Critical]
Does this regression break userspace ABI? 

By changing the semantics of mptcpi_write_seq, mptcpi_snd_una, and
mptcpi_rcv_nxt from absolute sequence numbers to relative zero-based offsets,
existing tools relying on absolute sequence numbers (like tcpdump correlation)
might break.

Furthermore, because the kernel does not expose the local_idsn and remote_idsn
offsets in the mptcp_info struct, userspace has no way to reconstruct the
original absolute values.

>  	info->mptcpi_retransmits = inet_csk(sk)->icsk_retransmits;
>  	info->mptcpi_bytes_sent = msk->bytes_sent;
>  	info->mptcpi_bytes_received = msk->bytes_received;
[ ... ]
> @@ -1024,8 +1045,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  
>  	mptcp_data_lock(sk);
>  	info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk->last_ack_recv);
> -	info->mptcpi_snd_una = msk->snd_una;
> -	info->mptcpi_rcv_nxt = msk->ack_seq;
> +	info->mptcpi_snd_una = msk->snd_una - local_idsn;
> +	info->mptcpi_rcv_nxt = msk->ack_seq - remote_idsn;

[Severity: Medium]
Could this regression leak the raw sequence number due to a Time-of-Check to
Time-of-Use race?

If mptcp_diag_fill_info() samples remote_key_valid as 0 earlier under
lock_sock_fast(), remote_idsn will be captured as 0. If
subflow_set_remote_key() then executes concurrently before we acquire the
data lock here, it updates msk->ack_seq to the absolute sequence number.

When we compute info->mptcpi_rcv_nxt = msk->ack_seq - remote_idsn here, it
will subtract 0 from the raw sequence number, returning the unnormalized
value and causing a discontinuity for userspace.

>  	info->mptcpi_bytes_acked = msk->bytes_acked;
>  	mptcp_data_unlock(sk);
>  }
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index af81ad5e699d1..794c6dd9e4150 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
[ ... ]
> @@ -493,6 +492,9 @@ 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);
> +
> +	/* publish last, once iasn and the fields above are fully populated */
> +	subflow->remote_key_valid = 1;

[Severity: Medium]
As mentioned earlier in mptcp_diag_fill_info(), writing to this bitfield
under the data lock without memory barriers means that concurrent lockless
readers might observe remote_key_valid == 1 while iasn is still
uninitialized.

>  }
>  
>  static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903054853.3400545-1-kalpan.jani@mpiricsoftware.com?part=1