net: phylink: add lock for serializing concurrent pl->phydev writes with resolver

Currently phylink_resolve() protects itself against concurrent
phylink_bringup_phy() or phylink_disconnect_phy() calls which modify
pl->phydev by relying on pl->state_mutex.

The problem is that in phylink_resolve(), pl->state_mutex is in a lock
inversion state with pl->phydev->lock. So pl->phydev->lock needs to be
acquired prior to pl->state_mutex. But that requires dereferencing
pl->phydev in the first place, and without pl->state_mutex, that is
racy.

Hence the reason for the extra lock. Currently it is redundant, but it
will serve a functional purpose once mutex_lock(&phy->lock) will be
moved outside of the mutex_lock(&pl->state_mutex) section.

Another alternative considered would have been to let phylink_resolve()
acquire the rtnl_mutex, which is also held when phylink_bringup_phy()
and phylink_disconnect_phy() are called. But since phylink_disconnect_phy()
runs under rtnl_lock(), it would deadlock with phylink_resolve() when
calling flush_work(&pl->resolve). Additionally, it would have been
undesirable because it would have unnecessarily blocked many other call
paths as well in the entire kernel, so the smaller-scoped lock was
preferred.

Link: https://lore.kernel.org/netdev/aLb6puGVzR29GpPx@shell.armlinux.org.uk/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Link: https://patch.msgid.link/20250904125238.193990-1-vladimir.oltean@nxp.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index c7cb95aa80074ad8b7d6be16ad729ba1052bc896..aa17ad2622fc7c7b74f8c69cce0ee7c29e2119fd 100644 (file)
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -67,6 +67,8 @@ struct phylink {
        struct timer_list link_poll;
 
        struct mutex state_mutex;
+       /* Serialize updates to pl->phydev with phylink_resolve() */
+       struct mutex phydev_mutex;
        struct phylink_link_state phy_state;
        unsigned int phy_ib_mode;
        struct work_struct resolve;
@@ -1591,8 +1593,11 @@ static void phylink_resolve(struct work_struct *w)
        struct phylink_link_state link_state;
        bool mac_config = false;
        bool retrigger = false;
+       struct phy_device *phy;
        bool cur_link_state;
 
+       mutex_lock(&pl->phydev_mutex);
+       phy = pl->phydev;
        mutex_lock(&pl->state_mutex);
        cur_link_state = phylink_link_is_up(pl);
 
@@ -1626,11 +1631,11 @@ static void phylink_resolve(struct work_struct *w)
                /* If we have a phy, the "up" state is the union of both the
                 * PHY and the MAC
                 */
-               if (pl->phydev)
+               if (phy)
                        link_state.link &= pl->phy_state.link;
 
                /* Only update if the PHY link is up */
-               if (pl->phydev && pl->phy_state.link) {
+               if (phy && pl->phy_state.link) {
                        /* If the interface has changed, force a link down
                         * event if the link isn't already down, and re-resolve.
                         */
@@ -1694,6 +1699,7 @@ static void phylink_resolve(struct work_struct *w)
                queue_work(system_power_efficient_wq, &pl->resolve);
        }
        mutex_unlock(&pl->state_mutex);
+       mutex_unlock(&pl->phydev_mutex);
 }
 
 static void phylink_run_resolve(struct phylink *pl)
@@ -1829,6 +1835,7 @@ struct phylink *phylink_create(struct phylink_config *config,
        if (!pl)
                return ERR_PTR(-ENOMEM);
 
+       mutex_init(&pl->phydev_mutex);
        mutex_init(&pl->state_mutex);
        INIT_WORK(&pl->resolve, phylink_resolve);
 
@@ -2089,6 +2096,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
                     dev_name(&phy->mdio.dev), phy->drv->name, irq_str);
        kfree(irq_str);
 
+       mutex_lock(&pl->phydev_mutex);
        mutex_lock(&phy->lock);
        mutex_lock(&pl->state_mutex);
        pl->phydev = phy;
@@ -2134,6 +2142,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 
        mutex_unlock(&pl->state_mutex);
        mutex_unlock(&phy->lock);
+       mutex_unlock(&pl->phydev_mutex);
 
        phylink_dbg(pl,
                    "phy: %s setting supported %*pb advertising %*pb\n",
@@ -2312,6 +2321,7 @@ void phylink_disconnect_phy(struct phylink *pl)
 
        ASSERT_RTNL();
 
+       mutex_lock(&pl->phydev_mutex);
        phy = pl->phydev;
        if (phy) {
                mutex_lock(&phy->lock);
@@ -2321,8 +2331,11 @@ void phylink_disconnect_phy(struct phylink *pl)
                pl->mac_tx_clk_stop = false;
                mutex_unlock(&pl->state_mutex);
                mutex_unlock(&phy->lock);
-               flush_work(&pl->resolve);
+       }
+       mutex_unlock(&pl->phydev_mutex);
 
+       if (phy) {
+               flush_work(&pl->resolve);
                phy_disconnect(phy);
        }
 }