netdev - Re: [PATCH net] net: phy: fix phy_uses_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 for Android: free password hash cracker in your pocket

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

Message-ID: <aLVwAn87GhFHMjEE@shell.armlinux.org.uk>
Date: Mon, 1 Sep 2025 11:05:54 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <vladimir.oltean@....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>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] net: phy: fix phy_uses_state_machine()

On Mon, Sep 01, 2025 at 12:35:30PM +0300, Vladimir Oltean wrote:
> On Mon, Sep 01, 2025 at 10:09:17AM +0100, Russell King (Oracle) wrote:
> > On Mon, Sep 01, 2025 at 11:42:25AM +0300, Vladimir Oltean wrote:
> > > On Sun, Aug 31, 2025 at 05:38:11PM +0100, Russell King (Oracle) wrote:
> > > > phydev->phy_link_change is initialised by phy_attach_direct(), and
> > > > overridden by phylink. This means that a never-connected PHY will
> > > > have phydev->phy_link_change set to NULL, which causes
> > > > phy_uses_state_machine() to return true. This is incorrect.
> > > 
> > > Another nitpick regarding phrasing here: the never-connected PHY doesn't
> > > _cause_ phy_uses_state_machine() to return true. It returns true _in
> > > spite_ of the PHY never being connected: the non-NULL quality of
> > > phydev->phy_link_change is not something that phy_uses_state_machine()
> > > tests for.
> > 
> > No. What I'm saying is that if phydev->phy_link_change is set to NULL,
> > _this_ causes phy_uses_state_machine() to return true and that
> > behaviour incorrect.
> > 
> > The first part is describing _when_ phydev->phy_link_change is set to
> > NULL.
> > 
> > It is not saying that a never-connected PHY directly causes
> > phy_uses_state_machine() to return true.
> > 
> > I think my phrasing of this is totally fine, even re-reading it now.
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 
> I think the language is sufficiently flexible for us to find a phrasing
> that avoids all ambiguity. What about:
> 
> "It was assumed the only two valid values for phydev->phy_link_change
> are phy_link_change() and phylink_phy_change(), thus phy_uses_state_machine()
> was oversimplified to only compare with one of these values. There is a
> third possible value (NULL), meaning that the PHY is unconnected, and
> does not use the state machine. This logic misinterprets this case as
> phy_uses_state_machine() == true, but in reality it should return false."

Well, having spent considerable time writing and rewriting the damn
commit message, this is what I'm now using, which I think covers the
problem in sufficient detail.

>>>>>>
net: phy: fix phy_uses_state_machine()

The blamed commit changed the conditions which phylib uses to stop
and start the state machine in the suspend and resume paths, and
while improving it, has caused two issues.

The original code used this test:

        phydev->attached_dev && phydev->adjust_link

and if true, the paths would handle the PHY state machine. This test
evaluates true for normal drivers that are using phylib directly
while the PHY is attached to the network device, but false in all
other cases, which include the following cases:

- when the PHY has never been attached to a network device.
- when the PHY has been detached from a network device (as phy_detach()
   sets phydev->attached_dev to NULL, phy_disconnect() calls
   phy_detach() and additionally sets phydev->adjust_link NULL.)
- when phylink is using the driver (as phydev->adjust_link is NULL.)

Only the third case was incorrect, and the blamed commit attempted to
fix this by changing this test to (simplified for brevity, see
phy_uses_state_machine()):

        phydev->phy_link_change == phy_link_change ?
                phydev->attached_dev && phydev->adjust_link : true

However, this also incorrectly evaluates true in the first two cases.

Fix the first case by ensuring that phy_uses_state_machine() returns
false when phydev->phy_link_change is NULL.

Fix the second case by ensuring that phydev->phy_link_change is set to
NULL when phy_detach() is called.
<<<<<<

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