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

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

[Kalpan Jani]

The sequence numbers exposed through MPTCP_INFO include random initial
data sequence numbers, making their absolute values difficult for
userspace to interpret.

Report write_seq and snd_una relative to the local IDSN, and rcv_nxt
relative to the peer's initial data sequence number.

Read the sequence baselines from the subflow context under RCU, handle
a missing context, and skip disconnected sockets. Publish
remote_key_valid only after initializing iasn.

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

---
Changes since v7:
- Return early for MPTCP sockets in TCP_CLOSE state.
- Access icsk_ulp_data under RCU and handle a NULL context
  when obtaining the local and remote IDSN values.
- Zero-initialize the MPTCP_INFO output structure.

v7: https://lore.kernel.org/all/20260930102804.2796208-1-kalpan.jani@mpiricsoftware.com/
---
 net/mptcp/sockopt.c | 37 ++++++++++++++++++++++++++++++++-----
 net/mptcp/subflow.c |  4 +++-
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 922f6ae5c80cb..c441c49e7f5f7 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1046,7 +1046,10 @@ 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 mptcp_subflow_context *subflow;
 	struct sock *sk = (struct sock *)msk;
+	u64 local_idsn = 0, remote_idsn = 0;
+	struct sock *first;
 	u32 flags = 0;
 	bool slow;
 	u32 now;
@@ -1058,7 +1061,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	info->mptcpi_add_addr_accepted = READ_ONCE(msk->pm.add_addr_accepted);
 	info->mptcpi_local_addr_used = READ_ONCE(msk->pm.local_addr_used);
 
-	if (inet_sk_state_load(sk) == TCP_LISTEN)
+	if (inet_sk_state_load(sk) == TCP_LISTEN ||
+	    inet_sk_state_load(sk) == TCP_CLOSE)
 		return;
 
 	/* The following limits only make sense for the in-kernel PM */
@@ -1084,9 +1088,23 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	info->mptcpi_flags = flags;
 
 	slow = lock_sock_fast(sk);
+	/* Hold a reference to the first subflow while accessing its context. */
+	rcu_read_lock();
+	first = READ_ONCE(msk->first);
+	if (first && !refcount_inc_not_zero(&first->sk_refcnt))
+		first = NULL;
+	rcu_read_unlock();
+
+	rcu_read_lock();
+	subflow = first ?
+		rcu_dereference(inet_csk(first)->icsk_ulp_data) : NULL;
+	if (subflow)
+		local_idsn = subflow->idsn;
+	rcu_read_unlock();
+
 	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,17 +1117,26 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	unlock_sock_fast(sk, slow);
 
 	mptcp_data_lock(sk);
+	rcu_read_lock();
+	subflow = first ?
+		rcu_dereference(inet_csk(first)->icsk_ulp_data) : NULL;
+	if (subflow && subflow->remote_key_valid)
+		remote_idsn = subflow->iasn - 1;
+	rcu_read_unlock();
 	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);
 
 static int mptcp_getsockopt_info(struct mptcp_sock *msk, char __user *optval, int __user *optlen)
 {
-	struct mptcp_info m_info;
+	struct mptcp_info m_info = {};
 	int len;
 
 	if (get_user(len, optlen))
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 v8] 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:
- [Medium] mptcp: MPTCP_INFO seq numbers revert to absolute on disconnect
- [Medium] mptcp: MPTCP_INFO drops statistics for TCP_CLOSE sockets
--

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

    mptcp: normalize seq numbers reported in mptcp_info

    The commit changes MPTCP_INFO to report write_seq and snd_una relative to
    the local IDSN, and rcv_nxt relative to the peer's initial data sequence
    number. It reads these baselines from the subflow context under RCU,
    handles missing contexts, and skips disconnected sockets.

    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..76bab204fc33b 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -982,7 +985,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	info->mptcpi_add_addr_accepted = READ_ONCE(msk->pm.add_addr_accepted);
>  	info->mptcpi_local_addr_used = READ_ONCE(msk->pm.local_addr_used);
>  
> -	if (inet_sk_state_load(sk) == TCP_LISTEN)
> +	if (inet_sk_state_load(sk) == TCP_LISTEN ||
> +	    inet_sk_state_load(sk) == TCP_CLOSE)
>  		return;

[Severity: Medium]
Will this early return cause MPTCP_INFO to drop final statistics for sockets
in the TCP_CLOSE state?

Since the info structure is zero-initialized at the start of the function,
returning early means userspace tools reading MPTCP_INFO on a fully closed
socket will see zeroes for all historical connection stats like
mptcpi_bytes_sent and mptcpi_retransmits, unlike standard TCP.

[ ... ]

> @@ -1008,9 +1012,23 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	info->mptcpi_flags = flags;
>  
>  	slow = lock_sock_fast(sk);
> +	/* Hold a reference to the first subflow while accessing its context. */
> +	rcu_read_lock();
> +	first = READ_ONCE(msk->first);
> +	if (first && !refcount_inc_not_zero(&first->sk_refcnt))
> +		first = NULL;
> +	rcu_read_unlock();
> +
> +	rcu_read_lock();
> +	subflow = first ?
> +		rcu_dereference(inet_csk(first)->icsk_ulp_data) : NULL;
> +	if (subflow)
> +		local_idsn = subflow->idsn;

[Severity: Medium]
Can local_idsn be read as zero if the first subflow was reset?

If the MPTCP connection survives but the first subflow is closed,
__mptcp_subflow_disconnect() invokes mptcp_subflow_ctx_reset(). This zeroes
the reset struct group, clearing subflow->idsn to 0.

If this occurs, subtracting 0 will suddenly expose raw absolute sequence
numbers to userspace instead of the intended relative values.

> +	rcu_read_unlock();
> +
>  	info->mptcpi_csum_enabled = READ_ONCE(msk->csum_enabled);
>  	info->mptcpi_token = msk->token;

[ ... ]

> @@ -1023,17 +1041,26 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	unlock_sock_fast(sk, slow);
>  
>  	mptcp_data_lock(sk);
> +	rcu_read_lock();
> +	subflow = first ?
> +		rcu_dereference(inet_csk(first)->icsk_ulp_data) : NULL;
> +	if (subflow && subflow->remote_key_valid)
> +		remote_idsn = subflow->iasn - 1;

[Severity: Medium]
Similarly, can subflow->iasn be read as zero or bypassed here?

If mptcp_subflow_ctx_reset() has been called, subflow->remote_key_valid and
subflow->iasn are also zeroed out. This would cause remote_idsn to remain 0,
again exposing absolute sequence numbers for mptcpi_rcv_nxt.

> +	rcu_read_unlock();
>  	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/20261014130809.1874184-1-kalpan.jani@mpiricsoftware.com?part=1

Re: [PATCH net-next v8] 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: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/38574170831

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


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 v8] mptcp: normalize seq numbers reported in mptcp_info

[netdev-bot+sashiko]

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

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

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 read in mptcp_diag_fill_info()
  (net/mptcp/sockopt.c:1043-1049).
- [High] mptcp_diag_fill_info() (net/mptcp/sockopt.c) takes both sequence
  baselines from the initial subflow's context: local_idsn =…
- [Medium] The new early return in mptcp_diag_fill_info()
  (net/mptcp/sockopt.c:988-990) also triggers for TCP_CLOSE.
- [Medium] mptcp_diag_fill_info() reads the sequence baselines under
  locks that do not serialize with the writer that resets them, so it…

--

mptcp: normalize seq numbers reported in mptcp_info

This patch reports mptcpi_write_seq and mptcpi_snd_una relative to the
local IDSN, and mptcpi_rcv_nxt relative to the peer's IDSN. Both baselines
are read from the initial subflow's context. It also makes
mptcp_diag_fill_info() return early for TCP_CLOSE sockets, and moves the
remote_key_valid assignment in subflow_set_remote_key() to after iasn is
set up.

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

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index fcf6feb2a9eb..76bab204fc33 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c

[ ... ]

> @@ -982,7 +985,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	info->mptcpi_add_addr_accepted = READ_ONCE(msk->pm.add_addr_accepted);
>  	info->mptcpi_local_addr_used = READ_ONCE(msk->pm.local_addr_used);
>  
> -	if (inet_sk_state_load(sk) == TCP_LISTEN)
> +	if (inet_sk_state_load(sk) == TCP_LISTEN ||
> +	    inet_sk_state_load(sk) == TCP_CLOSE)
>  		return;

[Severity: Medium]
Does this early return drop more than the sequence fields for closed
sockets?

Before this patch, only TCP_LISTEN returned here. Now a TCP_CLOSE msk also
skips these fields:

  - mptcpi_flags, including MPTCP_INFO_FLAG_FALLBACK
  - mptcpi_token and mptcpi_csum_enabled
  - mptcpi_retransmits and the bytes_* counters
  - mptcpi_subflows_total
  - the last_data_sent, last_data_recv and last_ack_recv timestamps

MPTCP has no msk-level TIME_WAIT. After a graceful shutdown, a socket the
application still holds is left in TCP_CLOSE, for example in
mptcp_check_data_fin():

	case TCP_FIN_WAIT2:
		mptcp_shutdown_subflows(msk);
		mptcp_set_state(sk, TCP_CLOSE);

Say an application calls shutdown(), reads EOF and then asks MPTCP_INFO
for final stats. It would get a mostly zeroed struct, and the call would
still succeed.

mptcp_getsockopt_full_info() would return this zeroed mptcp_info next to
populated subflow and tcp_info arrays. mptcp_diag_get_info() (ss -M) would
lose the counters and the fallback flag for closed sockets.

Would it be enough to skip only the sequence number normalization in this
state?

> @@ -1008,9 +1012,23 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	info->mptcpi_flags = flags;
>  
>  	slow = lock_sock_fast(sk);
> +	/* Hold a reference to the first subflow while accessing its context. */
> +	rcu_read_lock();
> +	first = READ_ONCE(msk->first);
> +	if (first && !refcount_inc_not_zero(&first->sk_refcnt))
> +		first = NULL;
> +	rcu_read_unlock();
> +
> +	rcu_read_lock();
> +	subflow = first ?
> +		rcu_dereference(inet_csk(first)->icsk_ulp_data) : NULL;
> +	if (subflow)
> +		local_idsn = subflow->idsn;
> +	rcu_read_unlock();
> +
>  	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: High]
Can these baselines drop back to zero while the connection is still
running?

idsn, iasn and remote_key_valid all live in the reset struct_group of
mptcp_subflow_context. The initial subflow's context can be cleared while
the msk stays established.

Suppose the initial subflow gets a peer RST while MP_JOIN subflows keep the
msk in TCP_ESTABLISHED. The worker then runs:

mptcp_worker()
  __mptcp_close_subflow()
    mptcp_close_ssk()
      __mptcp_close_ssk()
        dispose_it = msk->free_first || ssk != msk->first;  /* false */
        __mptcp_subflow_disconnect()
          tcp_disconnect(ssk, 0);
          mptcp_subflow_ctx_reset(subflow);
            memset(&subflow->reset, 0, sizeof(subflow->reset));

msk->first still points at that socket and the msk is still
TCP_ESTABLISHED. The new TCP_CLOSE check looks at the msk state, not the
first subflow's state, so it doesn't catch this.

After that, local_idsn and remote_idsn are both 0. mptcpi_write_seq,
mptcpi_snd_una and mptcpi_rcv_nxt jump back to absolute values in the
middle of the connection. Anything that computes deltas between samples,
like the mptcp_sockopt selftest, would see a random 64-bit jump.

The "missing context" fallback mentioned in the commit message also
silently reports absolute values.

Could the baselines come from state that lives as long as the msk? For
example, derive them from msk->local_key and msk->remote_key with
mptcp_crypto_key_sha(), or cache them in the msk.

[ ... ]

> @@ -1023,17 +1041,26 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	unlock_sock_fast(sk, slow);
>  
>  	mptcp_data_lock(sk);
> +	rcu_read_lock();
> +	subflow = first ?
> +		rcu_dereference(inet_csk(first)->icsk_ulp_data) : NULL;
> +	if (subflow && subflow->remote_key_valid)
> +		remote_idsn = subflow->iasn - 1;
> +	rcu_read_unlock();

[Severity: Critical]
Is the subflow context still alive when it is dereferenced here?

The msk socket lock has been dropped at this point. Only the msk data lock
and a new RCU read-side section are held. The reference on first keeps the
TCP socket alive, but not its MPTCP subflow context.

Neither release path clears icsk_ulp_data. subflow_ulp_release() does:

	mptcp_subflow_ops_undo_override(ssk);
	if (release)
		kfree_rcu(ctx, rcu);

tcp_cleanup_ulp() only sets icsk_ulp_ops to NULL. __mptcp_close_ssk()
calls kfree_rcu(subflow, rcu) directly when icsk_ulp_ops is already NULL.

Consider a sock_diag dump racing with close() on the same socket:

CPU1 (sock_diag)
mptcp_diag_get_info()
  mptcp_diag_fill_info()
    takes a ref on first, reads local_idsn
    unlock_sock_fast(sk, slow);
    <preempted>

CPU2 (close)
__mptcp_close()
  __mptcp_destroy_sock()
    mptcp_destroy()
      msk->free_first = 1;
      mptcp_destroy_common()
        __mptcp_close_ssk(first)
          dispose_it is true
          kfree_rcu() of the ctx, either directly or via
          __tcp_close()->tcp_cleanup_ulp()->subflow_ulp_release()
          WRITE_ONCE(msk->first, NULL);

CPU1
    mptcp_data_lock(sk);
    rcu_read_lock();
    rcu_dereference(inet_csk(first)->icsk_ulp_data)  /* stale, non-NULL */
    reads subflow->remote_key_valid and subflow->iasn

CPU2 owns the msk lock but doesn't hold the slock, so CPU1 can take
mptcp_data_lock() at the same time.

CPU1's rcu_read_lock() starts after kfree_rcu() was queued, so the grace
period doesn't cover it. The NULL check doesn't help either, because
icsk_ulp_data was never cleared.

Can this read freed slab memory and copy a value derived from it to
userspace through mptcpi_rcv_nxt? INET_DIAG_INFO for MPTCP doesn't require
CAP_NET_ADMIN.

Before this patch, mptcp_diag_fill_info() never touched the subflow
context outside the msk socket lock. Inside that lock, a non-NULL
msk->first means the context is still alive.

Would reading iasn and remote_key_valid inside the lock_sock_fast()
section avoid this? Deriving or caching the remote IDSN at the msk level
would also work.

>  	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]
Leaving the lifetime question aside, do these reads give a consistent
snapshot?

local_idsn is read under the msk socket lock. remote_key_valid and iasn are
read later, under only mptcp_data_lock().

mptcp_subflow_ctx_reset() runs from
__mptcp_close_ssk()->__mptcp_subflow_disconnect(). That path holds msk
ownership and the ssk lock, but not the msk data lock. So nothing orders
the memset against this reader.

The TCP_CLOSE check at the top of mptcp_diag_fill_info() is a lockless
inet_sk_state_load() done before any lock is taken. It can't rule out a
concurrent disconnect, such as:

  - the worker closing the initial subflow
  - a fastclose
  - mptcp_disconnect() via connect(AF_UNSPEC) from another thread

What if the reader sees the old remote_key_valid == 1 together with the
cleared iasn == 0? Then remote_idsn becomes (u64)-1 and mptcpi_rcv_nxt is
reported as ack_seq + 1.

local_idsn and snd_una/ack_seq can also come from different critical
sections. After a disconnect and reconnect, they can even come from
different connections.

remote_key_valid also shares a bitfield word with flags like map_valid and
mpc_map, which the ssk receive path writes under the ssk lock. Would KCSAN
flag this plain read done under only the msk data lock?

[ ... ]

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