[PATCH net-next v5 4/5] net: phy: use a dedicated mutex instead of rtnl for PSE control attach

[Carlo Szelinsky <github@szelinsky.de>]

phy_device_register() took rtnl_lock() around phy_try_attach_pse() to
serialise phydev->psec against the PSE controller notifier walk. But an
MDIO bus registered from ndo_init() runs with rtnl already held:

  register_netdevice()			# holds rtnl
    ndo_init() == ltq_etop_init()
      ltq_etop_mdio_init()
        mdiobus_register()
          mdiobus_scan()
            phy_device_register()
              rtnl_lock()		# deadlock

so any such driver (lantiq_etop, sni_ave, netsec) deadlocks on probe.

Replace rtnl with a dedicated phy_pse_lock mutex for the attach, for the
notifier attach/detach walks, and for the ethtool PSE paths that
dereference phydev->psec. A private lock cannot recurse against the
caller's rtnl, so the register path no longer deadlocks, while attach vs
notifier and detach vs ethtool stay mutually excluded.

rtnl also kept the ethtool PSE reads from racing the PSE_UNREGISTERED
detach that frees phydev->psec, so net/ethtool/pse-pd.c takes the same
lock across its phydev->psec accesses; guarding only the phy side would
reopen a use-after-free there. The lock order is
phy_pse_lock -> pse_list_mutex -> pcdev->lock, and the notifier walks
enter at phy_pse_lock and never take rtnl.

Reported-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Closes: https://lore.kernel.org/netdev/bac5e6e9-7358-4ccb-87fc-9c40baa33682@wp.pl/
Tested-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/phy/phy_device.c | 53 +++++++++++++++++++++++++++---------
 include/linux/phy.h          |  2 ++
 net/ethtool/pse-pd.c         | 15 ++++++----
 3 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f5febff4b00b..fa6c3d638b30 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1113,18 +1113,21 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 }
 EXPORT_SYMBOL(get_phy_device);
 
+/* Serialises phydev->psec against the PSE notifier and ethtool, not rtnl. */
+static DEFINE_MUTEX(phy_pse_lock);
+
 /* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
- * Caller must hold rtnl. A missing phandle (-ENOENT) or a not-yet-registered
- * controller (-EPROBE_DEFER) is silent; the notifier retries the latter at
- * PSE_REGISTERED time. Any other error means a broken binding and is warned
- * about, but left non-fatal so the phy still registers.
+ * Caller must hold phy_pse_lock. A missing phandle (-ENOENT) or a
+ * not-yet-registered controller (-EPROBE_DEFER) is silent; the notifier
+ * retries the latter at PSE_REGISTERED time. Any other error means a broken
+ * binding and is warned about, but left non-fatal so the phy still registers.
  */
 static void phy_try_attach_pse(struct phy_device *phydev)
 {
 	struct pse_control *psec;
 	struct device_node *np;
 
-	ASSERT_RTNL();
+	lockdep_assert_held(&phy_pse_lock);
 
 	np = phydev->mdio.dev.of_node;
 	if (!np)
@@ -1146,7 +1149,7 @@ static void phy_try_attach_pse(struct phy_device *phydev)
 
 static int phy_pse_attach_one(struct device *dev, void *data __maybe_unused)
 {
-	ASSERT_RTNL();
+	lockdep_assert_held(&phy_pse_lock);
 
 	if (dev->type != &mdio_bus_phy_type)
 		return 0;
@@ -1161,7 +1164,7 @@ static int phy_pse_detach_one(struct device *dev, void *data)
 	struct phy_device *phydev;
 	struct pse_control *psec;
 
-	ASSERT_RTNL();
+	lockdep_assert_held(&phy_pse_lock);
 
 	if (dev->type != &mdio_bus_phy_type)
 		return 0;
@@ -1181,16 +1184,16 @@ static int phy_pse_notifier_event(struct notifier_block *nb,
 {
 	switch (event) {
 	case PSE_REGISTERED:
-		rtnl_lock();
+		mutex_lock(&phy_pse_lock);
 		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
 				 phy_pse_attach_one);
-		rtnl_unlock();
+		mutex_unlock(&phy_pse_lock);
 		return NOTIFY_OK;
 	case PSE_UNREGISTERED:
-		rtnl_lock();
+		mutex_lock(&phy_pse_lock);
 		bus_for_each_dev(&mdio_bus_type, NULL, data,
 				 phy_pse_detach_one);
-		rtnl_unlock();
+		mutex_unlock(&phy_pse_lock);
 		return NOTIFY_OK;
 	default:
 		return NOTIFY_DONE;
@@ -1201,6 +1204,28 @@ static struct notifier_block phy_pse_notifier __read_mostly = {
 	.notifier_call = phy_pse_notifier_event,
 };
 
+/**
+ * phy_pse_control_lock - hold phydev->psec stable against PSE controller teardown
+ *
+ * The PSE_UNREGISTERED notifier detaches phydev->psec and drops its last
+ * reference. Callers that dereference phydev->psec (the ethtool PSE paths) must
+ * hold this lock across the access so the detach cannot run underneath them.
+ */
+void phy_pse_control_lock(void)
+{
+	mutex_lock(&phy_pse_lock);
+}
+EXPORT_SYMBOL_GPL(phy_pse_control_lock);
+
+/**
+ * phy_pse_control_unlock - release the lock taken by phy_pse_control_lock()
+ */
+void phy_pse_control_unlock(void)
+{
+	mutex_unlock(&phy_pse_lock);
+}
+EXPORT_SYMBOL_GPL(phy_pse_control_unlock);
+
 /* Core registration: add the phy to the MDIO bus. Does not touch rtnl or
  * PSE. phydev->psec is attached by the callers below, after device_add()
  * has made the phy visible on mdio_bus_type, so that a concurrent PSE
@@ -1260,7 +1285,9 @@ int phy_device_register_locked(struct phy_device *phydev)
 	if (err)
 		return err;
 
+	mutex_lock(&phy_pse_lock);
 	phy_try_attach_pse(phydev);
+	mutex_unlock(&phy_pse_lock);
 
 	return 0;
 }
@@ -1280,9 +1307,9 @@ int phy_device_register(struct phy_device *phydev)
 	if (err)
 		return err;
 
-	rtnl_lock();
+	mutex_lock(&phy_pse_lock);
 	phy_try_attach_pse(phydev);
-	rtnl_unlock();
+	mutex_unlock(&phy_pse_lock);
 
 	return 0;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 865b9baddb85..55a0049c6c2b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2162,6 +2162,8 @@ int phy_device_register(struct phy_device *phy);
 int phy_device_register_locked(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 void phy_device_remove(struct phy_device *phydev);
+void phy_pse_control_lock(void);
+void phy_pse_control_unlock(void);
 int phy_get_c45_ids(struct phy_device *phydev);
 int phy_init_hw(struct phy_device *phydev);
 int phy_suspend(struct phy_device *phydev);
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 757c9e0cc856..4edd9a514de1 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -71,7 +71,9 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
+	phy_pse_control_lock();
 	ret = pse_get_pse_attributes(phydev, info->extack, data);
+	phy_pse_control_unlock();
 
 	ethnl_ops_complete(dev);
 
@@ -281,9 +283,12 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
+
+	phy_pse_control_lock();
+
 	ret = ethnl_set_pse_validate(phydev, info);
 	if (ret)
-		return ret;
+		goto out;
 
 	if (tb[ETHTOOL_A_PSE_PRIO]) {
 		unsigned int prio;
@@ -291,7 +296,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		prio = nla_get_u32(tb[ETHTOOL_A_PSE_PRIO]);
 		ret = pse_ethtool_set_prio(phydev->psec, info->extack, prio);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	if (tb[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT]) {
@@ -301,7 +306,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		ret = pse_ethtool_set_pw_limit(phydev->psec, info->extack,
 					       pw_limit);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	/* These values are already validated by the ethnl_pse_set_policy */
@@ -319,11 +324,11 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		 */
 		ret = pse_ethtool_set_config(phydev->psec, info->extack,
 					     &config);
-		if (ret)
-			return ret;
 	}
 
+out:
 	/* Return errno or zero - PSE has no notification */
+	phy_pse_control_unlock();
 	return ret;
 }
 
-- 
2.43.0