[PATCH net v3 2/2] net: stmmac: fix a divide by zero in stmmac_xdp_xmit_xdpf()

[netdev-bot+sashiko at kernel.org]

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

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

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 zero guard added to stmmac_xdp_xmit_xdpf() performs two
  separate, unsynchronized plain loads of priv->tx_coal_frames[queue]:…

Pre-existing issues:
- [Medium] With `ethtool -C ... tx-frames 0`, the new branch in
  stmmac_xdp_xmit_xdpf()…

--

net: stmmac: fix a divide by zero in stmmac_xdp_xmit_xdpf()

This adds the missing zero test on priv->tx_coal_frames[queue] before the
modulo in stmmac_xdp_xmit_xdpf(), so that "ethtool -C eth0 tx-usecs 10
tx-frames 0" followed by an XDP_TX or ndo_xdp_xmit frame no longer divides
by zero. The other transmit paths already have this test.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..f9846250458a4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5253,7 +5253,9 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
>  
>  	tx_q->tx_count_frames++;
>  
> -	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
> +	if (!priv->tx_coal_frames[queue])
> +		set_ic = false;
> +	else if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
>  		set_ic = true;
>  	else
>  		set_ic = false;

[Severity: Medium]
Is one sample of priv->tx_coal_frames[queue] needed here instead of two?

The guard and the divisor are two separate plain loads of the same
concurrently written field, with no barrier or lock between them, so the
compiler is free to reload it for the modulo.

The writer side in __stmmac_set_coalesce() (stmmac_ethtool.c) stores it with
plain stores from process context, with no driver lock, no queue stop and no
NAPI quiescing:

	if (all_queues) {
		int i;

		for (i = 0; i < tx_cnt; i++) {
			priv->tx_coal_frames[i] =
				ec->tx_max_coalesced_frames;
			priv->tx_coal_timer[i] =
				ec->tx_coalesce_usecs;
		}
	} else if (has_tx) {
		priv->tx_coal_frames[queue] =
			ec->tx_max_coalesced_frames;

The reader in stmmac_xdp_xmit_xdpf() runs in softirq, reached from
stmmac_rx()/stmmac_rx_zc() via stmmac_xdp_xmit_back(), or from
stmmac_xdp_xmit(). If the guard load observes the old non-zero value and the
divisor load observes the freshly stored 0, the modulo still divides by zero
in softirq, which is the failure this commit is closing.

Would something like this be more robust?

	u32 coal = READ_ONCE(priv->tx_coal_frames[queue]);

	set_ic = coal && (tx_q->tx_count_frames % coal == 0);

The same double-read shape exists in stmmac_xmit(), stmmac_tso_xmit() and
stmmac_xdp_xmit_zc(), so a READ_ONCE() there too would keep the paths
consistent.

[Severity: Medium]
This isn't a bug introduced by this patch, but the commit message states
that with tx-frames 0 "the coalescing timer armed by stmmac_tx_timer_arm()
keeps reclaiming the descriptors". Does that hold for the AF_XDP zero-copy
XDP_TX path?

stmmac_finalize_xdp_rx() is the only arming attempt on that path:

	if (xdp_status & STMMAC_XDP_TX)
		stmmac_tx_timer_arm(priv, queue);

and stmmac_tx_timer_arm() picks the rxtx NAPI for an xsk_pool-backed queue,
which is the instance currently polling:

	napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
	...
	if (unlikely(!napi_is_scheduled(napi))) {
		...hrtimer_start(...)
	} else {
		hrtimer_try_to_cancel(&tx_q->txtimer);
	}

so napi_is_scheduled() is true and the timer is cancelled rather than armed.

The end-of-poll fallback in stmmac_napi_poll_rxtx() does not cover it
either, since tx_pending_packets is sampled before the XDP_TX descriptors
are produced:

	tx_done = stmmac_tx_clean(priv, budget, chan, &tx_pending_packets);
	tx_done = min(tx_done, budget);

	rx_done = stmmac_rx_zc(priv, budget, chan);
	...
	if (tx_pending_packets)
		stmmac_tx_timer_arm(priv, chan);

With tx-frames 0 no descriptor carries the interrupt-on-completion bit
either, so tx_q->dirty_tx stays behind tx_q->cur_tx and the dma_map_single()
mappings and xdp_frames of the last batch are held until unrelated traffic
causes another poll on that channel.

This window is reachable today with the defaults, since stmmac_init_coalesce()
sets

		priv->tx_coal_frames[chan] = STMMAC_TX_FRAMES;

and a zero-copy XDP_TX burst shorter than 25 frames likewise leaves set_ic
false while the timer is cancelled, so the patch only widens an existing
gap rather than creating one. Would it be worth dropping or softening the
claim that the timer always keeps reclaiming, or arming the timer after
stmmac_rx_zc() in stmmac_napi_poll_rxtx()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921135716.218420-1-xiaolinkui%40126.com