Re: [PATCH net-next v8] 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:
- [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