| 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
| ||
|
Date: Fri, 1 Mar 2024 10:22:09 +0000
From: <Raju.Lakkaraju@...rochip.com>
To: <andrew@...n.ch>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
<linux-kernel@...r.kernel.org>, <Bryan.Whitehead@...rochip.com>,
<richardcochran@...il.com>, <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH net 3/3] net: lan743x: Address problems with wake option
flags configuration sequences
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Thursday, February 29, 2024 8:36 PM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@...rochip.com>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.org; linux-
> kernel@...r.kernel.org; Bryan Whitehead - C21958
> <Bryan.Whitehead@...rochip.com>; richardcochran@...il.com;
> UNGLinuxDriver <UNGLinuxDriver@...rochip.com>
> Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option
> flags configuration sequences
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Thu, Feb 29, 2024 at 08:59:20AM +0000, Raju.Lakkaraju@...rochip.com
> wrote:
> > Hi Andrew,
> >
> > Thank you for review comments.
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@...n.ch>
> > > Sent: Tuesday, February 27, 2024 7:28 AM
> > > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@...rochip.com>
> > > Cc: netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.org;
> > > linux- kernel@...r.kernel.org; Bryan Whitehead - C21958
> > > <Bryan.Whitehead@...rochip.com>; richardcochran@...il.com;
> > > UNGLinuxDriver <UNGLinuxDriver@...rochip.com>
> > > Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with
> > > wake option flags configuration sequences
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:
> > > > Wake options handling has been reworked as follows:
> > > > a. We only enable secure on magic packet when both secure and magic
> wol
> > > > options are requested together.
> > > > b. If secure-on magic packet had been previously enabled, and a
> > > subsequent
> > > > command does not include it, we add it. This was done to workaround
> a
> > > > problem with the 'pm-suspend' application which is unaware of
> secure-on
> > > > magic packet being enabled and can unintentionally disable it prior to
> > > > putting the system into suspend.
> > >
> > > This seems to be in a bit of a grey area. But ethtool says:
> > >
> > > wol p|u|m|b|a|g|s|f|d...
> > > Sets Wake-on-LAN options. Not all devices support this.
> > > The argument to this option is a string of characters speci‐
> > > fying which options to enable.
> > > p Wake on PHY activity
> > > u Wake on unicast messages
> > > m Wake on multicast messages
> > > b Wake on broadcast messages
> > > a Wake on ARP
> > > g Wake on MagicPacket™
> > > s Enable SecureOn™ password for MagicPacket™
> > > f Wake on filter(s)
> > > d Disable (wake on nothing). This option
> > > clears all previous options.
> > >
> > > d clears everything. All other things enable one option. There does
> > > not appear to be a way to disable a single option, and i would say,
> > > adding options is incremental.
> > >
> >
> > Yes. "d" clears everything.
> > But, if we enable "g" then enable "a", "g" option overwritten by "a"
>
> This is where i say it is a bit of a grey area. For me, they are incremental. You
> can enable a and then later enable g, and you should have both enabled.
>
Yes. But, it's "Ethtool" issue.
Even though ethtool get the WOL configuration before to set, over written with wanted wol request configuration.
> > Please find the attached log information
> > > So:
> > >
> > > > a. We only enable secure on magic packet when both secure and magic
> wol
> > > > options are requested together.
> > >
> > > I don't think they need to be requested together. I think you can
> > > first enable Wake on MagicPacket and then in a second call to
> > > ethtool Enable SecureOn password for MagicPacket. I also don't think
> > > it would unreasonable to accept Enable SecureOn password for
> > > MagicPacket and have that imply Wake on MagicPacket.
> > >
> >
> > If we need to enable any 2 options, we have to provide both options
> together.
> > i.e.
> > sudo ethtool -s enp9s0 wol ga
>
> which i think is wrong. A driver should allow incremental adding WoL options.
>
Ok.
> >
> > > And:
> > >
> > > > b. If secure-on magic packet had been previously enabled, and a
> > > subsequent
> > > > command does not include it, we add it.
> > >
> > > If there has not been a d, secure-on magic packet should remain
> > > enabled until there is a d.
> > >
> >
> > This is not happened with existing "Ethtool".
> > Please find the log information in an attachment file.
>
> That could just be a driver bug.
>
> Take a step back. Is there any clear documentation about how ethtool -s wol
> should really work? Any comments in the code? Any man page
> documentation.
>
I'm not able to find any more "ethtool" documentations other than ethtool man page.
I have gone through the ethtool application code.
(git://git.kernel.org/pub/scm/network/ethtool/ethtool.git) and check the WOL options.
In ioctl method (Not netlink), do_sset( ) function, before set the parameters, first get the WOL configuration.
(i.e. in ethtool.c file, Line no. 3294)
Then, assign/overwrite WOL wanted config to wolopts parameter.
Existing Code: From Line 3290 - 3312
#######################################################################
3290 if (gwol_changed) {
3291 struct ethtool_wolinfo wol;
3292
3293 wol.cmd = ETHTOOL_GWOL;
3294 err = send_ioctl(ctx, &wol);
3295 if (err < 0) {
3296 perror("Cannot get current wake-on-lan settings");
3297 } else {
3298 /* Change everything the user specified. */
3299 if (wol_change)
3300 wol.wolopts = wol_wanted;
3301 if (sopass_change) {
3302 int i;
3303 for (i = 0; i < SOPASS_MAX; i++)
3304 wol.sopass[i] = sopass_wanted[i];
3305 }
3306
3307 /* Try to perform the update. */
3308 wol.cmd = ETHTOOL_SWOL;
3309 err = send_ioctl(ctx, &wol);
3310 if (err < 0)
3311 perror("Cannot set new wake-on-lan settings");
3312 }
#######################################################################
Current issue is overwriting wolopts with wol_wanted
i.e.
3300 wol.wolopts = wol_wanted;
Instead of over write, Need to "OR" the wolopts and wol_wanted
i.e.
3300 wol.wolopts |= wol_wanted
Final code changes should be:
#######################################################################
3290 if (gwol_changed) {
3291 struct ethtool_wolinfo wol;
3292
3293 wol.cmd = ETHTOOL_GWOL;
3294 err = send_ioctl(ctx, &wol);
3295 if (err < 0) {
3296 perror("Cannot get current wake-on-lan settings");
3297 } else {
3298 /* Change everything the user specified. */
3299 if (wol_change && wol_wanted)
3300 wol.wolopts |= wol_wanted;
else if (wol_change && !wol_wanted)
wol.wolopts = 0
3301 if (sopass_change) {
3302 int i;
3303 for (i = 0; i < SOPASS_MAX; i++)
3304 wol.sopass[i] = sopass_wanted[i];
3305 }
3306
3307 /* Try to perform the update. */
3308 wol.cmd = ETHTOOL_SWOL;
3309 err = send_ioctl(ctx, &wol);
3310 if (err < 0)
3311 perror("Cannot set new wake-on-lan settings");
3312 }
#######################################################################
Similar changes require in netlink functions also.
> Lets first understand how it is expected to work. Then we can decided if the
> driver is implementing it correctly.
>
> Andrew
Please find the "ethtool" application code main file as attachment file.
Thanks,
Raju
View attachment "ethtool.c" of type "text/plain" (161479 bytes)
Powered by blists - more mailing lists