[PATCH net-next v4 1/2] net: dsa: yt921x: Fix MIB overflow wraparound routine

David Yang posted 2 patches 1 month ago
There is a newer version of this series
[PATCH net-next v4 1/2] net: dsa: yt921x: Fix MIB overflow wraparound routine
Posted by David Yang 1 month ago
Reported by the following Smatch static checker warning:

  drivers/net/dsa/yt921x.c:702 yt921x_read_mib()
  warn: was expecting a 64 bit value instead of '(~0)'

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/netdev/aPsjYKQMzpY0nSXm@stanley.mountain/
Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/yt921x.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
index 0b3df732c0d1..5e4e8093ba16 100644
--- a/drivers/net/dsa/yt921x.c
+++ b/drivers/net/dsa/yt921x.c
@@ -682,21 +682,22 @@ static int yt921x_read_mib(struct yt921x_priv *priv, int port)
 		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
 		u32 reg = YT921X_MIBn_DATA0(port) + desc->offset;
 		u64 *valp = &((u64 *)mib)[i];
-		u64 val = *valp;
 		u32 val0;
-		u32 val1;
+		u64 val;
 
 		res = yt921x_reg_read(priv, reg, &val0);
 		if (res)
 			break;
 
 		if (desc->size <= 1) {
-			if (val < (u32)val)
-				/* overflow */
-				val += (u64)U32_MAX + 1;
-			val &= ~U32_MAX;
-			val |= val0;
+			u64 old_val = *valp;
+
+			val = (old_val & ~(u64)U32_MAX) | val0;
+			if (val < old_val)
+				val += 1ull << 32;
 		} else {
+			u32 val1;
+
 			res = yt921x_reg_read(priv, reg + 4, &val1);
 			if (res)
 				break;
-- 
2.51.0
Re: [net-next,v4,1/2] net: dsa: yt921x: Fix MIB overflow wraparound routine
Posted by Jakub Kicinski 1 month ago
This is an AI-generated review of your patch.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: yt921x: Use u64_stats_t for MIB stats

This patch converts MIB statistics from plain u64 to u64_stats_t and adds
u64_stats_sync protection for atomic statistics access on 32-bit systems.

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index 7b8c1549a0fb..3d87944830f6 100644

[ ... ]

> @@ -762,22 +779,27 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
>  	struct yt921x_priv *priv = to_yt921x_priv(ds);
>  	struct yt921x_port *pp = &priv->ports[port];
>  	struct yt921x_mib *mib = &pp->mib;
> +	unsigned int start;
>  	size_t j;
>
>  	mutex_lock(&priv->reg_lock);
>  	yt921x_read_mib(priv, port);
>  	mutex_unlock(&priv->reg_lock);
>
> -	j = 0;
> -	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> -		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> +	do {
> +		start = u64_stats_fetch_begin(&pp->syncp);

This uses pp->syncp via u64_stats_fetch_begin(), but the syncp field does
not appear to be initialized with u64_stats_init() anywhere.

Looking at yt921x_mdio_probe(), the port initialization loop initializes
pp->index and pp->mib_read but not pp->syncp:

    for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
        struct yt921x_port *pp = &priv->ports[i];
        pp->index = i;
        INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
    }

On 32-bit systems, u64_stats_init() calls seqcount_init() which sets up
the lockdep class. Without this initialization, CONFIG_DEBUG_LOCK_ALLOC
builds may produce lockdep warnings.

For comparison, the xrs700x driver at drivers/net/dsa/xrs700x/xrs700x.c
initializes its syncp field with u64_stats_init(&p->syncp).

Should u64_stats_init(&pp->syncp) be added to the port initialization loop
in yt921x_mdio_probe()?

[ ... ]

> diff --git a/drivers/net/dsa/yt921x.h b/drivers/net/dsa/yt921x.h
> index 61bb0ab3b09a..17fee83952c2 100644
> --- a/drivers/net/dsa/yt921x.h
> +++ b/drivers/net/dsa/yt921x.h

[ ... ]

> @@ -533,9 +535,13 @@ struct yt921x_port {
>  	bool isolated;
>
>  	struct delayed_work mib_read;
> +	struct u64_stats_sync syncp;
>  	struct yt921x_mib mib;
> -	u64 rx_frames;
> -	u64 tx_frames;
> +	u64_stats_t rx_frames;
> +	u64_stats_t tx_frames;
> +
> +	/* only used by read routine to avoid huge allocations on the stack */
> +	struct yt921x_mib mib_new;
>  };

-- 
pw-bot: cr