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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Accessing `msk->first` locklessly under `mptcp_data_lock(sk)` without `READ_ONCE()` or RCU protection leads to a TOCTOU double-fetch, resulting in a NULL pointer dereference or Use-After-Free.
- [Critical] The commit intentionally changes the semantics of UAPI fields `mptcpi_write_seq`, `mptcpi_snd_una`, and `mptcpi_rcv_nxt`, breaking userspace tools.
--

commit 7b94bc72d17bfabfa214a5c1eff0721e3dd01eea
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
Subject: mptcp: normalize seq numbers reported in mptcp_info

This commit normalizes the data sequence numbers reported via mptcp_info
by subtracting the initial sequence numbers. It avoids caching the initial
numbers on the socket by reading them from the subflow context under the
data lock.

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

> Note: this changes the semantics of mptcpi_write_seq, mptcpi_snd_una
> and mptcpi_rcv_nxt. They no longer report the raw on-the-wire MPTCP
> data sequence numbers, but values relative to the connection's
> initial sequence numbers. This is intentional. Tools that correlate
> these fields directly against captured DSS sequence numbers (e.g.
> via tcpdump) will need to account for the offset;

[Severity: Critical]
Does changing these UAPI fields from absolute on-the-wire sequence numbers
to relative offsets break the ABI for existing userspace tools?

The commit message notes that tools correlating these fields against captured
DSS sequence numbers will need to update how they account for the offset.
Will this break backwards compatibility for existing deployed userspace
applications that expect the previous behavior?

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index fcf6feb2a9eb5..1e0a5e06bb8a1 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
[ ... ]
> @@ -1008,9 +1009,19 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	info->mptcpi_flags = flags;
>  
>  	slow = lock_sock_fast(sk);
[ ... ]
>  	info->mptcpi_last_data_recv = jiffies_to_msecs(now - msk->last_data_recv);
>  	unlock_sock_fast(sk, slow);
>  
>  	mptcp_data_lock(sk);
> +	if (msk->first) {
> +		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);

[Severity: Critical]
Can this sequence cause a NULL pointer dereference due to a race with
__mptcp_close_ssk()?

The commit message states that the transition of msk->first to NULL cannot be
observed mid-flight because mptcp_close() holds the same socket lock that
this function acquires via lock_sock_fast().

However, unlock_sock_fast() releases that lock right before acquiring the
spinlock mptcp_data_lock().

If a concurrent thread running mptcp_close() proceeds to __mptcp_close_ssk()
and executes WRITE_ONCE(msk->first, NULL) without holding mptcp_data_lock():

net/mptcp/protocol.c:__mptcp_close_ssk() {
    ...
	if (ssk == msk->first)
		WRITE_ONCE(msk->first, NULL);
    ...
}

Could msk->first be modified to NULL between the if check and the dereference
since READ_ONCE() is not used here under the spinlock?

If the first fetch is non-NULL and the second fetch is NULL,
mptcp_subflow_ctx() would compute a negative offset, leading to a crash when
accessing subflow->iasn.

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