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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6d4f0ef0-3c6b-417e-82e1-d7f2635f6733@lunn.ch>
Date: Mon, 27 Oct 2025 13:14:42 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Yangfl <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org,
	Dan Carpenter <dan.carpenter@...aro.org>,
	David Laight <david.laight.linux@...il.com>
Subject: Re: [PATCH net-next v2 1/2] net: dsa: yt921x: Fix MIB overflow
 wraparound routine

> > > diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> > > index ab762ffc4661..97a7eeb4ea15 100644
> > > --- a/drivers/net/dsa/yt921x.c
> > > +++ b/drivers/net/dsa/yt921x.c
> > > @@ -687,21 +687,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;
> > > +             u64 val;
> > >               u32 val0;
> > > -             u32 val1;
> > >
> > >               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;
> > > +
> >
> > What David suggested, https://lore.kernel.org/all/20251024132117.43f39504@pumpkin/ was
> >
> >                 if (desc->size <= 1) {
> >                         u64 old_val = *valp;
> >                         val = upper32_bits(old_val) | val0;
> >                         if (val < old_val)
> >                                 val += 1ull << 32;
> >                 }
> >
> > I believe there is a minor typo here, it should be upper_32_bits(),
> > but what you implemented is not really what David suggested.
> >
> >         Andrew
> 
> I didn't find the definition for upper32_bits, so...

You should of asked, or searched a bit harder, because what you
changed it to is different.

/**
 * upper_32_bits - return bits 32-63 of a number
 * @n: the number we're accessing
 *
 * A basic shift-right of a 64- or 32-bit quantity.  Use this to suppress
 * the "right shift count >= width of type" warning when that quantity is
 * 32-bits.
 */
#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))

I don't see any shifting in your version.

And then i have to ask, which is correct?

How have you been testing this code? If this is TX bytes, for a 1G
link, it will overflow 32 bits in about 34 seconds. So a simple iperf
test could be used. If its TX packets, 64 byte packets could be done
in 5 hours.

	Andrew