netdev - Re: [PATCH] net: dsa: Fix conditional handling of Wake-on-Lan configuration in dsa_user_set

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: <0d151801-f27c-4f53-9fb1-ce459a861b82@lunn.ch>
Date: Sat, 5 Oct 2024 23:00:13 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Mohammed Anees <pvmohammedanees2003@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, f.fainelli@...il.com,
	kuba@...nel.org, linux-kernel@...r.kernel.org,
	linux@...linux.org.uk, netdev@...r.kernel.org, olteanv@...il.com,
	pabeni@...hat.com
Subject: Re: [PATCH] net: dsa: Fix conditional handling of Wake-on-Lan
 configuration in dsa_user_set_wol

On Sun, Oct 06, 2024 at 12:12:33AM +0530, Mohammed Anees wrote:
> In the original code, we initialize ret = -EOPNOTSUPP and then call 
> phylink_ethtool_set_wol(). If DSA supports WOL, we call set_wol(). 
> However, we aren’t checking if phylink_ethtool_set_wol() succeeds, 
> so I assumed both functions should be called, and if either fails,
> we return -EOPNOTSUPP.
> 
> 
> static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
> {
> 	struct dsa_port *dp = dsa_user_to_port(dev);
> 	struct dsa_switch *ds = dp->ds;
> 	int ret = -EOPNOTSUPP;
> 
> 	phylink_ethtool_set_wol(dp->pl, w);
> 
> 	if (ds->ops->set_wol)
> 		ret = ds->ops->set_wol(ds, dp->index, w);
> 
> 	return ret;
> }
> 
> >From your response, it seems either of the two function can handle setting 
> WOL, if so shouldn't we check the return value of phylink_ethtool_set_wol() 
> to ensure it succeeds?

It is actually a bit more subtle than that, and i think everything
gets it wrong. Yes, we should check the return code from
phylink_ethtool_set_wol. If it does not return an error, we are
done. If it returns an error other than EOPNOTSUPP, it should return
it. And in the case of EOPNOTSUPP we should try to see if DSA supports
the WoL mode. And this is probably an over simplification. ethtool man
page 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  specifying
                  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.

So userspace could say pumbagsf, with the PHY supporting pmub and the
MAC supporting agsf, and the two need to cooperate.

get_wol() needs to call both phylink_ethtool_get_wol() and dsa
get_wol, and combine the results.

	Andrew