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

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

[Jiale Yao]

phy_link_topo_add_phy() can fail to insert a PHY when its saved index is
already occupied.  Its caller may subsequently run the normal detach
path.  phy_link_topo_del_phy() then unconditionally removes the entry at
that index, including its ports, even though the entry belongs to a
different PHY.

Look up the topology entry first and only remove the PHY and its ports
when the entry belongs to the PHY being detached.

Fixes: 384968786909 ("net: phy: Introduce ethernet link topology representation")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/phy/phy_link_topology.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 214ccce23cf6..175eef44cf54 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;
+
 	list_for_each_entry(port, &phy->ports, head)
 		phy_link_topo_del_port(dev, port);
 
-- 
2.34.1

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Maxime Chevallier]

On 10/28/26 06:03, Jiale Yao wrote:
> phy_link_topo_add_phy() can fail to insert a PHY when its saved index is
> already occupied.

It's unclear to me how this can happen in real life...

>  Its caller may subsequently run the normal detach
> path.  phy_link_topo_del_phy() then unconditionally removes the entry at
> that index, including its ports, even though the entry belongs to a
> different PHY.
> 
> Look up the topology entry first and only remove the PHY and its ports
> when the entry belongs to the PHY being detached.

Maxime

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

[Andrew Lunn]

On Wed, Oct 28, 2026 at 01:03:12PM +0800, Jiale Yao wrote:
> phy_link_topo_add_phy() can fail to insert a PHY when its saved index is
> already occupied.

How did you get into this state? It indicates there is another bug
somewhere which needs fixing. Fixing that bug is much more important.

	  Andrew

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