Re: [PATCH net-next 4/4] net: dsa: motorcomm: Prevent rx_bytes/tx_bytes underflow

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 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] yt921x_counter_read() (drivers/net/dsa/motorcomm/smi.c)
  returns a false -EIO for a valid read.
- [Low] The commit fixes a user-visible bug: rtnl_link_stats64
  rx_bytes/tx_bytes (as shown by `ip -s link`) can read as about 2^64…

--

net: dsa: motorcomm: Prevent rx_bytes/tx_bytes underflow

This commit stops yt921x_mib_read() from publishing a partially refreshed
MIB buffer when a register read fails. It moves the rx_bytes/tx_bytes
derivation (good_bytes - ETH_FCS_LEN * frames) into the writer under
priv->reg_lock, and publishes the result only when check_sub_overflow()
reports no underflow. yt921x_dsa_get_stats64() then copies the published
values.

> diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
> index b45bc82117412..cb3912493cfb1 100644
> --- a/drivers/net/dsa/motorcomm/mib.c
> +++ b/drivers/net/dsa/motorcomm/mib.c

[ ... ]

> @@ -124,6 +126,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
>  		}
>  	}
>  
> +	if (res) {
> +		dev_err(dev, "Failed to %s port %d: %i\n", "read stats for",
> +			port, res);
> +		return res;
> +	}
> +

[Severity: Medium]
With this early return, any error from yt921x_counter_read() now throws
away the whole sample. Can yt921x_counter_read() return -EIO for a read
that is actually consistent?

drivers/net/dsa/motorcomm/smi.c:yt921x_counter_read() {
	...
	res = yt921x_reg_read(priv, reg, &old_lo);
	...
	res = yt921x_reg_read(priv, reg + 4, &hi);
	...
	res = yt921x_reg_read(priv, reg, &lo);
	...
	if (lo < old_lo) {
		u32 new_hi;

		res = yt921x_reg_read(priv, reg + 4, &new_hi);
		...
		if (new_hi != hi + 1)
			return -EIO;
		hi = new_hi;
	}
	...
}

Suppose the low word wraps after old_lo is read but before hi is read.
Then hi and lo are both post-wrap values and already form a valid 64-bit
counter. For example:

  old_lo = 0xfffffff0, hi = H + 1, lo = 0x10, new_hi = H + 1

In this case new_hi == hi, so the check fails and -EIO is returned.

The affected counters are the 64-bit rx_good_bytes, rx_bad_bytes and
tx_good_bytes. Their low words wrap about every 34 seconds at 1 Gb/s
line rate.

When this happens, yt921x_mib_read() logs "Failed to read stats for port
N: -5" and discards the sample. yt921x_mib_poll() then pushes the next
poll back from 3 to 12 seconds:

	if (res)
		delay *= 4;

The ethtool callbacks that ignore the return value would quietly report
the previous sample.

The reader came in with the earlier "net: dsa: motorcomm: Use safe 64-bit
counter reader" commit in this series, but the error only becomes visible
through this patch's discard path.

Could yt921x_counter_read() also accept new_hi == hi? Another option is a
hi/lo/hi2 sequence that re-reads lo when hi changes.

[ ... ]

> @@ -389,10 +400,8 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  
>  		stats->rx_packets = u64_stats_read(&pm->rx_frames);
>  		stats->tx_packets = u64_stats_read(&pm->tx_frames);
> -		stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) -
> -				  ETH_FCS_LEN * stats->rx_packets;
> -		stats->tx_bytes = u64_stats_read(&mib->tx_good_bytes) -
> -				  ETH_FCS_LEN * stats->tx_packets;
> +		stats->rx_bytes = u64_stats_read(&pm->rx_bytes);
> +		stats->tx_bytes = u64_stats_read(&pm->tx_bytes);

[Severity: Low]
This fixes rx_bytes/tx_bytes in rtnl_link_stats64 reading as about 2^64,
which users can see with ip -s link. Should the commit carry a Fixes: tag?

The unchecked subtraction dates back to the initial driver commit:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

Later, 25265e7ccdb5 ("net: dsa: motorcomm: Split MIB module") moved it,
and e7b23688d0d3 and ed4453587dc0 rewrote it, but none of them guarded
it against underflow. The driver has been in released kernels since late
2025.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107132040.1066996-1-mmyangfl%40gmail.com