Re: [PATCH] net: phy: only remove owned topology entries

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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 is titled "only remove owned topology entries", but it
  only adds the ownership check to phy_link_topo_del_phy().

--

net: phy: only remove owned topology entries

This commit adds an ownership check to phy_link_topo_del_phy(). The PHY
entry and its ports are now removed only when the entry at phy->phyindex
belongs to the PHY being detached. It covers the case where
phy_link_topo_add_phy() fails because the saved index is already in use,
and the attach error path then runs the normal detach code.

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 214ccce23cf69..175eef44cf545 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -185,6 +185,10 @@ void phy_link_topo_del_phy(struct net_device *dev,
>  	if (!topo)
>  		return;
>  
> +	pdn = xa_load(&topo->phys, phy->phyindex);
> +	if (!pdn || pdn->phy != phy)
> +		return;
> +

[Severity: High]
Does this cover every topology entry the detach path removes? The caller
still removes the module port with no ownership check, right after this
function returns:

drivers/net/phy/phy_device.c:phy_detach_internal() {
    ...
		phy_link_topo_del_phy(dev, phydev);
		if (phydev->mod_port)
			phy_link_topo_del_port(dev, phydev->mod_port);
    ...
}

phy_link_topo_del_port() erases whatever is stored at port->id:

drivers/net/phy/phy_link_topology.c:phy_link_topo_del_port() {
    ...
	xa_erase(&topo->ports, port->id);
}

Like phyindex, port->id is kept after removal and reused when the port is
added again. A stale id can collide in the new topology:

drivers/net/phy/phy_link_topology.c:phy_link_topo_add_port() {
    ...
	if (port->id)
		ret = xa_insert(&topo->ports, port->id, port, GFP_KERNEL);
    ...
}

Suppose a PHY's mod_port got its id while the PHY was attached to netdev
A, and the PHY is later attached to netdev B. There seem to be two ways
phy_attach_direct() reaches its error label and then calls
phy_detach_internal():

  phy_attach_direct()
    phy_link_topo_add_phy()    -> -EBUSY, saved phyindex is used in B
    goto error
      phy_detach_internal()
        phy_link_topo_del_phy()   -> now returns early
        phy_link_topo_del_port(dev, phydev->mod_port)
          xa_erase(&topo->ports, port->id)

In this first case, which is the scenario from the commit message,
mod_port was never inserted into B's topology. Can the xa_erase() then
drop some other port of B that happens to sit at the same id?

  phy_attach_direct()
    phy_link_topo_add_phy()    -> succeeds
    phy_link_topo_add_port(dev, phydev->mod_port)
                               -> -EBUSY, another port of B owns the id
    goto error
      phy_detach_internal()
        phy_link_topo_del_port(dev, phydev->mod_port)
          xa_erase(&topo->ports, port->id)

In the second case, is the other port's entry removed the same way?

In both cases an entry this PHY does not own is removed from the topology,
which is the problem the subject line says is fixed.

Would it be better to put the ownership check in phy_link_topo_del_port()
itself? For example, erase only when xa_load() returns this port, or use
xa_cmpxchg(&topo->ports, port->id, port, NULL, ...). That would cover
every caller, including the ports loop in phy_link_topo_del_phy().

Another option is for the attach error path to skip removing mod_port
when it was never inserted.

>  	list_for_each_entry(port, &phy->ports, head)
>  		phy_link_topo_del_port(dev, port);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028050312.126508-1-yaojiale02%40163.com