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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Kalpan,

On 02/09/2026 12:36, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Changing existing UAPI sequence fields from absolute to relative breaks backwards compatibility.
> - [High] Normalization falls back to 0 when `msk->first` is NULL, causing massive UAPI metric jumps for closed sockets.
> - [Medium] Data race on `subflow->iasn` and `subflow->remote_key_valid` yields corrupted UAPI values.
> --
> 
> commit 630c5d1d1841bed28f7903466f4319e79bf43c4c
> Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
> 
> 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. This commit normalizes mptcpi_write_seq, mptcpi_snd_una
> and mptcpi_rcv_nxt in mptcp_diag_fill_info() by subtracting the local and
> remote 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..88bd75672c96e 100644
>> --- a/net/mptcp/sockopt.c
>> +++ b/net/mptcp/sockopt.c
> [ ... ]
>> @@ -1008,9 +1009,22 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>>  	info->mptcpi_flags = flags;
>>  
>>  	slow = lock_sock_fast(sk);
>> +
>> +	if (msk->first) {
>> +		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
>> +
>> +		local_idsn = subflow->idsn;
>> +		/* subflow->iasn is incremented once in subflow_set_remote_key()
>> +		 * to account for the peer's virtual SYN; undo that here so
>> +		 * rcv_nxt normalizes against the same baseline write_seq and
>> +		 * snd_una use.
>> +		 */
>> +		remote_idsn = subflow->remote_key_valid ? subflow->iasn - 1 : 0;
> 
> [Severity: High]
> When the MPTCP connection enters TCP_CLOSE, msk->first can be set to NULL.
> When diagnostic queries execute mptcp_diag_fill_info(), local_idsn and
> remote_idsn will remain 0.
> 
> Does subtracting 0 cause the UAPI output to abruptly revert to the raw 64-bit
> absolute sequence numbers?
> 
> For diff-based monitoring tools relying on the new relative behavior, could
> this cause an astronomical jump in reported sequence values?

I would say (without checking the details), in this case:

- either we return 0 for the 3 different values below

- or, when closing the first subflow, the 3 fields in the msk are
updated with the offset.

But, before that, is it still possible to retrieve info from this socket
after the destroy part? If I'm not mistaken, this destroy part is
reached only after a "close(fd)", not a disconnect or anything else. So
the userspace shouldn't reach it via a `getsockopt(MPTCP_INFO)` (or
similar).

I guess there is still possible to reach this code after the destroy
part when diag info are obtained from netlink (e.g. `ss`) in parallel.
But that seems to be more a race than a specific "post-close" query, no?
If yes, then the main point is not to crash. Then maybe a workaround is
enough, e.g. resetting fields, or modifying them with an offset.

> [Severity: Medium]
> Is there a data race when accessing subflow->remote_key_valid and subflow->iasn
> here?
> 
> mptcp_diag_fill_info() accesses these fields under lock_sock_fast(), which only
> acquires the process context lock. Concurrently, subflow_set_remote_key()
> writes to these fields in softirq context under mptcp_data_lock():
> 
> net/mptcp/subflow.c:subflow_set_remote_key() {
> 	...
> 	subflow->remote_key_valid = 1;
> 	subflow->remote_key = mp_opt->sndr_key;
> 	mptcp_crypto_key_sha(subflow->remote_key, NULL, &subflow->iasn);
> 	subflow->iasn++;
> 	...
> }
> 
> Because remote_key_valid is set to 1 before mptcp_crypto_key_sha() populates
> iasn, can this diagnostic code read iasn while it contains garbage?

Set "remote_key_valid = 1" after having set iasn?

> 
>> +	}
>> +
>>  	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 changing the established UAPI semantics of mptcpi_write_seq,
> mptcpi_snd_una, and mptcpi_rcv_nxt from absolute cryptographic sequence numbers
> to 0-indexed relative values cause a regression in the kernel's ABI?
> 
> Could any userspace application or tracing tool that relies on these fields to
> correlate socket state with on-the-wire MPTCP packets (e.g., using tcpdump to
> match DSS sequence numbers) break immediately?

That's on purpose, adding a specific note in the commit message is
enough? People shouldn't expect these counters to be exactly the same as
on the wire.

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