netdev - Re: [PATCH net] net: phy: clear phydev->devlink when the link is deleted

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: <d8cb2a8f-af3d-4042-96a3-a762df1876a9@lunn.ch>
Date: Thu, 22 May 2025 18:19:13 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Wei Fang <wei.fang@....com>
Cc: "hkallweit1@...il.com" <hkallweit1@...il.com>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>,
	"xiaolei.wang@...driver.com" <xiaolei.wang@...driver.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: Re: [PATCH net] net: phy: clear phydev->devlink when the link is
 deleted

On Thu, May 22, 2025 at 01:57:20PM +0000, Wei Fang wrote:
> > On Thu, May 22, 2025 at 02:42:53PM +0800, Wei Fang wrote:
> > > The phydev->devlink is not cleared when the link is deleted, so calling
> > > phy_detach() again will cause a crash.
> > 
> > I would say crashing is correct. You have done something you should
> > not do, and the crash helped you find it. phy_attach() and
> > phy_detach() should always be in pairs.
> > 
> 
> phy_attach() and phy_detach() are called in pairs in my case. When
> re-enabling the network port, if an error occurs in the phy_attach_direct(),
> For example, if phy_init_hw() returns an error, it will jump to the error
> path and call phy_detach(). Because phy_detach() did not clear the
> phydev->devlink pointer when the network port was disabled,
> device_link_del() will access a NULL pointer and cause a crash. And this
> crash may cause the CPU to hang. I don't think it is reasonable to cause
> the CPU to hang.

Ah, now i get it...

phy_attach_direct() runs, but fails early, before the call to:

phydev->devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev,
						  DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);

So it has its old value, from a previous attach/detach cycle. At the
error: label, it calls phy_detach(phydev), which does a
device_link_del(), using the old value...

Please improve your changelog message, add more details.

    Andrew

---
pw-bot: cr