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

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

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

__stmmac_set_coalesce() applies the RX part of the request first and
only afterwards checks the TX parameters. The RX path already calls
stmmac_rx_watchdog() and stores rx_riwt[] and rx_coal_frames[], so when
the TX check rejects the request the driver returns -EINVAL after having
silently changed the hardware. A following ethtool -c then reports the
new RX values even though the command failed.

This became easy to hit once the per-queue interface was added.
__stmmac_get_coalesce() reports tx-usecs and tx-frames as 0 for a queue
index that is RX-only, and ethtool applies per-queue coalesce by reading
the current values first and sending them straight back. The next set is
therefore guaranteed to trip the test for both TX fields being zero,
right after the RX watchdog has been reprogrammed.

Move both TX checks in front of the RX block so a request is either
applied completely or rejected without touching the device.

Fixes: db2f2842e6f5 ("net: stmmac: add per-queue TX & RX coalesce ethtool support")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
v2:
- Trim the comment in front of the TX checks to one line; the reasoning
  belongs in the changelog. (Andrew Lunn)

 .../ethernet/stmicro/stmmac/stmmac_ethtool.c   | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 154cc0c7623d..e06d05c3b4c7 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;
+
+	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,
 		}
 	}
 
-	if ((ec->tx_coalesce_usecs == 0) &&
-	    (ec->tx_max_coalesced_frames == 0))
-		return -EINVAL;
-
-	if ((ec->tx_coalesce_usecs > STMMAC_MAX_COAL_TX_TICK) ||
-	    (ec->tx_max_coalesced_frames > STMMAC_TX_MAX_FRAMES))
-		return -EINVAL;
-
 	if (all_queues) {
 		int i;
 
-- 
2.25.1

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

[netdev-bot+sashiko]

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

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

[Linkui Xiao]

 > Thank you for your contribution! Sashiko AI review found 1 potential
 > issue(s) to consider.
 >
 > Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

Both issues are legitimate, thanks for the detailed analysis. v3 reworks
the fix and adds a second patch; details inline.

 > - [High] Incomplete fix: the TX validation relocated to the top of
 >   __stmmac_set_coalesce()...
 >
 > Does this reordering actually fix the per-queue case described in the
 > changelog? [...]
 > 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?

Yes, it still does. tgl_common_data() sets rx_queues_to_use = 6 and
tx_queues_to_use = 4, __stmmac_get_coalesce() reports tx-usecs and
tx-frames as 0 for index 4 and 5, ethtool feeds those zeros straight
back into the setter and the relocated test rejects the request. v2
changed when such a request fails, not whether it succeeds.

 > 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. [...]
 > 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?

Correct on both counts, and the roll-back one is the more serious of the
two: ethtool_set_per_queue_coalesce() discards the return code of the
replay, so an early -EINVAL there silently leaves the RX watchdog of an
already-applied queue at the value of the failed request.

 > 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?

That is what v3 patch 1/2 does, minus the getter change:

	has_rx = all_queues || queue < rx_cnt;
	has_tx = all_queues || queue < tx_cnt;

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

	if (has_tx &&
	    (ec->tx_coalesce_usecs > STMMAC_MAX_COAL_TX_TICK ||
	     ec->tx_max_coalesced_frames > STMMAC_TX_MAX_FRAMES))
		return -EINVAL;

	if (has_rx && priv->use_riwt) {

has_rx also gates the application, so the "else if (queue < rx_cnt)"
inside the RIWT block becomes a plain "else", and the TX store keeps its
own has_tx test. All validation still runs before the first
stmmac_rx_watchdog() call, so the property v2 was after is retained: a
request is either applied completely or rejected without touching the
device.

The mirrored case is fixed by the same change. With use_riwt set and
queue >= rx_cnt, has_rx is false, so stmmac_usec2riwt() is never asked
to convert the rx-usecs 0 that the getter reported and the MIN_DMA_RIWT
test no longer rejects per-queue TX requests for a TX-only index.
tx_queues_to_use > rx_queues_to_use does not occur in the in-tree
platform data - dwmac-intel is the other way round - but
stmmac_probe_config_dt() parses snps,rx-queues-to-use and
snps,tx-queues-to-use independently and clamps each one on its own, so
DT can produce it.

I left __stmmac_get_coalesce() reporting 0 rather than ~0. Once the
setter ignores the inapplicable fields the read-modify-write round trip
works either way, and ~0 would show up as 4294967295 in
"ethtool --per-queue ... --show-coalesce" for the indices that print 0
today. If the ~0 convention is wanted it should be a separate change,
together with whatever displays it.

 > [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? [...]
 > 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 divide by zero is real and reachable. tx_coal_frames[] starts at
STMMAC_TX_FRAMES in stmmac_init_coalesce() and can only become 0
through ethtool, and dwmac-intel TGL/EHL is exactly the x86 platform
where the modulo traps.

The fix belongs in stmmac_xdp_xmit_xdpf() rather than in the setter,
because tx-frames 0 is a supported request: it only stops the frame
count from raising TX completion interrupts, while the coalescing timer
that stmmac_xmit(), stmmac_tso_xmit(), stmmac_finalize_xdp_rx() and
stmmac_xdp_xmit() arm through stmmac_tx_timer_arm() keeps reclaiming the
descriptors. That is why those paths test the divisor - the test came
with c2837423cb54 ("net: stmmac: Rework TX Coalesce logic") and was
extended to the zero-copy path by 132c32ee5bc0 ("net: stmmac: Add TX via
XDP zero-copy socket") - and why __stmmac_set_coalesce() rejects the
request only when tx-usecs is zero as well, since then nothing would
complete the transmissions. Rejecting tx-frames 0 outright would break
that configuration.

So v3 adds patch 2/2, giving stmmac_xdp_xmit_xdpf() the missing test,
with be8b38a722e6 ("net: stmmac: Add support for XDP_TX action") as the
Fixes tag.

 > 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 [...]
 > 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?

In principle yes: nothing forces the two loads to observe the same
value. They are adjacent in all four chains with no call or barrier in
between, so compilers are likely to CSE them, but that is a favour and
not a guarantee. Annotating it properly means one snapshot per transmit
path plus WRITE_ONCE() on the stores in __stmmac_set_coalesce(), i.e. a
conversion of all four sites at once, which does not belong in a stable
fix. Patch 2/2 therefore uses the plain test that matches the existing
three; I will send the READ_ONCE()/WRITE_ONCE() conversion separately if
it is wanted.

pw-bot: cr