netdev - Re: [RFC] net: phy: allow ethtool to set any supported fixed speed

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] [day] [month] [year] [list]

Message-ID: <20191122151908.GK25745@shell.armlinux.org.uk>
Date:   Fri, 22 Nov 2019 15:19:08 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC] net: phy: allow ethtool to set any supported fixed speed

On Fri, Nov 22, 2019 at 03:18:10PM +0000, Russell King wrote:
> phylib restricts the fixed speed to 1000, 100 or 10Mbps, even if the
> PHY supports other speeds, or doesn't even support these speeds.
> Validate the fixed speed against the PHY capabilities, and return an
> error if we are unable to find a match.
> 
> Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> ---
> NOTE: is this correct behaviour - or should we do something like:
> 
> 	s = phy_lookup_setting(speed, duplex, phydev->support, false);
> 	if (!s)
> 		return -EINVAL;
> 
> 	phydev->speed = speed;
> 	phydev->duplex = duplex;

Sorry, that should've been:

	phydev->speed = s->speed;
	phydev->duplex = s->duplex;

> 
> IOW, set either an exact match, or a slower supported speed than was
> requested, or the slowest?  That's how phy_sanitize_settings() is
> implemented, which I replicated for phylink's ethtool implementation.
> 
> Another issue here is with the validation of the settings that the
> user passed in - this looks very racy.  Consider the following:
> 
> - another thread calls phy_ethtool_ksettings_set(), which sets
>   phydev->speed and phydev->duplex, and disables autoneg.
> - the phylib state machine is running, and overwrites the
>   phydev->speed and phydev->duplex settings
> - phy_ethtool_ksettings_set() then calls phy_start_aneg() which
>   sets the PHY up with the phylib-read settings rather than the
>   settings the user requested.
> 
> IMHO, phylib needs to keep the user requested settings separate from
> the readback state from the PHY.
> 
> Yet another issue is what to do when a PHY doesn't support disabled
> autoneg (or it's not known how to make it work) - the PHY driver
> doesn't get a look-in to validate the settings, phylib just expects
> every PHY out there to support it.  The best the PHY driver can do
> is to cause it's config_aneg() method to return -EINVAL, dropping
> the PHY state machine into PHY_HALTED mode via phy_error() - which
> will then provoke a nice big stack dump in phy_stop() when the
> network device is downed as phy_is_started() will return false.
> Clearly not a good user experience on any level (API or kernel
> behaviour.)
> 
>  drivers/net/phy/phy.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9e431b9f9d87..75d11c48afce 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -270,31 +270,32 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  	linkmode_and(advertising, advertising, phydev->supported);
>  
>  	/* Verify the settings we care about. */
> -	if (autoneg != AUTONEG_ENABLE && autoneg != AUTONEG_DISABLE)
> -		return -EINVAL;
> +	switch (autoneg) {
> +	case AUTONEG_ENABLE:
> +		if (linkmode_empty(advertising))
> +			return -EINVAL;
> +		break;
> +	
> +	case AUTONEG_DISABLE:
> +		if (duplex != DUPLEX_HALF && duplex != DUPLEX_FULL)
> +			return -EINVAL;
>  
> -	if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
> -		return -EINVAL;
> +		if (!phy_lookup_setting(speed, duplex, phydev->supported, true))
> +			return -EINVAL;
> +		break;
>  
> -	if (autoneg == AUTONEG_DISABLE &&
> -	    ((speed != SPEED_1000 &&
> -	      speed != SPEED_100 &&
> -	      speed != SPEED_10) ||
> -	     (duplex != DUPLEX_HALF &&
> -	      duplex != DUPLEX_FULL)))
> +	default:
>  		return -EINVAL;
> +	}
>  
>  	phydev->autoneg = autoneg;
> -
> +	phydev->duplex = duplex;
>  	phydev->speed = speed;
>  
>  	linkmode_copy(phydev->advertising, advertising);
> -
>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>  			 phydev->advertising, autoneg == AUTONEG_ENABLE);
>  
> -	phydev->duplex = duplex;
> -
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> -- 
> 2.20.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up