On 6/8/23 12:13, Andrew Lunn wrote:
On Thu, Jun 08, 2023 at 10:33:30AM -0700, Florian Fainelli wrote:
On 6/7/23 18:30, Andrew Lunn wrote:
(gdb) print /x (int)&((struct phy_driver *)0)->led_brightness_set
$1 = 0x1f0
so this would indeed look like an use-after-free here. If you tested with a
PHYLINK enabled driver you might have no seen due to
phylink_disconnect_phy() being called with RTNL held?
Yes, i've been testing with mvneta, which is phylink.
Humm, this is really puzzling because we have the below call trace as to
where we call schedule_work() which is in led_set_brightness_nopm() however
we have led_classdev_unregister() call flush_work() to ensure the workqueue
completed. Is there something else in that call stack that prevents the
system workqueue from running?
Has phy_remove() already been called? Last thing it does is:
phydev->drv = NULL;
This is one of the differences between my system and yours. With
mvneta, the mdio bus driver is an independent device. You have a
combined MAC and MDIO bus driver.
Yes, good point. I did change to the patch below, however that still
triggers an delayed led_brightness_set call which now gets scheduled
*after* we removed the MDIO bus controller and shutdown the MDIO bus clock:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2cad9cc3f6b8..f838c4f92524 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3053,7 +3053,7 @@ static int of_phy_led(struct phy_device *phydev,
init_data.fwnode = of_fwnode_handle(led);
init_data.devname_mandatory = true;
- err = devm_led_classdev_register_ext(dev, cdev, &init_data);
+ err = led_classdev_register_ext(dev, cdev, &init_data);
if (err)
return err;
@@ -3298,6 +3298,14 @@ static int phy_probe(struct device *dev)
return err;
}
+static void phy_remove_leds(struct phy_device *phydev)
+{
+ struct phy_led *phyled;
+
+ list_for_each_entry(phyled, &phydev->leds, list)
+ led_classdev_unregister(&phyled->led_cdev);
+}
+
static int phy_remove(struct device *dev)
{
struct phy_device *phydev = to_phy_device(dev);
@@ -3315,6 +3323,8 @@ static int phy_remove(struct device *dev)
/* Assert the reset signal */
phy_device_reset(phydev, 1);
+ phy_remove_leds(phydev);
+
phydev->drv = NULL;
return 0;
--
Florian
