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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Asymmetric normalization of local and remote sequence numbers creates an off-by-one inconsistency in MPTCP_INFO.
--

--- Patch [1]: [PATCH net-next] mptcp: normalize seq numbers reported in mptcp_info ---
commit 6231029f4c06cd4480ab1f97fc9151acd4862390
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

mptcp: normalize seq numbers reported in mptcp_info

This commit normalizes the sequence numbers reported by mptcp_info.
It caches the local and remote initial sequence numbers and subtracts them
in mptcp_diag_fill_info() so the diagnostic fields start near 0.

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..0468099b19bfc 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
[ ... ]
> @@ -1010,7 +1010,7 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	slow = lock_sock_fast(sk);
>  	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 - msk->local_idsn;
>  	info->mptcpi_retransmits = inet_csk(sk)->icsk_retransmits;
>  	info->mptcpi_bytes_sent = msk->bytes_sent;
>  	info->mptcpi_bytes_received = msk->bytes_received;
[ ... ]
> @@ -1024,8 +1024,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  
>  	mptcp_data_lock(sk);
>  	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 - msk->local_idsn;
> +	info->mptcpi_rcv_nxt = msk->ack_seq - msk->remote_idsn;
>  	info->mptcpi_bytes_acked = msk->bytes_acked;
>  	mptcp_data_unlock(sk);
>  }
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index af81ad5e699d1..4fa19835c4055 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -465,6 +465,7 @@ void __mptcp_sync_state(struct sock *sk, int state)
>  		 * even for the FASTOPEN scenarios
>  		 */
>  		WRITE_ONCE(msk->write_seq, subflow->idsn + 1);
> +		WRITE_ONCE(msk->local_idsn, subflow->idsn);
>  		WRITE_ONCE(msk->snd_nxt, msk->write_seq);
>  		mptcp_set_state(sk, state);
>  		sk->sk_state_change(sk);
[ ... ]
> @@ -491,6 +492,7 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  
>  	WRITE_ONCE(msk->remote_key, subflow->remote_key);
>  	WRITE_ONCE(msk->ack_seq, subflow->iasn);
> +	WRITE_ONCE(msk->remote_idsn, subflow->iasn);

[Severity: Medium]
Will this result in an asymmetric baseline where mptcpi_write_seq and
mptcpi_snd_una start at 1, while mptcpi_rcv_nxt starts at 0?

Looking at subflow_set_remote_key(), subflow->iasn is incremented before it is
assigned to msk->remote_idsn:

	mptcp_crypto_key_sha(subflow->remote_key, NULL, &subflow->iasn);
	subflow->iasn++;

However, in __mptcp_sync_state(), msk->local_idsn is assigned the unincremented
subflow->idsn:

	WRITE_ONCE(msk->write_seq, subflow->idsn + 1);
	WRITE_ONCE(msk->local_idsn, subflow->idsn);

When these are subtracted in mptcp_diag_fill_info(), msk->write_seq (which is
subflow->idsn + 1) minus msk->local_idsn results in 1, while msk->ack_seq minus
msk->remote_idsn results in 0.

Should msk->local_idsn and msk->remote_idsn both use either the incremented or
unincremented hash to ensure the normalized sequences start at the same value?

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