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

[sashiko-bot@kernel.org]

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