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

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

[Kalpan Jani]

mptcpi_write_seq, mptcpi_snd_una and mptcpi_rcv_nxt report the raw
64-bit data sequence numbers, seeded from the connection's IDSN/IASN.
Since the IDSN/IASN come from mptcp_crypto_key_sha(), these fields
carry an effectively random offset and are not useful to userspace as
absolute values: a caller has to snapshot two getsockopt(MPTCP_INFO)
calls and subtract to get anything meaningful, which is exactly what
tools/testing/selftests/net/mptcp/mptcp_sockopt.c already does.

mptcp_info also reports mptcpi_bytes_sent, mptcpi_bytes_received and
mptcpi_bytes_acked, which give the same information as a plain byte
count starting at 0. The snapshot-and-diff workaround for the seq
fields is redundant once those are available.

Normalize mptcpi_write_seq, mptcpi_snd_una and mptcpi_rcv_nxt in
mptcp_diag_fill_info() by subtracting the local and remote initial
sequence numbers, read from msk->first's subflow context rather than
caching them on mptcp_sock, to avoid growing every mptcp_sock for a
diag-only need.

msk->first only changes when the whole msk is already in TCP_CLOSE,
confirmed by testing with a debug print in __mptcp_close_ssk() under
mptcp_join.sh: every NULL transition observed had msk_state ==
TCP_CLOSE, none while the connection was still established.
mptcp_close() holds the same sk lock this function acquires via
lock_sock_fast(), so no caller of MPTCP_INFO (direct getsockopt,
MPTCP_FULL_INFO, or the netlink diag path in mptcp_diag.c) can
observe that transition mid-flight; if it somehow did, write_seq and
snd_una are just left unnormalized on an already-gone socket.

subflow->idsn is set once at handshake time and never modified
afterwards, so local_idsn can be read directly under the socket lock
alongside write_seq and snd_una, which are themselves socket-lock
protected.

subflow->iasn is different: it's written by subflow_set_remote_key(),
called from mptcp_propagate_state() under mptcp_data_lock(), not the
socket lock. remote_idsn is therefore computed in this function's
existing mptcp_data_lock() section instead, right next to where
ack_seq is read: this puts the read under the same lock the writer
uses, and samples remote_idsn and ack_seq atomically together, so a
concurrent subflow_set_remote_key() can't update one without the
other being visible here too. subflow->iasn is incremented by one in
that function to account for the peer's virtual SYN, and ack_seq
carries that same increment, so the increment is undone here
(iasn - 1) to keep rcv_nxt normalized against the same baseline
write_seq and snd_una use.

subflow_set_remote_key() also sets subflow->remote_key_valid only
after subflow->iasn and the other fields it guards are fully
populated, instead of first, so it can't be observed true while iasn
is still stale even by a reader sharing its lock.

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; mptcpi_bytes_sent,
mptcpi_bytes_received and mptcpi_bytes_acked remain unaffected and
are the recommended fields for tracking absolute transferred-byte
counts.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/445
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

Changes since v4:
- The v4 reorder of remote_key_valid alone was insufficient: it only
  fixed write ordering inside subflow_set_remote_key(), but the read
  in mptcp_diag_fill_info() was still taken under a different lock
  (lock_sock_fast()) than the writer (mptcp_data_lock(), reached via
  mptcp_propagate_state()). Moved the remote_idsn computation into
  this function's existing mptcp_data_lock() section, next to
  ack_seq, so both are read under the same lock as the writer and
  sampled atomically together, closing the cross-lock race, the
  TOCTOU window between remote_idsn and ack_seq, and (as a side
  effect of using a real lock) any 32-bit tearing concern (reported
  by Sashiko).

Changes since v3:
- Reordered subflow->remote_key_valid to be set after iasn is fully
  populated in subflow_set_remote_key(), instead of before: reading
  remote_key_valid as a readiness flag while iasn could still be
  stale/uninitialized was a data race (reported by Sashiko).
- Added a comment in mptcp_diag_fill_info() documenting why the
  msk->first NULL fallback to 0 cannot be observed by any external
  caller, given the shared lock_sock/lock_sock_fast serialization
  with mptcp_close() (reported by Sashiko, confirmed with Matt).
- Documented in the commit message that this intentionally changes
  the UAPI semantics of the three fields, per Matt's request
  (reported by Sashiko as a compatibility concern).

Changes since v2:
- Dropped the msk->local_idsn/msk->remote_idsn fields entirely.
  Read idsn/iasn from msk->first's subflow context in
  mptcp_diag_fill_info() instead, per Matt's suggestion, to avoid
  growing mptcp_sock for a diag-only need. Verified msk->first only
  goes NULL as part of whole-msk teardown, not during a live
  multi-subflow connection.

Changes since v1:
- Cached msk->remote_idsn before subflow->iasn++ instead of after:
  the increment accounts for the peer's virtual SYN, and caching
  remote_idsn post-increment left mptcpi_rcv_nxt starting at 0 while
  mptcpi_write_seq/mptcpi_snd_una started at 1 for the same
  connection (reported by Sashiko).

v1: https://lore.kernel.org/all/20260825113355.3573376-1-kalpan.jani@mpiricsoftware.com/
v2: https://lore.kernel.org/all/20260827041058.2833707-1-kalpan.jani@mpiricsoftware.com/
v3: https://lore.kernel.org/all/20260902102125.2035540-1-kalpan.jani@mpiricsoftware.com/
v4: https://lore.kernel.org/all/20260903054853.3400545-1-kalpan.jani@mpiricsoftware.com/
---
 net/mptcp/sockopt.c | 28 +++++++++++++++++++++++++---
 net/mptcp/subflow.c |  4 +++-
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 922f6ae5c80cb..56e10d27835e5 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1047,6 +1047,7 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 {
 	struct sock *sk = (struct sock *)msk;
+	u64 local_idsn = 0, remote_idsn = 0;
 	u32 flags = 0;
 	bool slow;
 	u32 now;
@@ -1084,9 +1085,19 @@ 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, write_seq
+	 * and snd_una below are just left unnormalized, which is harmless
+	 * since the socket is already gone.
+	 */
+	if (msk->first)
+		local_idsn = mptcp_subflow_ctx(msk->first)->idsn;
+
 	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;
 	info->mptcpi_retransmits = inet_csk(sk)->icsk_retransmits;
 	info->mptcpi_bytes_sent = msk->bytes_sent;
 	info->mptcpi_bytes_received = msk->bytes_received;
@@ -1099,9 +1110,20 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	unlock_sock_fast(sk, slow);
 
 	mptcp_data_lock(sk);
+	if (msk->first) {
+		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
+
+		/* subflow->iasn is incremented once in subflow_set_remote_key(),
+		 * which runs under this same mptcp_data_lock() (see
+		 * mptcp_propagate_state()); compute remote_idsn here, under the
+		 * same lock as the writer, and atomically with ack_seq below.
+		 */
+		remote_idsn = subflow->remote_key_valid ? subflow->iasn - 1 : 0;
+	}
+
 	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;
 	info->mptcpi_bytes_acked = msk->bytes_acked;
 	mptcp_data_unlock(sk);
 }
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 2d7ccb01d2342..a3313a3db5a76 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -482,7 +482,6 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
 	if (subflow->remote_key_valid)
 		return;
 
-	subflow->remote_key_valid = 1;
 	subflow->remote_key = mp_opt->sndr_key;
 	mptcp_crypto_key_sha(subflow->remote_key, NULL, &subflow->iasn);
 	subflow->iasn++;
@@ -494,6 +493,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;
 }
 
 static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
-- 
2.43.0

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

[sashiko-bot]

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

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

[MPTCP CI]

Hi Kalpan,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: 
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/33748666442

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e18ff27ebcdc
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1157000


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)