Re: [PATCH net-next v2 2/3] net: mdio: make device map changes hotplug-safe

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] `mdiobus_remove_devices()` can race with concurrent non-dynamic device removals, leading to a double free of the device reference count.
- [Medium] The dynamic removal safety mechanisms introduced in the patch are dead code because `device_remove_dynamic` is never invoked.

Pre-existing issues:
- [High] Accessing `bus->owner` in `phy_detach()` and the error path of `phy_attach_direct()` after potentially freeing the `bus`.
--

--- Patch [2]: [PATCH net-next v2 2/3] net: mdio: make device map changes hotplug-safe ---
Note: The format of this review has been altered to a free-form summary due
to recitation restrictions. A standard inline diff has been omitted.

commit 22975fc157973334c571ff5ec87c6baf39897f09
Author: James Hilliard <[email protected]>
Subject: net: mdio: make device map changes hotplug-safe

This commit adds serialization and safety mechanisms to MDIO device
registration and removal. It aims to protect the device map from concurrent
dynamic firmware changes and bus teardowns by pinning dynamically removed
devices.

[Severity: High]
Does mdiobus_remove_devices() have a race condition with concurrent
non-dynamic device removals?

Looking at mdiobus_remove_devices() in drivers/net/phy/mdio_bus_provider.c,
the loop reads the device pointer from the map and subsequently calls
device_free() without taking a reference or checking for a concurrent
removal's success.

If a concurrent thread performs a non-dynamic removal (like an SFP unplug)
calling phy_device_remove() on the same device, it could successfully
unpublish the device and call phy_device_free().

Then mdiobus_remove_devices() continues and calls
mdiodev->device_remove(mdiodev). This fails internally with -ENODEV in
mdiobus_begin_remove(). However, __phy_device_remove() and device_remove()
swallow this error and return early without indicating failure to the void
returning caller.

Because of this, mdiobus_remove_devices() unconditionally proceeds to call
mdiodev->device_free(mdiodev), resulting in a double free of the device
reference count.

[Severity: Medium]
Are the dynamic removal protections reachable in practice?

The patch introduces mdio_device_remove_dynamic() and
phy_mdio_device_remove_dynamic() intended to pass dynamic = true to the
removal routines.

However, the function pointer device_remove_dynamic declared in
include/linux/mdio.h is never invoked anywhere. The standard APIs continue
to call phy_device_remove() in drivers/net/phy/phy_device.c, which hardcodes
__phy_device_remove(phydev, false). This appears to leave the dynamic
removal safety mechanisms inactive.

[Severity: High]
This is a pre-existing issue, but can accessing bus->owner cause a
use-after-free?

In phy_detach() in drivers/net/phy/phy_device.c, the code calls
put_device(&phydev->mdio.dev) which drops the network driver's reference to
the PHY device.

If the MDIO bus is a separate device that was already unregistered and freed
by its driver, dropping the child PHY device removes the last reference to
the parent bus kobject, causing the bus structure to be freed immediately.

The very next line checks if (ndev_owner != bus->owner), reading from
potentially freed memory.

The same sequence exists in the error_put_device path of
phy_attach_direct(), which calls put_device(d) followed by checking
bus->owner.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2