Re: [PATCH net-next v4 3/3] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

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 <corey@xxxxxxxxxxxx>
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@xxxxxxxxx/

> 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.

> @@ -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.