Message-ID: <aLmZ5Ry8lPHf2Qvg@shell.armlinux.org.uk>
Date: Thu, 4 Sep 2025 14:53:41 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 net 2/2] net: phy: transfer phy_config_inband()
 locking responsibility to phylink

On Thu, Sep 04, 2025 at 03:52:38PM +0300, Vladimir Oltean wrote:
> Problem description
> ===================
> 
> Lockdep reports a possible circular locking dependency (AB/BA) between
> &pl->state_mutex and &phy->lock, as follows.
> 
> phylink_resolve() // acquires &pl->state_mutex
> -> phylink_major_config()
>    -> phy_config_inband() // acquires &pl->phydev->lock
> 
> whereas all the other call sites where &pl->state_mutex and
> &pl->phydev->lock have the locking scheme reversed. Everywhere else,
> &pl->phydev->lock is acquired at the top level, and &pl->state_mutex at
> the lower level. A clear example is phylink_bringup_phy().
> 
> The outlier is the newly introduced phy_config_inband() and the existing
> lock order is the correct one. To understand why it cannot be the other
> way around, it is sufficient to consider phylink_phy_change(), phylink's
> callback from the PHY device's phy->phy_link_change() virtual method,
> invoked by the PHY state machine.
> 
> phy_link_up() and phy_link_down(), the (indirect) callers of
> phylink_phy_change(), are called with &phydev->lock acquired.
> Then phylink_phy_change() acquires its own &pl->state_mutex, to
> serialize changes made to its pl->phy_state and pl->link_config.
> So all other instances of &pl->state_mutex and &phydev->lock must be
> consistent with this order.
> 
> Problem impact
> ==============
> 
> I think the kernel runs a serious deadlock risk if an existing
> phylink_resolve() thread, which results in a phy_config_inband() call,
> is concurrent with a phy_link_up() or phy_link_down() call, which will
> deadlock on &pl->state_mutex in phylink_phy_change(). Practically
> speaking, the impact may be limited by the slow speed of the medium
> auto-negotiation protocol, which makes it unlikely for the current state
> to still be unresolved when a new one is detected, but I think the
> problem is there. Nonetheless, the problem was discovered using lockdep.
> 
> Proposed solution
> =================
> 
> Practically speaking, the phy_config_inband() requirement of having
> phydev->lock acquired must transfer to the caller (phylink is the only
> caller). There, it must bubble up until immediately before
> &pl->state_mutex is acquired, for the cases where that takes place.
> 
> Solution details, considerations, notes
> =======================================
> 
> This is the phy_config_inband() call graph:
> 
>                           sfp_upstream_ops :: connect_phy()
>                           |
>                           v
>                           phylink_sfp_connect_phy()
>                           |
>                           v
>                           phylink_sfp_config_phy()
>                           |
>                           |   sfp_upstream_ops :: module_insert()
>                           |   |
>                           |   v
>                           |   phylink_sfp_module_insert()
>                           |   |
>                           |   |   sfp_upstream_ops :: module_start()
>                           |   |   |
>                           |   |   v
>                           |   |   phylink_sfp_module_start()
>                           |   |   |
>                           |   v   v
>                           |   phylink_sfp_config_optical()
>  phylink_start()          |   |
>    |   phylink_resume()   v   v
>    |   |  phylink_sfp_set_config()
>    |   |  |
>    v   v  v
>  phylink_mac_initial_config()
>    |   phylink_resolve()
>    |   |  phylink_ethtool_ksettings_set()
>    v   v  v
>    phylink_major_config()
>             |
>             v
>     phy_config_inband()
> 
> phylink_major_config() caller #1, phylink_mac_initial_config(), does not
> acquire &pl->state_mutex nor do its callers. It must acquire
> &pl->phydev->lock prior to calling phylink_major_config().
> 
> phylink_major_config() caller #2, phylink_resolve() acquires
> &pl->state_mutex, thus also needs to acquire &pl->phydev->lock.
> 
> phylink_major_config() caller #3, phylink_ethtool_ksettings_set(), is
> completely uninteresting, because it only calls phylink_major_config()
> if pl->phydev is NULL (otherwise it calls phy_ethtool_ksettings_set()).
> We need to change nothing there.
> 
> Other solutions
> ===============
> 
> The lock inversion between &pl->state_mutex and &pl->phydev->lock has
> occurred at least once before, as seen in commit c718af2d00a3 ("net:
> phylink: fix ethtool -A with attached PHYs"). The solution there was to
> simply not call phy_set_asym_pause() under the &pl->state_mutex. That
> cannot be extended to our case though, where the phy_config_inband()
> call is much deeper inside the &pl->state_mutex section.
> 
> Fixes: 5fd0f1a02e75 ("net: phylink: add negotiation of in-band capabilities")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>

Thanks!

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

Powered by blists - more mailing lists