netdev - Re: [BUG] net/phy: ethtool versus phy_state

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: <YUPMQ1HZDBEELnz0@lunn.ch>
Date:   Fri, 17 Sep 2021 00:59:15 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Walter Stoll <Walter.Stoll@...gon.com>
Cc:     "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [BUG] net/phy: ethtool versus phy_state_machine race condition

On Thu, Sep 16, 2021 at 01:08:21PM +0000, Walter Stoll wrote:
> Effect observed
> ---------------
> 
> During final test of one of our products, we use ethtool to set the mode of
> the ethernet port eth0 as follows:
> 
> ethtool -s eth0 speed 100 duplex full autoneg off
> 
> However, very rarely the command does not work as expected. Even though the
> command executes without error, the port is not set accordingly. As a result,
> the test fails.
> 
> We observed the effect with kernel version v5.4.138-rt62. However, we think
> that the most recent kernel exhibits the same behavior because the structure of
> the sources in question (see below) did not change. This also holds for the non
> realtime kernel.
> 
> 
> Root cause
> ----------
> 
> We found that there exists a race condition between ethtool and the PHY state-
> machine.
> 
> Execution of the ethtool command involves the phy_ethtool_ksettings_set()
> function being executed, see 
> https://elixir.bootlin.com/linux/v5.4.138/source/drivers/net/phy/phy.c#L315
> 
> Here the mode is stored in the phydev structure. The phy_start_aneg() function
> then takes the mode from the phydev structure and finally stores the mode into
> the PHY.
> 
> However, the phy_ethtool_ksettings_set() function can be interrupted by the
> phy_state_machine() worker thread. If this happens just before the
> phy_start_aneg() function is called, then the new settings are overwritten by
> the current settings of the PHY. This is because the phy_state_machine()
> function reads back the PHY settings, see
> https://elixir.bootlin.com/linux/v5.4.138/source/drivers/net/phy/phy.c#L918
> 
> This is exactly what happens in our case. We were able to proof this by
> inserting various dev_info() calls in the code. 

Hi Walter

This makes sense.  We have a similar problem with MAC code calling
phy_read_status() without holding the PHY lock as well. I have some
patches for that, which i need to rebase. I will see if your proposed
fixed can be added to that, or if it should be a separate series.

      Andrew