Re: [PATCH net-next v4 3/3] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook
Carlo Szelinsky <[email protected]>
| Newsgroups | org.kernel.vger.netdev,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
> This patch causes a deadlock with the etop driver (lantiq arx100 soc > family). Thanks Aleksander, and thanks for testing it on real hardware. It's a real deadlock and you're right about the cause. The cause is that this patch makes phy_device_register() take rtnl_lock() around the psec attach. etop registers its MDIO bus from ndo_init (ltq_etop_mdio_init -> mdiobus_register -> mdiobus_scan -> phy_device_register), and ndo_init already runs under rtnl inside register_netdevice(). So the second rtnl_lock() deadlocks. It only shows up on drivers that register the bus from ndo_init - most do it from probe, outside rtnl - which is why it didn't turn up here earlier. I don't think we should revert. As Jonas said, this should be fixed in the patch. My idea for the next version is to stop using rtnl for the psec attach/detach and use a small dedicated mutex instead. A private lock can't recurse against the caller's rtnl, so the etop case would go away. Kory, others - does that sound like the right direction to you? One thing I want to flag: rtnl there wasn't only guarding the attach, it was also keeping the ethtool code that reads phydev->psec from racing the detach that frees it. So the mutex would need to sit in two places - the phy side and the ethtool paths in net/ethtool/pse-pd.c - otherwise the deadlock is gone but the use-after-free comes back. Just so we pick the right approach. Thanks, Carlo