netdev - Re: [PATCH net] net: phylink: fix suspend/resume with WoL enabled and link down

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9910f0885c4ee48878569d3e286072228088137a.camel@gmail.com>
Date: Thu, 17 Apr 2025 07:30:05 -0700
From: Alexander H Duyck <alexander.duyck@...il.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>, Andrew Lunn
	 <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>,  Jakub Kicinski <kuba@...nel.org>, Joakim Zhang
 <qiangqing.zhang@....com>,  netdev@...r.kernel.org, Paolo Abeni
 <pabeni@...hat.com>
Subject: Re: [PATCH net] net: phylink: fix suspend/resume with WoL enabled
 and link down

On Wed, 2025-04-16 at 17:16 +0100, Russell King (Oracle) wrote:
> When WoL is enabled, we update the software state in phylink to
> indicate that the link is down, and disable the resolver from
> bringing the link back up.
> 
> On resume, we attempt to bring the overall state into consistency
> by calling the .mac_link_down() method, but this is wrong if the
> link was already down, as phylink strictly orders the .mac_link_up()
> and .mac_link_down() methods - and this would break that ordering.
> 
> Fixes: f97493657c63 ("net: phylink: add suspend/resume support")
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> ---
> 
> To fix the suspend/resume with link down, this is what I think we
> should do. Untested at the moment.
> 
>  drivers/net/phy/phylink.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 69ca765485db..d2c59ee16ebc 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -81,6 +81,7 @@ struct phylink {
>  	unsigned int pcs_state;
>  
>  	bool link_failed;
> +	bool suspend_link_up;
>  	bool major_config_failed;
>  	bool mac_supports_eee_ops;
>  	bool mac_supports_eee;

I'm pretty sure this extra bit of state isn't needed.

> @@ -2545,14 +2546,16 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
>  		/* Stop the resolver bringing the link up */
>  		__set_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
>  
> -		/* Disable the carrier, to prevent transmit timeouts,
> -		 * but one would hope all packets have been sent. This
> -		 * also means phylink_resolve() will do nothing.
> -		 */
> -		if (pl->netdev)
> -			netif_carrier_off(pl->netdev);
> -		else

This is the only spot where we weren't setting netif_carrier_on/off and
old_link_state together. I suspect you could just carry old_link_state
without needing to add a new argument. Basically you would just need to
drop the "else" portion of this statement.

In the grand scheme of things with the exception of this one spot
old_link_state is essentially the actual MAC/PCS link state whereas
netif_carrier_off is the administrative state.

> +		pl->suspend_link_up = phylink_link_is_up(pl);
> +		if (pl->suspend_link_up) {
> +			/* Disable the carrier, to prevent transmit timeouts,
> +			 * but one would hope all packets have been sent. This
> +			 * also means phylink_resolve() will do nothing.
> +			 */
> +			if (pl->netdev)
> +				netif_carrier_off(pl->netdev);
>  			pl->old_link_state = false;
> +		}
>  
>  		/* We do not call mac_link_down() here as we want the
>  		 * link to remain up to receive the WoL packets.
> @@ -2603,15 +2606,18 @@ void phylink_resume(struct phylink *pl)
>  	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
>  		/* Wake-on-Lan enabled, MAC handling */
>  
> -		/* Call mac_link_down() so we keep the overall state balanced.
> -		 * Do this under the state_mutex lock for consistency. This
> -		 * will cause a "Link Down" message to be printed during
> -		 * resume, which is harmless - the true link state will be
> -		 * printed when we run a resolve.
> -		 */
> -		mutex_lock(&pl->state_mutex);
> -		phylink_link_down(pl);
> -		mutex_unlock(&pl->state_mutex);
> +		if (pl->suspend_link_up) {
> +			/* Call mac_link_down() so we keep the overall state
> +			 * balanced. Do this under the state_mutex lock for
> +			 * consistency. This will cause a "Link Down" message
> +			 * to be printed during resume, which is harmless -
> +			 * the true link state will be printed when we run a
> +			 * resolve.
> +			 */
> +			mutex_lock(&pl->state_mutex);
> +			phylink_link_down(pl);
> +			mutex_unlock(&pl->state_mutex);
> +		}

You should be able to do all of this with just old_link_state. The only
thing that would have to change is that you would need to set
old_link_state to false after the if statement.

I'm assuming part of the reason for forcing the link down here also has
to do with the fact that you are using phylink_mac_initial_config which
calls phylink_major_config after this?