Re: [PATCH net-next v3] net: phy: Fix suspicious rcu_dereference usage

On Tue, Jan 21, 2025 at 10:38:45AM +0100, Kory Maincent wrote:

On Mon, 20 Jan 2025 11:12:28 -0800
Jakub Kicinski [off-list ref] wrote:

quoted

On Mon, 20 Jan 2025 15:19:25 +0100 Kory Maincent wrote:

quoted

The path reported to not having RTNL lock acquired is the suspend path of
the ravb MAC driver. Without this fix we got this warning:  

I maintain that ravb is buggy, plenty of drivers take rtnl_lock 
from the .suspend callback. We need _some_ write protection here,
the patch as is only silences a legitimate warning.

Indeed if the suspend path is buggy we should fix it. Still there is lots of
ethernet drivers calling phy_disconnect without rtnl (IIUC) if probe return an
error or in the remove path. What should we do about it?

They could trigger the same warning, although I think they would be
relatively safe because register_netdev() hasn't been called, and thus
nothing that the netdev provides should be used. (If it can be used, as
the driver has not completed initialisation, then it's probably racy
anyway.)

I don't think throwing ASSERT_RTNL() into phy_detach() will do anything
to solve this. If the RCU warning doesn't trigger (because phy_detach()
only gets called on error which practically never happens) then
ASSERT_RTNL() isn't going to trigger either. Warnings in functions will
only work when they're called in a context that will trigger the
warning!

So, I think it's something that can only be addressed by reviewing
drivers and patching.

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