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

[PATCH net-next v7] 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 is snapshotted once via READ_ONCE(), instead of being
re-read separately for local_idsn and remote_idsn, so both come from
the same subflow and stay consistent with each other even if
msk->first changes between the two normalizations.

__mptcp_close_ssk() can drop the last reference on the first subflow
(sock_put()) before it clears msk->first, so a plain sock_hold() on
the snapshotted pointer is not safe: if the count has already reached
zero, sock_hold() would resurrect an object mid-teardown. Instead,
refcount_inc_not_zero(&first->sk_refcnt) is used, matching the
existing pattern in mptcp_diag.c and token.c for taking a reference
found via a lockless lookup: on failure, first is treated the same as
NULL. On success, the extra reference keeps the subflow context alive
(it is otherwise released via kfree_rcu()) for the rest of this
function, and is released with sock_put() before returning.

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. If it
somehow did happen mid-flight, 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. 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, 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. 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.

remote_key_valid shares its storage word with other subflow bitfields
(e.g. map_valid), which are written from softirq under the subflow's
own socket lock rather than mptcp_data_lock(). That is unrelated to
this patch: remote_key_valid was already read and written under
mptcp_data_lock(sk) (sk being the parent msk) by
subflow_set_remote_key()/mptcp_propagate_state() before this change,
and mptcp_diag_fill_info()'s new read here uses that same lock,
matching the existing pattern rather than introducing a new one.

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 v6:
- Documented that remote_key_valid's storage word is shared with
  other subflow bitfields (e.g. map_valid) written under a different
  lock (the subflow's own socket lock, from softirq), and that this
  predates this patch: remote_key_valid was already read/written
  under mptcp_data_lock() by subflow_set_remote_key(), and this
  patch's read of it uses that same existing lock rather than a new
  one (reported by Sashiko).

Changes since v5:
- Replaced the two independent READ_ONCE(msk->first) snapshots (one
  per lock section) with a single snapshot taken once and held via
  refcount_inc_not_zero(&first->sk_refcnt), reused in both places
  local_idsn and remote_idsn are computed. Two independent snapshots
  could each briefly return a non-NULL msk->first and then, less than
  a beat apart, see it become NULL, so local_idsn could be computed
  from a real subflow while remote_idsn fell back to 0 for the same
  call, producing an inconsistent baseline between the send and
  receive side. This also replaces the previous rcu_read_lock()-only
  protection, which only covered the lifetime of each lockless
  dereference and not a msk->first read that was itself unprotected
  against a concurrent WRITE_ONCE() from __mptcp_close_ssk()
  (reported by Sashiko).

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/
v5: https://lore.kernel.org/all/20260903105515.906187-1-kalpan.jani@mpiricsoftware.com/
v6: https://lore.kernel.org/all/20260909050916.1785316-1-kalpan.jani@mpiricsoftware.com/
---
 net/mptcp/sockopt.c | 53 ++++++++++++++++++++++++++++++++++++++++++---
 net/mptcp/subflow.c |  4 +++-
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 922f6ae5c80cb..0e6c4cd0dc9f1 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1047,6 +1047,8 @@ 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;
+	struct sock *first;
 	u32 flags = 0;
 	bool slow;
 	u32 now;
@@ -1084,9 +1086,37 @@ 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.
+	 *
+	 * first is snapshotted once here, rather than re-read later, so
+	 * local_idsn and remote_idsn below both come from the same subflow
+	 * and stay consistent with each other even if msk->first changes
+	 * between the two normalizations. __mptcp_close_ssk() can drop the
+	 * last reference on this subflow (sock_put()) before it clears
+	 * msk->first, so refcount_inc_not_zero() is used instead of
+	 * sock_hold(): if the count has already reached zero, first is
+	 * treated the same as NULL instead of resurrecting a dying socket.
+	 * On success, the extra reference keeps the subflow context alive
+	 * for the rest of this function; it is released with sock_put() at
+	 * the end.
+	 */
+	rcu_read_lock();
+	first = READ_ONCE(msk->first);
+	if (first && !refcount_inc_not_zero(&first->sk_refcnt))
+		first = NULL;
+	rcu_read_unlock();
+
+	if (first)
+		local_idsn = mptcp_subflow_ctx(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,11 +1129,28 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	unlock_sock_fast(sk, slow);
 
 	mptcp_data_lock(sk);
+	if (first) {
+		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(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.
+		 * first is the same held reference from earlier in this
+		 * function, so it is guaranteed to still be valid here and to
+		 * be the same subflow local_idsn was computed from above.
+		 */
+		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);
+
+	if (first)
+		sock_put(first);
 }
 EXPORT_SYMBOL_GPL(mptcp_diag_fill_info);
 
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 v7] 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] mptcp: breaks UAPI backward compatibility for sequence numbers
- [Critical] mptcp: use-after-free in mptcp_diag_fill_info when accessing subflow context
--

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

    mptcp: normalize seq numbers reported in mptcp_info

This commit normalizes the MPTCP sequence numbers reported via mptcp_info
by subtracting the local and remote initial sequence numbers. It updates
mptcpi_write_seq, mptcpi_snd_una, and mptcpi_rcv_nxt to provide relative
values instead of absolute on-the-wire 64-bit sequence numbers. The
normalization logic accesses the connection's first subflow context to fetch
these initial sequence numbers.

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..918ec873fcb05 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -1008,9 +1010,37 @@ 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.
> +	 *
> +	 * first is snapshotted once here, rather than re-read later, so
> +	 * local_idsn and remote_idsn below both come from the same subflow
> +	 * and stay consistent with each other even if msk->first changes
> +	 * between the two normalizations. __mptcp_close_ssk() can drop the
> +	 * last reference on this subflow (sock_put()) before it clears
> +	 * msk->first, so refcount_inc_not_zero() is used instead of
> +	 * sock_hold(): if the count has already reached zero, first is
> +	 * treated the same as NULL instead of resurrecting a dying socket.
> +	 * On success, the extra reference keeps the subflow context alive
> +	 * for the rest of this function; it is released with sock_put() at
> +	 * the end.
> +	 */
> +	rcu_read_lock();
> +	first = READ_ONCE(msk->first);
> +	if (first && !refcount_inc_not_zero(&first->sk_refcnt))
> +		first = NULL;
> +	rcu_read_unlock();
> +
> +	if (first)
> +		local_idsn = mptcp_subflow_ctx(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;

[Severity: Critical]
Does this change introduce a UAPI regression?

Since this modifies UAPI fields (mptcpi_write_seq, mptcpi_snd_una, and
mptcpi_rcv_nxt) to use a relative offset instead of the absolute sequence
number, it might permanently break existing userspace tools (like tcpdump) that
rely on absolute on-the-wire values to correlate with packet captures.

>  	info->mptcpi_retransmits = inet_csk(sk)->icsk_retransmits;
>  	info->mptcpi_bytes_sent = msk->bytes_sent;
>  	info->mptcpi_bytes_received = msk->bytes_received;
> @@ -1023,11 +1053,28 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	unlock_sock_fast(sk, slow);

[Severity: Critical]
Does taking a reference on the socket's sk_refcnt earlier actually protect the
subflow ULP context across this lock gap?

When we drop the socket lock here via unlock_sock_fast(), could a concurrent
mptcp_close() acquire the lock and call __tcp_close(), which unconditionally
executes tcp_cleanup_ulp()?

If tcp_cleanup_ulp() runs, it frees the ULP context via kfree_rcu() regardless
of the elevated sk_refcnt. Since unlock_sock_fast() can sleep and we are not
in an RCU read-side critical section, the RCU grace period might elapse before
we re-acquire the lock below.

>  
>  	mptcp_data_lock(sk);
> +	if (first) {
> +		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(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.
> +		 * first is the same held reference from earlier in this
> +		 * function, so it is guaranteed to still be valid here and to
> +		 * be the same subflow local_idsn was computed from above.
> +		 */
> +		remote_idsn = subflow->remote_key_valid ? subflow->iasn - 1 : 0;

Could this regression result in a use-after-free when reading subflow->iasn if
the ULP context was freed during the lock gap?

> +	}
> +
>  	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);
> +
> +	if (first)
> +		sock_put(first);
>  }

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

Re: [PATCH net-next v7] 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): Unstable: 1 failed test(s): selftest_simult_flows ⚠️ 
- 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: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/36704096200

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


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)

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 1 · High: 1 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] Use-after-free / NULL pointer dereference in
  mptcp_diag_fill_info().
- [High] mptcp_diag_fill_info() (net/mptcp/sockopt.c:1033-1072) reads
  local_idsn, remote_key_valid and iasn from msk->first's subflow…
- [Medium] The patch changes the meaning of three existing uAPI fields in
  struct mptcp_info (mptcpi_write_seq, mptcpi_snd_una, mptcpi_rcv_nxt;…
- [Medium] The commit claims that remote_idsn is read 'under the same
  lock the writer uses' and that local_idsn and remote_idsn 'stay…
- [Low] The per-version changelog ("Changes since v6" down to "Changes
  since v1", plus the v1-v6 lore links) is in the commit message body…
- [Low] After this patch, data-sequence-space values exported by MPTCP
  diag interfaces use different reference points.
- [Low] tools/testing/selftests/net/mptcp/mptcp_sockopt.c
  do_getsockopt_mptcp_info() uses `s->mi.mptcpi_write_seq == 0` as a 'no…

--

mptcp: normalize seq numbers reported in mptcp_info

This patch normalizes mptcpi_write_seq, mptcpi_snd_una and mptcpi_rcv_nxt
in mptcp_diag_fill_info(). It subtracts the local IDSN and the remote IASN,
both read from the subflow context of msk->first, and holds a reference on
msk->first for the whole function. It also moves the remote_key_valid
assignment in subflow_set_remote_key() so it happens after iasn is set.

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

> Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
>
> Changes since v6:

[Severity: Low]
This isn't a bug, but the per-version changelog is in the commit message
body, after the Signed-off-by trailer. That covers "Changes since v6" down
to "Changes since v1", plus the v1-v6 lore links.

Should all of that go below the --- separator so it stays out of the git
log?

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index fcf6feb2a9eb5..918ec873fcb05 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c

[ ... ]

> @@ -1008,9 +1010,37 @@ 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.
> +	 *
> +	 * first is snapshotted once here, rather than re-read later, so
> +	 * local_idsn and remote_idsn below both come from the same subflow
> +	 * and stay consistent with each other even if msk->first changes
> +	 * between the two normalizations. __mptcp_close_ssk() can drop the
> +	 * last reference on this subflow (sock_put()) before it clears
> +	 * msk->first, so refcount_inc_not_zero() is used instead of
> +	 * sock_hold(): if the count has already reached zero, first is
> +	 * treated the same as NULL instead of resurrecting a dying socket.
> +	 * On success, the extra reference keeps the subflow context alive
> +	 * for the rest of this function; it is released with sock_put() at
> +	 * the end.
> +	 */
> +	rcu_read_lock();
> +	first = READ_ONCE(msk->first);
> +	if (first && !refcount_inc_not_zero(&first->sk_refcnt))
> +		first = NULL;
> +	rcu_read_unlock();
> +
> +	if (first)
> +		local_idsn = mptcp_subflow_ctx(first)->idsn;

[Severity: High]
Can this baseline be wiped while the connection is still established?

Suppose the peer resets the initial MPC subflow, or it times out, while
other subflows keep the msk in TCP_ESTABLISHED. The worker then runs
__mptcp_close_subflow()->mptcp_close_ssk()->__mptcp_close_ssk().

free_first is only set in mptcp_destroy(), so for the first subflow of a
live msk:

net/mptcp/protocol.c:__mptcp_close_ssk() {
    ...
	dispose_it = msk->free_first || ssk != msk->first;
    ...
	if (!dispose_it) {
		__mptcp_subflow_disconnect(ssk, subflow, msk->fastclosing);
		release_sock(ssk);

		goto out;
	}
    ...
}

On a TCP_CLOSE ssk, __mptcp_subflow_disconnect() calls
mptcp_subflow_ctx_reset():

	memset(&subflow->reset, 0, sizeof(subflow->reset));

The reset struct_group covers idsn, the iasn union and remote_key_valid.
msk->first is not cleared on this path.

>From then on, every getsockopt(MPTCP_INFO) or inet_diag query would get
local_idsn == 0 and remote_idsn == 0. mptcpi_write_seq, mptcpi_snd_una and
mptcpi_rcv_nxt would then jump in the middle of the connection, from small
normalized values back to the raw DSNs.

That would make two statements inaccurate. The commit message says
"subflow->idsn is set once at handshake time and never modified
afterwards". The comment above says "which is harmless since the socket is
already gone".

The testing described in the commit message watched for msk->first
becoming NULL. It would not catch this case, because the pointer stays set
and only the context is zeroed.

Does the baseline need to live somewhere that survives a subflow ctx
reset, for example cached on the msk as in v2?

>  	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: Medium]
This changes the meaning of three existing uAPI fields in struct
mptcp_info, with no new field or flag.

The inet_diag path also uses mptcp_diag_fill_info():

mptcp_diag_get_info()->mptcp_diag_fill_info()

So "ss -M -i" and other INET_DIAG_INFO consumers get the new semantics too.
The commit message only discusses getsockopt(MPTCP_INFO) and the selftest.

After this change no unprivileged interface exposes the raw values, and
mptcp_info does not export the keys needed to recompute the offset.

The commit message also seems to contradict itself. It says the raw values
"are not useful to userspace as absolute values". It then says tools that
correlate against captured DSS sequence numbers "will need to account for
the offset".

How would those tools recover the offset? Should the commit message at
least mention the inet_diag consumers?

[Severity: Low]
This isn't a bug, but do_getsockopt_mptcp_info() in
tools/testing/selftests/net/mptcp/mptcp_sockopt.c treats a zero write_seq
as "no baseline sample yet", and it only checks deltas:

	if (s->mi.mptcpi_write_seq == 0)
		s->mi = i;

	assert(s->mi.mptcpi_write_seq + w == i.mptcpi_write_seq);

With the new semantics, 0 is a valid normalized value before the handshake
completes, because mptcp_connect() sets msk->write_seq to subflow->idsn.
The test only samples after connect()/accept(), so it does not fail today.

Because only differences are checked, the test passes whether or not the
normalization is applied. Could the selftest also check the absolute
normalized values?

> @@ -1023,11 +1053,28 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	unlock_sock_fast(sk, slow);
>  
>  	mptcp_data_lock(sk);
> +	if (first) {
> +		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(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.
> +		 * first is the same held reference from earlier in this
> +		 * function, so it is guaranteed to still be valid here and to
> +		 * be the same subflow local_idsn was computed from above.
> +		 */
> +		remote_idsn = subflow->remote_key_valid ? subflow->iasn - 1 : 0;

[Severity: Critical]
Does the reference on first->sk_refcnt really keep the subflow context
alive here? The ctx is a separate allocation reached through
icsk_ulp_data. It can be freed or detached no matter what sk_refcnt is.

At this point unlock_sock_fast() has dropped the msk socket lock and no
rcu_read_lock() is held. mptcp_data_lock() is only
spin_lock_bh(&sk->sk_lock.slock), and it does not exclude a task that owns
the msk through lock_sock().

The first path is close() on the msk:

__mptcp_close()->__mptcp_destroy_sock()->mptcp_destroy()
  msk->free_first = 1;
  mptcp_destroy_common()->__mptcp_close_ssk(first)

On that dispose path the ctx is either freed directly:

	if (!inet_csk(ssk)->icsk_ulp_ops) {
		WARN_ON_ONCE(!sock_flag(ssk, SOCK_DEAD));
		kfree_rcu(subflow, rcu);
	} else {
		/* otherwise tcp will dispose of the ssk and subflow ctx */
		__tcp_close(ssk, 0);

or TCP destroy reaches subflow_ulp_release(). That function does
kfree_rcu(ctx, rcu) without clearing icsk_ulp_data, which leaves a
dangling pointer.

The second path is closing a listener that still has unaccepted children:

mptcp_subflow_queue_clean()
  lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
  __mptcp_unaccepted_force_close(sk)
    ...->__mptcp_close_ssk()
      mptcp_subflow_drop_ctx(ssk)
        subflow_ulp_fallback()
          rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
        kfree_rcu(ctx, rcu);

Here the second mptcp_subflow_ctx(first) call returns NULL, so reading
subflow->remote_key_valid dereferences a NULL pointer.

An inet_diag dump reaches mptcp_diag_fill_info() through
mptcp_diag_dump()->...->inet_sk_diag_fill()->mptcp_diag_get_info(). It
holds only an msk reference, and INET_DIAG_INFO needs no capability, so a
dump can race with either path.

In the kfree_rcu() case, the freed memory ends up in mptcpi_rcv_nxt and is
returned to userspace.

The earlier local_idsn read runs under the msk socket lock, so only this
second read seems affected. The comments saying the reference "keeps the
subflow context alive" and that first "is guaranteed to still be valid
here" also look inaccurate.

Could remote_key_valid and iasn be sampled inside the lock_sock_fast()
section instead? Another option is rcu_read_lock() plus
rcu_dereference(inet_csk(first)->icsk_ulp_data) with a NULL check, as
subflow_get_info() in net/mptcp/diag.c does.

[Severity: Medium]
Apart from the lifetime question, is mptcp_data_lock() really the lock
that serializes every writer of these fields?

subflow_set_remote_key() is not the only writer. mptcp_subflow_ctx_reset()
also writes idsn, iasn and remote_key_valid through its memset of the reset
group. It runs from __mptcp_subflow_disconnect() under lock_sock(msk) and
lock_sock_nested(ssk), not under mptcp_data_lock():

mptcp_worker()
  __mptcp_close_subflow()->mptcp_close_ssk()->__mptcp_close_ssk()
    __mptcp_subflow_disconnect()
      mptcp_subflow_ctx_reset()
        memset(&subflow->reset, 0, sizeof(subflow->reset));

This function reads local_idsn under lock_sock_fast(), drops the msk lock,
and then reads remote_key_valid and iasn under mptcp_data_lock(). In that
gap the worker can take lock_sock(msk) and reset the ctx of a TCP_CLOSE
first subflow, while msk->first stays the same.

That gives a plain read racing with the memset. It can also give a single
reply where write_seq and snd_una use the original IDSN, but rcv_nxt is
either raw (remote_idsn == 0) or uses a different baseline.

Doesn't that contradict the commit message, which says remote_idsn is read
"under the same lock the writer uses" and that both values "stay
consistent with each other"?

> +	}
> +
>  	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: Low]
After this change the msk-level fields are relative to IDSN/IASN. The
per-subflow ULP diag info still exports a raw DSN from the same sequence
space. subflow_get_info() in net/mptcp/diag.c does:

	     nla_put_u64_64bit(skb, MPTCP_SUBFLOW_ATTR_MAP_SEQ, sf->map_seq,
			       MPTCP_SUBFLOW_ATTR_PAD) ||

and map_seq starts from subflow->iasn.

Tools that compare the msk rcv_nxt with subflow mappings, such as ss
showing both MPTCP and TCP ULP info, would now see the two differ by a
random 64-bit offset.

Should MPTCP_SUBFLOW_ATTR_MAP_SEQ be normalized the same way, or should the
difference at least be documented?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930102804.2796208-1-kalpan.jani%40mpiricsoftware.com

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

[Matthieu Baerts]

Hi Kalpan,

On 30/09/2026 12:28, Kalpan Jani wrote:
> 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.

Thank you for your patience! It looks like it becomes way more
complicated than expected, e.g. I didn't think about
mptcp_subflow_ctx_reset() being called on the first subflow. Maybe it
should only be called when coming from mptcp_disconnect()?

Also, perhaps mptcp_diag_fill_info() can stop early in case of
disconnected MPTCP socket? Most fields are reset on disconnection, so no
need to bother. (And/Or check icsk_ulp_data validity?)

About the patch itself, and similar to the general "kernel code style",
please avoid big comments in the code, and huge amount of text in the
commit message simply describing the code change. I know LLM loves doing
that, but that's not human friendly. What is important to add in a
commit message are the reasons why you need this modification, and why
you took one way and not another (+ pitfalls/remarks), but no need to
describe the diff. If it starts to be too long, that's probably because
you need to split your patch. If you need to add text to "please"
sashiko, please mention it somehow.

Note: no need to address the 'low' comments from Clashiko (except the
one about the '---' missing after the Signed-off-by).

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.