Re: [PATCH] net: dsa: microchip: drop the parent MDIO bus reference

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch fixes code that never runs, and the reference leak it
  describes is still live in mainline.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the cached
  dev->parent_mdio_bus pointer is only protected by a struct device…

--

net: dsa: microchip: drop the parent MDIO bus reference

This patch adds a devres action to ksz_mdio_register(). The action calls
put_device() on the parent MDIO bus returned by of_mdio_find_bus(). The
goal is to stop leaking that struct device reference on probe error
paths and when the switch is torn down.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 767c8fe8e819a..ddd821fa0d3e1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c

[ ... ]

> @@ -2431,6 +2442,12 @@ int ksz_mdio_register(struct ksz_device *dev)
>  			goto put_mdio_node;
>  		}
>  
> +		ret = devm_add_action_or_reset(ds->dev,
> +					       ksz_mdio_put_parent_bus,
> +					       parent_bus);
> +		if (ret)
> +			goto put_mdio_node;
> +

[Severity: High]
Can any current chip reach this branch? It sits behind this check in
ksz_mdio_register():

	if (parent_bus_node && !dev->info->phy_side_mdio_supported) {
		...
		ret = -EINVAL;

		goto put_mdio_node;

In ksz_switch_chips[], only the LAN9370-LAN9374 entries set
phy_side_mdio_supported = true. All of them use lan937x_switch_ops, so
their setup path is lan937x_setup()->lan937x_mdio_register(), not
ksz_mdio_register().

ksz_mdio_register() is called only from ksz8_setup(), ksz8463_setup()
and ksz9477_setup(). None of those chips support side MDIO. For them, a
DT with mdio-parent-bus returns -EINVAL before of_mdio_find_bus() is
called, so the new put_device() action is never registered.

Commit 03d10c776802 ("net: dsa: microchip: implement lan937x-specific MDIO
registration") copied the side MDIO logic into lan937x_main.c, and that
copy still takes the reference without dropping it:

drivers/net/dsa/microchip/lan937x_main.c:lan937x_mdio_register() {
    ...
	} else if (parent_bus_node) {
		parent_bus = of_mdio_find_bus(parent_bus_node);
		if (!parent_bus) {
			ret = -EPROBE_DEFER;

			goto put_mdio_node;
		}

		dev->parent_mdio_bus = parent_bus;
	}
    ...
}

Does the parent bus struct device reference still leak there? That
would happen on the later error paths (devm_mdiobus_alloc(),
lan937x_mdio_bus_preinit(), lan937x_create_phy_addr_map(),
ksz_parse_dt_phy_config(), devm_of_mdiobus_register()), on deferred
re-probes, and on every unbind of a LAN937x switch.

ksz_mdio_put_parent_bus() is static to ksz_common.c, so as written
lan937x_mdio_register() cannot use it. Should the fix go into
lan937x_mdio_register() instead? The side MDIO branch in
ksz_mdio_register() looks unreachable and could perhaps be removed.

With Cc: stable and Fixes: 9afaf0eec2ab, this would be backported to
trees both with and without the lan937x split. Would it only have an
effect in the trees without the split?

>  		dev->parent_mdio_bus = parent_bus;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The
commit message says:

  "The child MDIO bus is unregistered before the action runs, so the
  parent bus stays valid for the whole lifetime of the switch."

Does this hold for anything beyond the mii_bus memory? The reference
from of_mdio_find_bus() keeps struct mii_bus allocated. However, there
is no device_link to the parent MDIO controller and no
try_module_get(bus->owner).

The parent controller driver can be unbound through sysfs, or its
module unloaded, while the switch stays bound. PHY accesses on the side
bus would still go through:

ksz_parent_mdio_read()->mdiobus_read_nested()->__mdiobus_read()->bus->read()

__mdiobus_read() in drivers/net/phy/mdio_bus.c does not check
bus->state:

	if (bus->read)
		retval = bus->read(bus, addr, regnum);

Could the parent driver's read or write callback then run against
released devm resources or unloaded module text? That depends on the
specific parent MDIO driver. The same applies to ksz_parent_mdio_write()
and to the pointer cached in lan937x_mdio_register().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103100531.1309946-1-vulab%40iscas.ac.cn