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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.