net/mptcp/sockopt.c | 53 ++++++++++++++++++++++++++++++++++++++++++--- net/mptcp/subflow.c | 4 +++- 2 files changed, 53 insertions(+), 4 deletions(-)
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() and held with
sock_hold() for the duration of this function, instead of being
re-read separately for local_idsn and remote_idsn. This avoids two
problems: a lockless double-fetch of msk->first (the pointer could
change between an "is it set" check and its use), and local_idsn and
remote_idsn being computed from two different subflows if msk->first
changed between the two normalizations. The subflow context is
otherwise released via kfree_rcu() once msk->first is cleared and the
last reference drops, so holding a reference keeps it valid regardless
of what mptcp_close()/__mptcp_close_ssk() does concurrently; the hold
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.
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 v5:
- Replaced the two independent READ_ONCE(msk->first) snapshots (one
per lock section) with a single snapshot taken once and held via
sock_hold(), 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/
---
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.0Hi Matt,
Sashiko flagged a possible bitfield word-sharing race in v6:
remote_key_valid is read in mptcp_diag_fill_info() under
mptcp_data_lock(msk), but it shares a storage word with map_valid,
which is written in subflow_check_data_avail() and
subflow_data_ready() under the subflow socket's own lock in softirq
context. Different socks, different locks, so a concurrent read and
read-modify-write on that u32 looks possible.
As far as I can tell this is pre-existing rather than something this
patch introduces - remote_key_valid was already read and written from
those paths before, and the patch only adds one more reader. Is this
a known/accepted characteristic here, or does it need addressing? And
if it does, should that be part of this patch or a separate one?
Cheers,
Kalpan Jani
From: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
To: <mptcp@lists.linux.dev>
Cc: <shardul.b@mpiricsoftware.com>, <janak@mpiric.us>, <kalpanjani009@gmail.com>, "Kalpan Jani"<kalpan.jani@mpiricsoftware.com>
Date: Wed, 09 Sep 2026 15:18:00 +0530
Subject: [PATCH net-next v6] mptcp: normalize seq numbers reported in mptcp_info
> 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() and held with
> sock_hold() for the duration of this function, instead of being
> re-read separately for local_idsn and remote_idsn. This avoids two
> problems: a lockless double-fetch of msk->first (the pointer could
> change between an "is it set" check and its use), and local_idsn and
> remote_idsn being computed from two different subflows if msk->first
> changed between the two normalizations. The subflow context is
> otherwise released via kfree_rcu() once msk->first is cleared and the
> last reference drops, so holding a reference keeps it valid regardless
> of what mptcp_close()/__mptcp_close_ssk() does concurrently; the hold
> 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.
>
> 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 v5:
> - Replaced the two independent READ_ONCE(msk->first) snapshots (one
> per lock section) with a single snapshot taken once and held via
> sock_hold(), 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/
> ---
> 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
>
>
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/34338526910
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/09de406972ff
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1161206
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)
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The commit intentionally changes the semantics of UAPI fields (`mptcpi_write_seq`, `mptcpi_snd_una`, `mptcpi_rcv_nxt`) from absolute on-the-wire sequence numbers to relative sequence numbers, explicitly breaking existing userspace tools.
- [Medium] Unsynchronized concurrent read and read-modify-write of bitfields in `struct mptcp_subflow_context`.
--
commit aed02b920bd2e723a86c213c2ac56f6fb6ab0d6e
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
mptcp: normalize seq numbers reported in mptcp_info
This patch modifies mptcpi_write_seq, mptcpi_snd_una, and mptcpi_rcv_nxt
to report relative sequence numbers rather than raw 64-bit data sequence
numbers. It does this by subtracting the initial sequence numbers from the
subflow context. The commit message notes this intentionally changes UAPI
semantics.
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_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]
Will this change break existing userspace tools?
The commit message mentions this intentionally changes the semantics of
mptcpi_write_seq, mptcpi_snd_una, and mptcpi_rcv_nxt from absolute
on-the-wire sequence numbers to relative sequence numbers.
Tools and scripts correlating these sequence numbers with network captures,
like those using tcpdump, might break when relying on the previous UAPI
semantics.
Is it acceptable to introduce a backward compatibility regression in the
exported UAPI values here?
> 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);
>
> 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: Medium]
Does this introduce a data race on the u32 container holding the
remote_key_valid bitfield?
The mptcp_diag_fill_info() function reads remote_key_valid while holding
mptcp_data_lock(sk). However, other concurrent writers modifying neighboring
bitfields in the same u32 container (like map_valid) do so under
bh_lock_sock(first) during softirq packet processing.
Because these locks are distinct, doesn't this result in an unmitigated
concurrent read and read-modify-write on the shared u32 word when diagnostic
tools are run concurrently with active connections?
> + }
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909094800.766170-1-kalpan.jani@mpiricsoftware.com?part=1
© 2016 - 2026 Red Hat, Inc.