Re: [PATCH net-next v4 1/2] net: stmmac: fix TX coalesce race and div-by-zero in XDP/legacy path

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the stmmac_xdp_xmit_xdpf() divide-by-
  zero is "only theoretical", but a privileged user can trigger it and…

Pre-existing issues:
- [Low] __stmmac_set_coalesce() publishes tx_coal_frames[q] and
  tx_coal_timer[q] as two separate stores and shares no synchronisation…

--

net: stmmac: fix TX coalesce race and div-by-zero in XDP/legacy path

This patch wraps every access to tx_coal_frames[] in READ_ONCE() or
WRITE_ONCE(), since ethtool writes the array while the TX fast paths read
it without a lock. It also guards the modulo in stmmac_xdp_xmit_xdpf()
against a zero tx_coal_frames[queue].

> Both issues were found by code inspection while extending the XDP TX
> path with multi-buff support, and neither has ever been triggered, so
> they are only theoretical. Tested on the Qualcomm RB3-gen2 board.

[Severity: Medium]

Is the division by zero in stmmac_xdp_xmit_xdpf() really only
theoretical?

__stmmac_set_coalesce() only rejects the case where both values are zero:

	if ((ec->tx_coalesce_usecs == 0) &&
	    (ec->tx_max_coalesced_frames == 0))
		return -EINVAL;

So "ethtool -C ethX tx-frames 0" is accepted. tx-usecs keeps its default
of STMMAC_COAL_TX_TIMER, and stmmac_ethtool_ops advertises
ETHTOOL_COALESCE_MAX_FRAMES. After that, tx_coal_frames[queue] is 0.

>From then on, before this patch, every XDP_TX verdict
(stmmac_xdp_xmit_back()->stmmac_xdp_xmit_xdpf()) and every ndo_xdp_xmit
redirect into the device would evaluate:

	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)

On x86, for example the dwmac-intel platforms that advertise XDP
basic and redirect, this raises a divide error in NAPI softirq context.

On arm64, UDIV with a zero divisor returns 0 instead of trapping. That
may be why the RB3-gen2 board never hit it.

CAP_NET_ADMIN can trigger this with standard tools. Could the commit
message describe it as a reachable crash rather than a theoretical one?
Would it also make sense to send the div-by-zero fix separately from the
READ_ONCE()/WRITE_ONCE() annotations?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 1cf0f8820b337..30e0d499326d8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c

[ ... ]

> @@ -887,14 +888,14 @@ static int __stmmac_set_coalesce(struct net_device *dev,
>  		int i;
>  
>  		for (i = 0; i < tx_cnt; i++) {
> -			priv->tx_coal_frames[i] =
> -				ec->tx_max_coalesced_frames;
> +			WRITE_ONCE(priv->tx_coal_frames[i],
> +				   ec->tx_max_coalesced_frames);
>  			priv->tx_coal_timer[i] =
>  				ec->tx_coalesce_usecs;
>  		}
>  	} else if (queue < tx_cnt) {
> -		priv->tx_coal_frames[queue] =
> -			ec->tx_max_coalesced_frames;
> +		WRITE_ONCE(priv->tx_coal_frames[queue],
> +			   ec->tx_max_coalesced_frames);
>  		priv->tx_coal_timer[queue] =
>  			ec->tx_coalesce_usecs;
>  	}

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch.
tx_coal_frames[] and tx_coal_timer[] are still published as two separate
stores, with no synchronisation against the TX path.

Can a concurrent stmmac_xmit() see a mixed pair? Take a change from
(frames=N, usecs=0) to (frames=0, usecs=U). The TX path could read the
new frames value:

	tx_coal = READ_ONCE(priv->tx_coal_frames[queue]);
	...
	else if (!tx_coal)
		set_ic = false;

It could then read the old timer value in stmmac_tx_timer_arm():

	u32 tx_coal_timer = priv->tx_coal_timer[queue];

	if (!tx_coal_timer)
		return;

That frame would then get neither an IC interrupt nor the completion
timer. Its completion would wait for the next TX or NAPI activity on that
queue. The same window exists for stmmac_tso_xmit() and
stmmac_xdp_xmit_xdpf().

READ_ONCE()/WRITE_ONCE() make each field consistent on its own, which is
all the commit message claims. The window across the two fields is still
there.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-b4-stmmac-xmit-mb-v4-0-664e0c3eb289%40oss.qualcomm.com