Re: [RFC net-next v10] 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: stale remote_idsn after mptcp_disconnect() corrupts subflow map_seq
- [Medium] mptcp: torn local_idsn and snd_una snapshot in mptcp_diag_fill_info()
--

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

mptcp: normalize seq numbers reported in mptcp_info

This patch normalizes the sequence number fields in mptcp_info and
MPTCP_SUBFLOW_ATTR_MAP_SEQ relative to the initial data sequence numbers. It
also passes the requester capability state to idiag_get_info callbacks so that
sequence numbers in INET_DIAG_INFO are zeroed for unprivileged callers.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/445

[ ... ]

> diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c
> index 70cf9ebce8338..590cdee419ef9 100644
> --- a/net/mptcp/diag.c
> +++ b/net/mptcp/diag.c
> @@ -16,6 +16,7 @@ static int subflow_get_info(struct sock *sk, struct sk_buff *skb, bool net_admin
>  {
>  	struct mptcp_subflow_context *sf;
>  	struct nlattr *start;
> +	u64 map_seq;
>  	u32 flags = 0;
>  	bool slow;
>  	int err;
> @@ -35,6 +36,11 @@ static int subflow_get_info(struct sock *sk, struct sk_buff *skb, bool net_admin
>  		goto nla_failure;
>  	}
>  
> +	map_seq = 0;
> +	if (sf->conn && sf->map_seq)
> +		map_seq = sf->map_seq -
> +			  READ_ONCE(mptcp_sk(sf->conn)->remote_idsn);

[Severity: Medium]
Can this underflow map_seq if a connected MPTCP socket is disconnected via
mptcp_disconnect() and then reconnected with a fallback to TCP?

When an initial connection completes the MP_CAPABLE handshake,
subflow_set_remote_key() sets msk->remote_idsn and msk->can_ack = true. If the
socket is subsequently disconnected, mptcp_disconnect() resets msk->can_ack,
msk->ack_seq, and the byte counters, while __mptcp_subflow_disconnect() resets
the subflow context (zeroing subflow->iasn and subflow->map_seq), but
msk->remote_idsn (as well as msk->local_idsn, msk->write_seq, msk->snd_nxt,
and msk->snd_una) is left holding the previous connection's values:

net/mptcp/protocol.c:mptcp_disconnect() {
	...
	WRITE_ONCE(msk->can_ack, false);
	...
	/* for fallback's sake */
	WRITE_ONCE(msk->ack_seq, 0);
	atomic64_set(&msk->rcv_wnd_sent, 0);
}

If the socket is then reconnected via mptcp_connect() and falls back to plain
TCP before receiving a peer key (for example via mptcp_early_fallback() or
when the SYN-ACK lacks MP_CAPABLE in subflow_finish_connect()),
subflow_set_remote_key() is not called. When data arrives on the fallback
connection, subflow_check_data_avail() computes subflow->map_seq relative to
subflow->iasn == 0.

In mptcp_diag_fill_info(), reading msk->remote_idsn is guarded by
READ_ONCE(msk->can_ack), so mptcpi_rcv_nxt uses remote_idsn == 0. However,
subflow_get_info() does not check can_ack and subtracts the stale
msk->remote_idsn of the previous connection from the 0-based sf->map_seq.

Should mptcp_disconnect() reset the sequence and IDSN fields, and should
subflow_get_info() also gate remote_idsn on READ_ONCE(msk->can_ack)?

[ ... ]

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 922f6ae5c80cb..b20263b4cd55c 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -1047,6 +1047,7 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>  void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  {
>  	struct sock *sk = (struct sock *)msk;
> +	u64 local_idsn = 0, remote_idsn = 0;
>  	u32 flags = 0;
>  	bool slow;
>  	u32 now;
> @@ -1084,9 +1085,11 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	info->mptcpi_flags = flags;
>  
>  	slow = lock_sock_fast(sk);
> +	local_idsn = READ_ONCE(msk->local_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;
>  	info->mptcpi_retransmits = inet_csk(sk)->icsk_retransmits;
>  	info->mptcpi_bytes_sent = msk->bytes_sent;
>  	info->mptcpi_bytes_received = msk->bytes_received;
> @@ -1099,17 +1102,20 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
>  	unlock_sock_fast(sk, slow);
>  
>  	mptcp_data_lock(sk);
> +	if (READ_ONCE(msk->can_ack))
> +		remote_idsn = READ_ONCE(msk->remote_idsn);
>  	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;

[Severity: Medium]
Could mptcp_diag_fill_info() observe a torn pair of local_idsn and
msk->snd_una if it races with mptcp_connect()?

In mptcp_getsockopt(), the socket lock is released before calling
mptcp_getsockopt_sol_mptcp() -> mptcp_getsockopt_info() ->
mptcp_diag_fill_info(). In mptcp_diag_fill_info(), local_idsn is sampled under
lock_sock_fast(sk) and then the socket lock is released via
unlock_sock_fast(sk, slow) before acquiring mptcp_data_lock(sk).

If another thread concurrently calls connect() -> inet_stream_connect() ->
mptcp_connect() right after unlock_sock_fast(sk, slow), mptcp_connect()
initializes msk->local_idsn and msk->snd_una under lock_sock(sk) without
holding mptcp_data_lock(sk):

net/mptcp/protocol.c:mptcp_connect() {
	...
	WRITE_ONCE(msk->local_idsn, subflow->idsn);
	WRITE_ONCE(msk->write_seq, subflow->idsn);
	WRITE_ONCE(msk->snd_nxt, subflow->idsn);
	WRITE_ONCE(msk->snd_una, subflow->idsn);
}

When mptcp_diag_fill_info() then acquires mptcp_data_lock(sk), msk->snd_una
will be subflow->idsn while the previously sampled local_idsn is still 0,
causing info->mptcpi_snd_una to report the raw unnormalized subflow->idsn to
userspace.

Would it be safer to re-read READ_ONCE(msk->local_idsn) or sample msk->snd_una
using READ_ONCE() within the same critical section?

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