Re: [PATCH net-next v4 3/3] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook
From: Paolo Abeni <pabeni@redhat.com>
Date: 2026-07-03 07:10:46
Also in:
lkml
From: AI Reviewer <redacted> This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/ --- Now I have all the information I need. Let me produce the report. commit da23b745f9e416f319d1edba4054506a8dabbe37 Author: Corey Leavitt [off-list ref] Subject: net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook This patch moves ownership of phydev->psec from fwnode_mdio into the phy subsystem via a PSE notifier, defers the final pse_control_put() from phy_device_remove() to phy_device_release(), and adds a phy_device_register_locked() variant for callers that already hold rtnl. Link: https://lore.kernel.org/netdev/e00048dd-1ed3-40c3-9912-59bccf015ad5@gmail.com/ (local)
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 0615228459ef..f5febff4b00b 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c@@ -223,8 +223,19 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev) static void phy_device_release(struct device *dev) { + struct phy_device *phydev = to_phy_device(dev); + + /* bus_for_each_dev() holds get_device() across each iteration + * step, deferring this release callback until any in-flight PSE + * notifier walk has advanced past this phy. pse_control_put() + * takes pse_list_mutex, so this path must run in sleepable + * context. + */ + might_sleep(); + pse_control_put(phydev->psec); + fwnode_handle_put(dev->fwnode); - kfree(to_phy_device(dev)); + kfree(phydev); }
[High]
Can this combine with the PSE_UNREGISTERED walk to leave a use-after-free
window for phys that have already been device_del()'d but still have an
outstanding reference?
The notifier walks the bus klist:
case PSE_UNREGISTERED:
rtnl_lock();
bus_for_each_dev(&mdio_bus_type, NULL, data,
phy_pse_detach_one);
rtnl_unlock();
bus_for_each_dev() iterates subsys_private's klist_devices, and
device_del() -> bus_remove_device() -> klist_del() unlinks the device
from that klist. A phy that has been removed from the bus but is still
pinned (for example by a netdev that took get_device() via
phy_attach_direct()) is no longer reachable from this walk, so
phy_pse_detach_one() will not clear its phydev->psec.
quoted hunk ↗ jump to hunk
@@ -1152,8 +1299,6 @@ EXPORT_SYMBOL(phy_device_register); void phy_device_remove(struct phy_device *phydev) { unregister_mii_timestamper(phydev->mii_ts); - pse_control_put(phydev->psec); - device_del(&phydev->mdio.dev); /* Assert the reset signal */
[High]
Previously phy_device_remove() performed pse_control_put() synchronously
before device_del(), so the put always ran while phydev->psec still
pointed at a live pcdev. After this change the put is deferred to
phy_device_release(), which only runs when the last device reference is
dropped.
Consider this sequence on two CPUs:
CPU A (phy teardown) CPU B (rmmod PSE driver)
-------------------------------- -----------------------------------
phy_detach()
/* netdev still pins phydev */
phy_device_remove()
device_del() /* off klist */
pse_controller_unregister()
blocking_notifier_call_chain(
PSE_UNREGISTERED)
phy_pse_notifier_event()
bus_for_each_dev(mdio_bus_type,
..., phy_pse_detach_one)
/* phydev not on klist,
* phydev->psec not cleared */
pse_release_pis()
kfree(pcdev->pi)
/* later, last ref dropped */
phy_device_release()
pse_control_put(phydev->psec)
__pse_control_release()
pcdev->pi[psec->id].admin_state_enabled
/* UAF: pcdev->pi already freed */
module_put(psec->pcdev->owner)
Does the commit message paragraph
"The final pse_control_put() of phydev->psec moves from
phy_device_remove() to phy_device_release(), so it runs only after
every reference on the device -- including the bus-iterator
references taken by bus_for_each_dev() in the notifier walk -- has
been dropped."
cover the case where a phy is already off the bus klist before the
notifier fires? bus_for_each_dev() only holds get_device() on devices
it can reach via the klist, so a phy that has been device_del()'d but
is still pinned by some other holder (an attached netdev being the
typical case) is invisible to phy_pse_detach_one().
Similarly, when an MDIO bus is torn down (mdiobus_unregister() walks
its phys and calls phy_device_remove() on each), is there any guarantee
that no consumer is still holding a reference that would carry the
phy past device_del() and into a later phy_device_release()?
If this race is reachable, would it be safer to keep the
pse_control_put() in phy_device_remove() (or somewhere else that runs
before device_del() returns), so the put cannot outlive any
pcdev->pi[] that PSE_UNREGISTERED is about to free?
--
This is an AI-generated review.