Re: [PATCH net v2] net: stmmac: ethtool: validate TX coalesce before reprogramming RX

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · 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).

- [High] Incomplete fix: the TX validation relocated to the top of
  __stmmac_set_coalesce()…

Pre-existing issues:
- [High] __stmmac_set_coalesce() accepts tx_max_coalesced_frames == 0 as
  long as tx_coalesce_usecs != 0 (the check at stmmac_ethtool.c:855 only…

--

net: stmmac: ethtool: validate TX coalesce before reprogramming RX

The two TX parameter checks in __stmmac_set_coalesce() are moved ahead of the
RX block so that a request rejected because of its TX values no longer
reprograms the RX watchdog and rx_riwt[]/rx_coal_frames[] first.  The
changelog motivates this with the per-queue path, where ethtool reads the
current values back and sends them straight in again.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 1be5310ca766..193b65f9650f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -850,6 +850,16 @@ static int __stmmac_set_coalesce(struct net_device *dev,
>  	else if (queue >= max_cnt)
>  		return -EINVAL;
>  
> +	/* Check TX parameters before applying any RX setting */
> +
> +	if (ec->tx_coalesce_usecs == 0 &&
> +	    ec->tx_max_coalesced_frames == 0)
> +		return -EINVAL;

[Severity: High]
Does this reordering actually fix the per-queue case described in the
changelog?  The relocated check runs for every queue index, but the TX apply
block further down is gated on the queue having a TX ring:

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

and __stmmac_get_coalesce() deliberately reports zeros for an index that has
no TX ring:

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

On a device with rx_queues_to_use > tx_queues_to_use (dwmac-intel
tgl_common_data sets 6 RX and 4 TX queues, so queues 4 and 5 are RX-only),
does ethtool -Q ethX queue_mask 0x10 --coalesce rx-usecs N still return
-EINVAL here?

If so, is the net effect for that queue not worse than before?  Previously
the RX watchdog and rx_riwt[]/rx_coal_frames[] were programmed and -EINVAL
was returned; now the early return means the RX part is never applied, so
per-queue RX coalescing on an RX-only queue index has no effect at all.

There is a second consequence in the ethtool core.
ethtool_set_per_queue_coalesce() snapshots the getter output per queue and
replays it on failure, discarding the return code:

net/ethtool/ioctl.c:ethtool_set_per_queue_coalesce() {
	...
roll_back:
	if (ret != 0) {
		tmp = backup;
		for_each_set_bit(i, queue_mask, bit) {
			dev->ethtool_ops->set_per_queue_coalesce(dev, i, tmp);
			tmp++;
		}
	}
	...
}

For an RX-only queue the saved backup necessarily carries tx_coalesce_usecs
== 0 and tx_max_coalesced_frames == 0, so the new first check rejects the
replay before the RX block runs.  Can the RX watchdog and rx_riwt[] of that
queue therefore stay at the values of the failed request after a later queue
in the mask fails?

> +
> +	if (ec->tx_coalesce_usecs > STMMAC_MAX_COAL_TX_TICK ||
> +	    ec->tx_max_coalesced_frames > STMMAC_TX_MAX_FRAMES)
> +		return -EINVAL;
> +
>  	if (priv->use_riwt) {
>  		rx_riwt = stmmac_usec2riwt(ec->rx_coalesce_usecs, priv);
>  
> @@ -875,14 +885,6 @@ static int __stmmac_set_coalesce(struct net_device *dev,

The mirror of the same problem is left in place for a TX-only index.  With
priv->use_riwt set and queue >= rx_cnt, the getter reports
rx_coalesce_usecs = 0, stmmac_usec2riwt() returns 0 and

		if ((rx_riwt > MAX_DMA_RIWT) || (rx_riwt < MIN_DMA_RIWT))
			return -EINVAL;

rejects the request before any TX value is stored.

include/linux/ethtool.h documents both halves of the contract:

 * @get_per_queue_coalesce: ... If only a RX queue or a TX
 *	queue has this number, set the inapplicable fields to ~0 and return 0.
 * @set_per_queue_coalesce: ... If only a RX queue or a TX
 *	queue has this number, ignore the inapplicable fields.

Would making the validation direction-aware, so the TX checks only apply when
all_queues || queue < tx_cnt and the rx_riwt check only when
all_queues || queue < rx_cnt (plus the getter reporting ~0 instead of 0 for
the inapplicable fields), address both directions rather than reordering
alone?

[Severity: High]
This isn't a bug introduced by this patch, but since the validation is being
moved anyway, is the zero tx_max_coalesced_frames case worth handling here
too?  The check above only rejects both TX fields being zero, so
ethtool -C ethX tx-usecs 10 tx-frames 0 stores priv->tx_coal_frames[queue]
== 0, and stmmac_xdp_xmit_xdpf() divides by it with no zero test:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_xdp_xmit_xdpf() {
	...
	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
		set_ic = true;
	...
}

That is reached from stmmac_xdp_xmit_back() for XDP_TX and from
stmmac_xdp_xmit() for ndo_xdp_xmit, so does this divide by zero in softirq
context on x86?

The other TX sites do test for zero, but the guard and the divisor are
separate unannotated loads of the same plain u32 with no lock, for example in
stmmac_xmit() and stmmac_tso_xmit():

	else if (!priv->tx_coal_frames[queue])
		set_ic = false;
	...
	else if ((tx_q->tx_count_frames %
		  priv->tx_coal_frames[queue]) < tx_packets)

and in stmmac_xdp_xmit_zc():

		if (!priv->tx_coal_frames[queue])
			set_ic = false;
		else if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)

Can a concurrent ethtool store of 0 land between the guard and the modulo
reload?  Would a single READ_ONCE() snapshot per transmit path be
appropriate?

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