[PATCH net v3 3/4] net: phy: serialise attach and detach with PHY driver bind and unbind

[Aleksei Sviridkin]

phy_attach_direct() reads phydev->drv and d->driver with nothing held
against the driver core, then calls into the driver through
phy_init_hw() and phy_resume(). An unbind that runs during an attach
can remove the driver under it, and an attach that runs while a driver
is still probing can call config_init before the driver's probe has
finished.

Found on a Keenetic KN-1012 (MT7981) while testing how a port copes
with its PHY driver coming and going, on an OpenWrt 6.18 kernel.
Unbinding the wan PHY driver through sysfs while "ip link set wan up"
ran oopsed on the first attempt, with nothing added to widen the
window, inside the driver's own calibration code:

  Unable to handle kernel access to user memory outside uaccess
  routines at virtual address 0000000000000098
  Comm: ip
  Call trace:
   tx_amp_fill_result.isra.0+0x90/0x3a0 (P)
   mt798x_phy_calibration+0x164/0x520
   mt798x_phy_config_init+0x470/0x6f8
   phy_init_hw+0x64/0xa0
   phy_attach_direct+0x184/0x380
   phylink_fwnode_phy_connect+0x198/0x27c
   phylink_of_phy_connect+0x18/0x20
   mtk_open+0x38/0xb70
   __dev_open+0xf8/0x1e0

phy_remove() had cleared phydev->drv while config_init was running,
and the driver read phydev->drv->phy_id. No NULL test in phylib reaches
that dereference.

The device lock cannot be taken here: attach may run under rtnl, while
phy_probe() and phy_remove() take rtnl under the device lock, through
the SFP bus of a PHY with a cage and through the netdev LED trigger.
Add a per-PHY mutex and a flag saying that a driver has finished
probing and is not being removed. The driver-core callbacks take the
mutex only to flip the flag; attach and detach hold it while they call
into the driver. An attach that finds a driver bound but the flag clear
refuses with -EAGAIN.

phy_remove() clears the flag before any teardown, so the rest of it can
run without the mutex: an attach that loses the race is refused, and
one that wins holds the mutex until it is done with the driver. The
unbind then goes on to remove the driver from the PHY that attach just
attached; what the consumer does with it after that is not changed
here.

Fixes: 00db8189d984 ("This patch adds a PHY Abstraction Layer to the Linux Kernel, enabling ethernet drivers to remain as ignorant as is reasonable of the connected PHY's design and operation details.")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@xxxxxx>
---
 drivers/net/phy/phy_device.c | 57 +++++++++++++++++++++++++++++++-----
 include/linux/phy.h          |  6 ++++
 2 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 29d65f6cfd59..1355ea78c86f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -801,6 +801,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	dev->max_n_ports = 1;
 
 	mutex_init(&dev->lock);
+	mutex_init(&dev->bind_lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
 
 	/* Request the appropriate module unconditionally; don't
@@ -1729,6 +1730,8 @@ static int phy_sfp_probe(struct phy_device *phydev)
 	return ret;
 }
 
+static int __phy_probe(struct device *dev);
+
 static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 {
 	return phydrv->config_intr && phydrv->handle_interrupt;
@@ -1743,11 +1746,15 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
  *
  * Description: Called by drivers to attach to a particular PHY
  *     device. The phy_device is found, and properly hooked up
- *     to the phy_driver.  If no driver is attached, then a
+ *     to the phy_driver.  If no driver is bound, then a
  *     generic driver is used.  The phy_device is given a ptr to
  *     the attaching device, and given a callback for link status
  *     change.  The phy_device is returned to the attaching driver.
  *     This function takes a reference on the phy device.
+ *
+ * Return: 0 on success, -EAGAIN if a driver is being bound to or
+ * unbound from the PHY, -EBUSY if the PHY is already attached, or
+ * another negative error code.
  */
 int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface)
@@ -1776,6 +1783,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	get_device(d);
 
+	mutex_lock(&phydev->bind_lock);
+
 	/* Assume that if there is no driver, that it doesn't
 	 * exist, and we should use the genphy driver.
 	 */
@@ -1786,22 +1795,28 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			d->driver = &genphy_driver.mdiodrv.driver;
 
 		phydev->is_genphy_driven = 1;
+	} else if (!phydev->bound) {
+		phydev_err(phydev, "driver is binding or unbinding\n");
+		err = -EAGAIN;
+		goto error_unlock;
 	}
 
 	if (!try_module_get(d->driver->owner)) {
 		phydev_err(phydev, "failed to get the device driver module\n");
 		err = -EIO;
-		goto error_put_device;
+		goto error_unlock;
 	}
 	phydev->drv_owner = d->driver->owner;
 
 	if (phydev->is_genphy_driven) {
-		err = d->driver->probe(d);
+		err = __phy_probe(d);
 		if (err >= 0)
 			err = device_bind_driver(d);
 
 		if (err)
 			goto error_module_put;
+
+		phydev->bound = true;
 	}
 
 	phydev->phy_link_change = phy_link_change;
@@ -1878,6 +1893,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phy_resume(phydev);
 
+	mutex_unlock(&phydev->bind_lock);
+
 	/**
 	 * If the external phy used by current mac interface is managed by
 	 * another mac interface, so we should create a device link between
@@ -1890,6 +1907,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	return err;
 
 error:
+	mutex_unlock(&phydev->bind_lock);
 	/* phy_detach() does all of the cleanup below */
 	phy_detach(phydev);
 	return err;
@@ -1899,7 +1917,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	phydev->drv_owner = NULL;
 	phydev->is_genphy_driven = 0;
 	d->driver = NULL;
-error_put_device:
+error_unlock:
+	mutex_unlock(&phydev->bind_lock);
 	put_device(d);
 	if (ndev_owner != bus->owner)
 		module_put(bus->owner);
@@ -1935,7 +1954,11 @@ void phy_detach(struct phy_device *phydev)
 		sysfs_remove_file(&phydev->mdio.dev.kobj,
 				  &dev_attr_phy_standalone.attr);
 
-	phy_suspend(phydev);
+	mutex_lock(&phydev->bind_lock);
+	if (phydev->bound)
+		phy_suspend(phydev);
+	mutex_unlock(&phydev->bind_lock);
+
 	if (dev) {
 		struct hwtstamp_provider *hwprov;
 
@@ -3678,12 +3701,12 @@ struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
 
 /**
- * phy_probe - probe and init a PHY device
+ * __phy_probe - probe and init a PHY device
  * @dev: device to probe and init
  *
  * Take care of setting up the phy_device structure, set the state to READY.
  */
-static int phy_probe(struct device *dev)
+static int __phy_probe(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 	struct device_driver *drv = phydev->mdio.dev.driver;
@@ -3827,10 +3850,30 @@ static int phy_probe(struct device *dev)
 	return err;
 }
 
+static int phy_probe(struct device *dev)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	int err;
+
+	err = __phy_probe(dev);
+	if (err)
+		return err;
+
+	mutex_lock(&phydev->bind_lock);
+	phydev->bound = true;
+	mutex_unlock(&phydev->bind_lock);
+
+	return 0;
+}
+
 static int phy_remove(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 
+	mutex_lock(&phydev->bind_lock);
+	phydev->bound = false;
+	mutex_unlock(&phydev->bind_lock);
+
 	cancel_delayed_work_sync(&phydev->state_queue);
 
 	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev))
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 33a207ac5c30..8903123d25c8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -653,6 +653,8 @@ struct phy_oatc14_sqi_capability {
  * @n_ports: Number of ports currently attached to the PHY
  * @max_n_ports: Max number of ports this PHY can expose
  * @lock:  Mutex for serialization access to PHY
+ * @bind_lock: Serialises attach and detach with driver bind and unbind
+ * @bound: A driver has finished probing and is not being removed
  * @state_queue: Work queue for state machine
  * @link_down_events: Number of times link was lost
  * @shared: Pointer to private data shared by phys in one package
@@ -784,6 +786,10 @@ struct phy_device {
 
 	struct mutex lock;
 
+	/* Serialises attach and detach with bind and unbind */
+	struct mutex bind_lock;
+	bool bound;
+
 	/* This may be modified under the rtnl lock */
 	bool sfp_bus_attached;
 	struct sfp_bus *sfp_bus;
-- 
2.53.0