Re: [PATCH mptcp-next v4 4/5] mptcp: track app-limited state in mptcp_sendmsg

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mptcp: mptcp_rate_check_app_limited() bails out when data is in flight
- [Medium] mptcp: unlocked msk->snd_una read in mptcp_rate_check_app_limited()
--

commit 6075a59b5fc2bd09596bbf554a3f811da115a224
Author: Geliang Tang <geliang@kernel.org>

mptcp: track app-limited state in mptcp_sendmsg

This commit adds mptcp_rate_check_app_limited() to run
tcp_rate_check_app_limited() across all subflows from mptcp_sendmsg_locked()
when no unsent data is queued at the MPTCP level.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7ccb1c7506a37..ab7de11a448ce 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2038,6 +2038,25 @@ static void mptcp_rps_record_subflows(const struct mptcp_sock *msk)
>  	}
>  }
>  
> +static void mptcp_rate_check_app_limited(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct mptcp_subflow_context *subflow;
> +
> +	if (msk->write_seq != msk->snd_nxt ||
> +	    mptcp_send_head(sk) || mptcp_rtx_head(sk))
> +		return;

[Severity: Medium]
Does checking mptcp_rtx_head(sk) here prevent subflows from being marked
application-limited whenever any sent data is still in flight?

Every sent mptcp_data_frag stays on msk->rtx_queue until a DATA_ACK advances
msk->snd_una past it and __mptcp_clean_una() removes it, so mptcp_rtx_head()
returns non-NULL whenever msk->snd_una != msk->snd_nxt:

net/mptcp/protocol.h:mptcp_rtx_head() {
	struct mptcp_sock *msk = mptcp_sk(sk);

	if (msk->snd_una == msk->snd_nxt)
		return NULL;

	return list_first_entry_or_null(&msk->rtx_queue, struct mptcp_data_frag, list);
}

In contrast, tcp_rate_check_app_limited() is designed to run while packets are
in flight below cwnd (tcp_packets_in_flight(tp) < tcp_snd_cwnd(tp)) and
already checks tp->lost_out <= tp->retrans_out on each subflow, while
MPTCP-level reinjection in __mptcp_retransmit_pending_data() resets
msk->first_pending (which is covered by mptcp_send_head(sk)). With the
mptcp_rtx_head(sk) check in place, won't mptcp_rate_check_app_limited() bail
out whenever any unacknowledged data is in flight on the connection?

[Severity: Medium]
Additionally, is it safe to call mptcp_rtx_head(sk) from
mptcp_sendmsg_locked() -> mptcp_rate_check_app_limited() without holding
mptcp_data_lock(sk)?

mptcp_rtx_head() performs a plain 64-bit read of msk->snd_una, whereas
incoming DATA_ACK processing in softirq context updates msk->snd_una under
mptcp_data_lock(sk) even while the user owns the msk socket lock:

mptcp_incoming_options()
  ack_update_msk()
    mptcp_data_lock(sk)
    __mptcp_snd_una_update()
      WRITE_ONCE(msk->snd_una, new_snd_una);

Other callers of mptcp_rtx_head() (__mptcp_clean_una(),
__mptcp_retransmit_pending_data(), and __mptcp_retrans()) hold
mptcp_data_lock(sk), and helpers that read msk->snd_una under only the msk
socket lock use READ_ONCE(msk->snd_una). Can this cause a data race or 64-bit
load tearing on 32-bit architectures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793588257.git.tanggeliang@kylinos.cn?part=4