Re: [PATCH net-next v13 05/13] net: ethtool: Allow passing a phy index for some commands

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sun, Jun 16, 2024 at 06:02:31PM +0200, Maxime Chevallier wrote:
> Hello Jakub,
> 
> On Thu, 13 Jun 2024 18:26:13 -0700
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > > +			struct nlattr *phy_id;
> > > +
> > > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > > +			phydev = phy_link_topo_get_phy(dev,
> > > +						       nla_get_u32(phy_id));  
> > 
> > Sorry for potentially repeating question (please put the answer in the
> > commit message) - are phys guaranteed not to disappear, even if the
> > netdev gets closed? this has no rtnl protection
> 
> I'll answer here so that people can correct me if I'm wrong, but I'll
> also add it in the commit logs as well (and possibly with some fixes
> depending on how this discussion goes)
> 
> While a PHY can be attached to/detached from a netdevice at open/close,
> the phy_device itself will keep on living, as its lifetime is tied to
> the underlying mdio_device (however phy_attach/detach take a ref on the
> phy_device, preventing it from vanishing while it's attached to a
> netdev)
> 
> I think the worst that could happen is that phy_detach() gets
> called (at ndo_close() for example, but that's not the only possible
> call site for that), and right after we manually unbind the PHY, which
> will drop its last refcount, while we hold a pointer to it :
> 
> 			phydev = phy_link_topo_get_phy()
>  phy_detach(phydev)
>  unbind on phydev
> 			/* access phydev */
> 			
> PHY device lifetime is, from my understanding, not protected by
> rtnl() so should a lock be added, I don't think rtnl_lock() would be
> the one to use.

... and that will cause deadlocks. For example, ethernet drivers can
call phy_disconnect() from their .ndo_close method, which will be
called with the RTNL lock held. This calls phy_detach(), so
phy_detach() also gets called while the RTNL lock is held.

SFP will call all phylib methods while holding the RTNL lock as well
(because that's the only safe way to add or remove a PHY, as it stops
other changes to the config that may conflict, and also ensures that
e.g. paths in phylib will not be in-use when the PHY is being
destroyed.)

So, rather than thinking that phylib should add RTNL locking, it
would be much more sensible to do what phylink does, and enforce
that the RTNL will be held when netdev related methods are called,
but also require that paths that end up changing phylib's configuration
(e.g. removing a PHY driver) end up taking the RTNL lock - because
that is the only way to be sure that none of the phylib methods
that call into the driver are currently executing in another thread.

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