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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aAERy1qnTyTGT-_w@shell.armlinux.org.uk>
Date: Thu, 17 Apr 2025 15:35:55 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Alexander H Duyck <alexander.duyck@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"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 Thu, Apr 17, 2025 at 07:30:05AM -0700, Alexander H Duyck wrote:
> 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.

Sorry to say, but you have that wrong. Neither are 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.

Nope.

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

Another of phylink's guarantees is that it won't do the mac_config()
etc with the link up. So, in order to ensure that everything is
correctly programmed after resume, it needs mac_config() etc called
which means the link needs to come down first.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!