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] [thread-next>] [day] [month] [year] [list]
Message-ID: <LV8PR11MB8700258564E1FF5DF96E6FE99F232@LV8PR11MB8700.namprd11.prod.outlook.com>
Date: Mon, 4 Mar 2024 11:20:03 +0000
From: <Raju.Lakkaraju@...rochip.com>
To: <andrew@...n.ch>, <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>,
	<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, 

For Ethtool netlink, we need to change the "ethnl_set_wol( )" to add option is incremental.
Can you please check those changes OK or not.

Thanks,
Raju.

> -----Original Message-----
> From: Raju Lakkaraju - I30499
> Sent: Friday, March 1, 2024 3:52 PM
> To: Andrew Lunn <andrew@...n.ch>
> 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
> 
> 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.
> 

For Netlink:

--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -107,16 +107,26 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
        struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
        struct net_device *dev = req_info->dev;
        struct nlattr **tb = info->attrs;
+       __u32 wolopts = 0;
        bool mod = false;
        int ret;
 
        dev->ethtool_ops->get_wol(dev, &wol);
-       ret = ethnl_update_bitset32(&wol.wolopts, WOL_MODE_COUNT,
+       ret = ethnl_update_bitset32(&wolopts, WOL_MODE_COUNT,
                                    tb[ETHTOOL_A_WOL_MODES], wol_mode_names,
                                    info->extack, &mod);
        if (ret < 0)
                return ret;
-       if (wol.wolopts & ~wol.supported) {
+
+       if (wolopts) {
+               wol.wolopts |= wolopts;
+       } else {
+               wol.wolopts = 0;
+               memset(&wol.sopass, 0, sizeof(wol.sopass));
+               mod = true;
+       }
+
+       if (wolopts & ~wol.supported) {
                NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_WOL_MODES],
                                    "cannot enable unsupported WoL mode");
                return -EINVAL;
###############################################################

Please find the an attachment file.

> > 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

Download attachment "0001-ethtool-netlink-adding-options-is-incremental.patch" of type "application/octet-stream" (1482 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ