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: <LV8PR11MB8700FC048AB32D6E7168855E9F5E2@LV8PR11MB8700.namprd11.prod.outlook.com>
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ