Re: [PATCH net-next v7] 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:
- [Critical] mptcp: breaks UAPI backward compatibility for sequence numbers
- [Critical] mptcp: use-after-free in mptcp_diag_fill_info when accessing subflow context
--

commit d5f0de8b3b4a150c893d97d9c69889501c979b26
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

    mptcp: normalize seq numbers reported in mptcp_info

This commit normalizes the MPTCP sequence numbers reported via mptcp_info
by subtracting the local and remote initial sequence numbers. It updates
mptcpi_write_seq, mptcpi_snd_una, and mptcpi_rcv_nxt to provide relative
values instead of absolute on-the-wire 64-bit sequence numbers. The
normalization logic accesses the connection's first subflow context to fetch
these 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..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_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;

[Severity: Critical]
Does this change introduce a UAPI regression?

Since this modifies UAPI fields (mptcpi_write_seq, mptcpi_snd_una, and
mptcpi_rcv_nxt) to use a relative offset instead of the absolute sequence
number, it might permanently break existing userspace tools (like tcpdump) that
rely on absolute on-the-wire values to correlate with packet captures.

>  	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);

[Severity: Critical]
Does taking a reference on the socket's sk_refcnt earlier actually protect the
subflow ULP context across this lock gap?

When we drop the socket lock here via unlock_sock_fast(), could a concurrent
mptcp_close() acquire the lock and call __tcp_close(), which unconditionally
executes tcp_cleanup_ulp()?

If tcp_cleanup_ulp() runs, it frees the ULP context via kfree_rcu() regardless
of the elevated sk_refcnt. Since unlock_sock_fast() can sleep and we are not
in an RCU read-side critical section, the RCU grace period might elapse before
we re-acquire the lock below.

>  
>  	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;

Could this regression result in a use-after-free when reading subflow->iasn if
the ULP context was freed during the lock gap?

> +	}
> +
>  	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/20260930102804.2796208-1-kalpan.jani@mpiricsoftware.com?part=1