linux-kernel - Re: [PATCH net-next] net: phy: introduce phydev->port

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: <YCM8JiO4FfKx5ECo@lunn.ch>
Date:   Wed, 10 Feb 2021 02:51:34 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Michael Walle <michael@...le.cc>
Cc:     bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] net: phy: introduce phydev->port

> @@ -1552,6 +1552,7 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
>  	phydev->asym_pause = 0;
>  	phydev->speed = SPEED_UNKNOWN;
>  	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->port = fiber ? PORT_FIBRE : PORT_TP;
>  
>  	if (phydev->autoneg == AUTONEG_ENABLE)
>  		err = marvell_read_status_page_an(phydev, fiber, status);

...

> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else
> -		cmd->base.port = PORT_MII;
> +		cmd->base.port = phydev->port;
>  	cmd->base.transceiver = phy_is_internal(phydev) ?
>  				XCVR_INTERNAL : XCVR_EXTERNAL;
>  	cmd->base.phy_address = phydev->mdio.addr;

This is a general comment, not a problem specific to this patch.

There is some interesting race conditions here. The marvell driver
first checks the fibre page and gets the status of the fiber port. As
you can see from the hunk above, it clears out pause, duplex, speed,
sets port to PORT_FIBRE, and then reads the PHY registers to set these
values. If link is not detected on the fibre, it swaps page and does
it all again, but for the copper port. So once per second,
phydev->port is going to flip flop PORT_FIBER->PORT_TP, if copper has
link.

Now, the read_status() call into the driver should be performed while
holding the phydev->lock. So to the PHY state machine, this flip/flop
does not matter, it is atomic with respect to the lock. But
phy_ethtool_ksettings_get() is not talking the lock. It could see
speed, duplex, and port while they have _UNKNOWN values, or port is
part way through a flip flop. I think we need to take the lock here.
phy_ethtool_ksettings_set() should also probably take the lock.

     Andrew