netdev - Re: [PATCH v2] 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: <20241007222234.ekpqibldugchuvk7@skbuf>
Date: Tue, 8 Oct 2024 01:22:34 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Mohammed Anees <pvmohammedanees2003@...il.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH v2] net: dsa: Fix conditional handling of Wake-on-Lan
 configuration in dsa_user_set_wol

Hi Mohammed,

On Mon, Oct 07, 2024 at 04:49:38AM +0530, Mohammed Anees wrote:
> In the original implementation of dsa_user_set_wol(), the return
> value of phylink_ethtool_set_wol() was not checked, which could
> lead to errors being ignored. This wouldn't matter if it returned
> -EOPNOTSUPP, since that indicates the PHY layer doesn't support
> the option, but if any other value is returned, it is problematic
> and must be checked. The solution is to check the return value of
> phylink_ethtool_set_wol(), and if it returns anything other than
> -EOPNOTSUPP, immediately return the error. Only if it returns
> -EOPNOTSUPP should the function proceed to check whether WoL can
> be set by ds->ops->set_wol().
> 
> Fixes: 57719771a244 ("Merge tag 'sound-6.11' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound")

The Fixes: tag is completely bogus. It's supposed to point to the commit
which introduced the issue (aka what 'git bisect' would land on).

> Signed-off-by: Mohammed Anees <pvmohammedanees2003@...il.com>
> ---
> v2:
> - Added error checking for phylink_ethtool_set_wol(), ensuring correct
> handling compared to v1.
> ___

this should have been "---" not "___".

>  net/dsa/user.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 74eda9b30608..bae5ed22db91 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -1215,14 +1215,17 @@ static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
> -	phylink_ethtool_set_wol(dp->pl, w);
> +	ret = phylink_ethtool_get_wol(dp->pl, w);

Could you tell us a bit about your motivation for making a change?
How have you noticed the lack of error checking in phylink_ethtool_set_wol()?
What user-visible problem has it caused?

Since patches with Fixes: tags to older than net-next commits
get backported to stable kernels, the triage rules from
Documentation/process/stable-kernel-rules.rst apply.

If this is purely theoretical and you are not already testing this,
then please do so. (it seems you aren't, because you replaced
phylink_ethtool_get_wol() with phylink_ethtool_set_wol()).

Luckily, you could somewhat exercise the code paths using the dsa_loop
mock-up driver even if you don't have a supported hardware switch. It
isn't the same as the real thing, but with some instrumentation and
carefully chosen test cases and simulated return codes, I could see it
being used to prove a point.

If you don't have the interest of testing this patch, then I would
request you to abandon it. The topic of combining MAC WoL with PHY WoL
is not trivial and a theoretical "fix" can make things that used to work
stop working. It's unlikely that basing patches off of a chat on the
mailing list is going to make things better if it all stays theoretical
and no one tests anything, even if that chat is with Andrew and Russell,
the opinions of both of whom are extremely respectable in this area.

In principle there's also the option of requesting somebody else to
test if you cannot, like the submitter of the blamed patch, if there's
reasonable suspicion that something is not right. In this case,
interesting thing, the phylink_ethtool_set_wol() call got introduced in
commit aab9c4067d23 ("net: dsa: Plug in PHYLINK support") by Florian,
but there was no phy_ethtool_set_wol() prior to that. So it's not clear
at all what use cases came to depend upon the phylink_ethtool_set_wol()
call in the meantime since 2018. I'm not convinced that said commit
voluntarily introduced the call.

If this is not an exclusively theoretical issue and this was just an
honest mistake, then please do continue the patch submission process.
But for my curiosity, what platform are you experimenting on?