linux-kernel - Re: [PATCH net-next v6 3/4] net: macb: Add ARP support to WOL

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Thu, 20 Jun 2024 17:41:43 +0100
From: Simon Horman <horms@...nel.org>
To: "Karumanchi, Vineeth" <vineeth.karumanchi@....com>
Cc: nicolas.ferre@...rochip.com, claudiu.beznea@...on.dev,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	linux@...linux.org.uk, vadim.fedorenko@...ux.dev, andrew@...n.ch,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, git@....com
Subject: Re: [PATCH net-next v6 3/4] net: macb: Add ARP support to WOL

Hi Vineeth,

On Thu, Jun 20, 2024 at 09:29:01PM +0530, Karumanchi, Vineeth wrote:
> Hi Simon,
> 
> On 6/18/2024 4:26 PM, Simon Horman wrote:
> > On Mon, Jun 17, 2024 at 12:34:12PM +0530, Vineeth Karumanchi wrote:

...

> > > @@ -5290,6 +5289,14 @@ static int __maybe_unused macb_suspend(struct device *dev)
> > >   		macb_writel(bp, TSR, -1);
> > >   		macb_writel(bp, RSR, -1);
> > > +		tmp = (bp->wolopts & WAKE_MAGIC) ? MACB_BIT(MAG) : 0;
> > > +		if (bp->wolopts & WAKE_ARP) {
> > > +			tmp |= MACB_BIT(ARP);
> > > +			/* write IP address into register */
> > > +			tmp |= MACB_BFEXT(IP,
> > > +					 (__force u32)(cpu_to_be32p((uint32_t *)&ifa->ifa_local)));
> > 
> > Hi Vineeth and Harini,
> > 
> > I guess I must be reading this wrong, beause I am confused
> > by the intent of the endeness handling above.
> > 
> > * ifa->ifa_local is a 32-bit big-endian value
> > 
> > * It's address is cast to a 32-bit host-endian pointer
> > 
> >    nit: I think u32 would be preferable to uint32_t; this is kernel code.
> > 
> > * The value at this address is then converted to a host byte order value.
> > 
> >    nit: Why is cpu_to_be32p() used here instead of the more commonly used
> >         cpu_to_be32() ?
> > 
> >    More importantly, why is a host byte order value being converted from
> >    big-endian to host byte order?
> > 
> > * The value returned by cpu_to_be32p, which is big-endian, because
> >    that is what that function does, is then cast to host-byte order.
> > 
> > 
> > So overall we have:
> > 
> > 1. Cast from big endian to host byte order
> > 2. Conversion from host byte order to big endian
> >     (a bytes-swap on litte endian hosts; no-op on big endian hosts)
> > 3. Cast from big endian to host byte oder
> > 
> > All three of these steps seem to warrant explanation.
> > And the combination is confusing to say the least.
> > 
> 
> tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
> 
> The above snippet will address above points.
> Consider the ip address is : 11.11.70.78
> 
> 1. ifa->ifa_local : returns be32 -> 0x4E460b0b
> 2. be32_to_cpu(ifa->ifa_local) : converts be32 to host byte order u32:
> 0x0b0b464e
> 
> There are no sparse errors as well.
> I will make the change, please let me know your suggestions/thoughts.

Thanks for your response, your proposal looks good to me.