Commit 0ba5b2f2 authored by Vladimir Oltean's avatar Vladimir Oltean Committed by Jakub Kicinski
Browse files

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: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: default avatarRussell King (Oracle) <rmk+kernel@armlinux.org.uk>
Link: https://patch.msgid.link/20250904125238.193990-1-vladimir.oltean@nxp.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent 03e79de4

drivers/net/phy/phylink.c

+16 −3
Original line number Diff line number Diff line
@@ -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);

	}

}