Re: [PATCH mptcp-next v3 4/4] mptcp: sched: add penalise counters and tracepoint

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `mptcp_subflow_penalise` tracepoint logs an inaccurate verdict because it is emitted before the scheduler finalizes the decision to penalize a subflow.
- [Medium] Unprotected lockless plain reads and writes of TCP and subflow state violate concurrency rules, which will trigger KCSAN warnings.
--

--- Patch [4]: [PATCH mptcp-next v3 4/4] mptcp: sched: add penalise counters and tracepoint ---
commit 5337879700eb732d117ee91fe901b615a12833e3
Author: Shardul Bankar <[email protected]>

mptcp: sched: add penalise counters and tracepoint

Add observability for the cwnd-halving penalty introduced in the previous
patches. It adds MPTcpExt SNMP counters and an mptcp_subflow_penalise
tracepoint.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e4d26c2dce1ee..0cd18705ab404 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1656,6 +1656,7 @@ static void mptcp_penalise_cwnd(struct sock *ssk)
>  		return;
>  	subflow->last_penalise = tcp_jiffies32;

[Severity: Medium]
Is this unprotected write safe from load tearing?

Since subflow->last_penalise is concurrently read locklessly in
mptcp_penalise_throttle_ok(), should this be updated using WRITE_ONCE()
to avoid KCSAN warnings?

> +	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_CWNDPENALISED);
>  	if (cwnd >= tp->snd_ssthresh)
>  		tp->snd_ssthresh = max_t(u32, tp->snd_ssthresh >> 1, 2);
>  }

[ ... ]

> @@ -1739,6 +1740,8 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  	subflow = mptcp_subflow_ctx(ssk);
>  	penal_cand = fastest && ssk != fastest &&
>  		     subflow->avg_pacing_rate < max_pace / MPTCP_PENALISE_RATE_RATIO;
> +	if (penal_cand)
> +		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_PENALCAND);

[Severity: Medium]
Does this block trigger KCSAN data race warnings?

These lockless plain reads of TCP socket fields (tcp_snd_cwnd, icsk_ca_state,
tcp_is_cwnd_limited) execute under the MPTCP connection lock, while softirqs
may concurrently process incoming ACKs and update the states.

Do these need data_race() or READ_ONCE() annotations to meet kernel
concurrency standards?

>  	subflow->penalise = penal_cand &&
>  			    tcp_snd_cwnd(tcp_sk(ssk)) > MPTCP_PENALISE_MIN_CWND &&
>  			    inet_csk(ssk)->icsk_ca_state == TCP_CA_Open &&
> @@ -1746,6 +1749,9 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  			    mptcp_snd_wnd_test(msk) &&
>  			    mptcp_penalise_throttle_ok(subflow);
>  
> +	/* trace the penalise verdict, decided here after the selection loop */
> +	trace_mptcp_subflow_penalise(subflow, max_pace);
> +

[Severity: Medium]
Will this tracepoint log an inaccurate verdict?

If there is no window room to send (!burst), the code immediately below
sets subflow->penalise = false.

Since the tracepoint is emitted before this window limitation check, could
it log penalise=1 even when the penalty is later canceled?

>  	burst = min(MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - msk->snd_nxt);
>  	wmem = READ_ONCE(ssk->sk_wmem_queued);
>  	if (!burst) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-mptcp_penalise_send_v2-v3-0-e6a2ad2f1b82@mpiricsoftware.com?part=4