Re: [PATCH net-next v18 02/10] net: phy: phy_link_topology: Track ports in phy_link_topology

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 1

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] Ports of SFP module PHYs that are connected via PHY_UPSTREAM_PHY
  can be freed while they are still in dev->link_topo->ports, leaving…
- [Medium] phy_link_topo_del_port() erases topo->ports[port->id] without
  checking that the stored entry is this port, and port->id is…
- [Medium] The patch makes two paths touch the same data under locks that
  never overlap.
- [Low] Adding and removing ports in the topology is asymmetric.

--

net: phy: phy_link_topology: Track ports in phy_link_topology

This commit adds a ports xarray to struct phy_link_topology and an id
field to struct phy_port. PHY-driven ports are added to the netdev
topology when phy_link_topo_add_phy() runs. They are removed from it in
phy_link_topo_del_phy() and phy_del_port().

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5b13a74e2fa92..c4d2c03b8282b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1580,6 +1580,9 @@ static void phy_del_port(struct phy_device *phydev, struct phy_port *port)
>  
>  	list_del(&port->head);
>  
> +	if (phydev->attached_dev)
> +		phy_link_topo_del_port(phydev->attached_dev, port);
> +

[Severity: High]

Can this leave freed ports in dev->link_topo->ports for SFP module PHYs?

phy_sfp_connect_phy() adds the module PHY with PHY_UPSTREAM_PHY:

    struct net_device *dev = phydev->attached_dev;

    if (dev)
        return phy_link_topo_add_phy(dev, phy, PHY_UPSTREAM_PHY, phydev);

The new loop in phy_link_topo_add_phy() then inserts all of the module
PHY's ports into the upstream netdev's topology. The module PHY never
goes through phy_attach_direct(), so its attached_dev stays NULL. Because
of that, the check above never calls phy_link_topo_del_port() for it.

One path is an unbind of the module PHY's driver:

phy_remove(M)
  phy_cleanup_ports(M)
    phy_del_port()      <- M->attached_dev is NULL, no xa_erase()
    phy_port_destroy()  <- kfree(port)

Later, phy_sfp_disconnect_phy() -> phy_link_topo_del_phy(dev, M) walks
M->ports. That list is empty by then, so the entry is never erased.

A second path involves MAC drivers that connect the PHY in ndo_open and
disconnect it in ndo_stop (for example mvpp2 or mvneta via phylink). With
a module PHY present, taking the link down runs phy_detach_internal(P) ->
phy_link_topo_del_phy(dev, P), which removes only P's own ports.

When the module is then unplugged, sfp_sm_phy_detach() -> sfp_remove_phy()
-> phy_sfp_disconnect_phy() sees P->attached_dev == NULL and skips
phy_link_topo_del_phy(dev, M). phy_device_remove(M) -> phy_remove() ->
phy_cleanup_ports() then frees M's ports without erasing them.

xa_destroy() doesn't dereference the entries, so this commit alone won't
crash. However, a later patch in this series, "net: ethtool: Introduce
ethtool command to list ports", adds an ETHTOOL_MSG_PORT_GET dump with no
admin permission flag. That dump walks xa_for_each(&dev->link_topo->ports).
Its port_prepare_data() copies port->supported and port->interfaces and
dereferences port->upstream_port.

Wouldn't that dump read freed memory? phy_del_port(),
phy_sfp_disconnect_phy() and phy_link_topo_del_phy() are unchanged at the
end of the series.

>  	phydev->n_ports--;
>  }
>  
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 0462283c80202..28c5bf3a8849a 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c

[ ... ]

> @@ -56,12 +60,45 @@ static struct phy_link_topology *phy_link_topo_get_or_alloc(struct net_device *d
>  	return dev->link_topo;
>  }
>  
> +int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port)
> +{

[ ... ]

> +	/* Attempt to re-use a previously allocated port_id */
> +	if (port->id)
> +		ret = xa_insert(&topo->ports, port->id, port, GFP_KERNEL);
> +	else
> +		ret = xa_alloc_cyclic(&topo->ports, &port->id, port,
> +				      xa_limit_32b, &topo->next_port_index,
> +				      GFP_KERNEL);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_add_port);
> +
> +void phy_link_topo_del_port(struct net_device *dev, struct phy_port *port)
> +{
> +	struct phy_link_topology *topo = dev->link_topo;
> +
> +	if (!topo)
> +		return;
> +
> +	xa_erase(&topo->ports, port->id);

[Severity: Medium]

Can this erase an entry that belongs to a different port?

port->id is deliberately kept across detach and attach. When the id is
reused, phy_link_topo_add_port() calls xa_insert(). That returns -EBUSY if
another port in the target topology already owns the index, and there is
no fallback to a fresh allocation.

In that case phy_link_topo_add_phy() rolls back the ports it inserted and
returns the error. phy_attach_direct() then does:

    err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
    if (err)
        goto error;

The error path calls phy_detach_internal() while attached_dev is still
set:

phy_detach_internal()
  phy_link_topo_del_phy(dev, phydev)
    list_for_each_entry(port, &phy->ports, head)
      phy_link_topo_del_port(dev, port)
        xa_erase(&topo->ports, port->id)

This walks every port in phy->ports, including the port that failed and
any ports that were never inserted. The failing port's id belongs to
another live port.

Wouldn't that live port be removed from the topology while its owner
still exists? Would checking the stored entry, for example with
xa_cmpxchg(&topo->ports, port->id, port, NULL, 0), avoid this?

The same full teardown runs when phy_link_topo_add_phy() fails earlier,
either on pdn allocation -ENOMEM or on a phyindex collision.

On -ENOMEM from xa_alloc_cyclic(), __xa_alloc() may already have written
an index into port->id that was never stored. The port could then carry an
id it doesn't own.

The PHY attach also fails with -EBUSY in the collision case. The code is
unchanged at the end of the series. Later patches add more pre-registered
ports, such as the phylink SFP cage port allocated at phylink_create()
time, and these could collide with kept ids.

> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_del_port);

[ ... ]

> @@ -110,8 +147,20 @@ int phy_link_topo_add_phy(struct net_device *dev,
>  	if (ret < 0)
>  		goto err;
>  
> +	/* Add all the PHY's ports to the topology */
> +	list_for_each_entry(port, &phy->ports, head) {
> +		ret = phy_link_topo_add_port(dev, port);
> +		if (ret)
> +			goto del_ports;
> +	}

[Severity: Low]

Should phy_add_port() also add the port to the topology?

phy_del_port() now calls phy_link_topo_del_port() whenever
phydev->attached_dev is set, but phy_add_port() only does:

    list_add(&port->head, &phydev->ports);

    phydev->n_ports++;

Ports reach topo->ports only through this loop, at attach or connect time.

Consider an attached PHY whose driver is unbound and rebound through
sysfs. phy_remove() leaves attached_dev and the PHY's topology entry in
place, and phy_cleanup_ports() -> phy_del_port() removes the original
ports from the topology.

The rebind then runs phy_probe() -> phy_setup_ports() -> phy_add_port().
That rebuilds phydev->ports but doesn't add the new ports to the topology.
No phy_attach_direct() runs, so the netdev's topology is missing all of
the PHY's ports until the next detach and attach.

The same gap applies to a module PHY whose driver binds after
phy_sfp_connect_phy() has already walked an empty port list.

The comment in phy_link_topo_get_or_alloc() also says:

 *  - If we have a PHY, phy_link_topo_add_phy() will always run first
 *    and trigger the alloc. Only then the ports can be added through
 *    phylib or sfp.

However, there doesn't seem to be a per-port add path in phylib.
phy_add_port() is unchanged at the end of the series.

> +
>  	return 0;
>  
> +del_ports:
> +	list_for_each_entry_continue_reverse(port, &phy->ports, head)
> +		phy_link_topo_del_port(dev, port);
> +
> +	xa_erase(&topo->phys, phy->phyindex);
>  err:
>  	kfree(pdn);
>  	return ret;
> @@ -123,10 +172,14 @@ void phy_link_topo_del_phy(struct net_device *dev,
>  {
>  	struct phy_link_topology *topo = dev->link_topo;
>  	struct phy_device_node *pdn;
> +	struct phy_port *port;
>  
>  	if (!topo)
>  		return;
>  
> +	list_for_each_entry(port, &phy->ports, head)
> +		phy_link_topo_del_port(dev, port);
> +

[Severity: Medium]

Is it safe to walk phy->ports here with only RTNL held?

phy_link_topo_add_phy() and phy_link_topo_del_phy() now walk
phydev->ports under RTNL, from phy_attach_direct() and
phy_detach_internal(). The driver unbind path changes the same list under
device_lock only, without RTNL:

phy_remove()
  phy_cleanup_ports()
    phy_del_port()
      list_del(&port->head)
      phy_link_topo_del_port()   <- xa_erase() on dev->link_topo->ports
    phy_port_destroy()           <- kfree(port)

Suppose an attached PHY is unbound through sysfs while "ip link set down"
runs phy_disconnect() -> phy_detach_internal() -> phy_link_topo_del_phy().
Could this walk then follow a port that phy_cleanup_ports() has just
unlinked and freed?

phy_del_port() also reads phydev->attached_dev with no RTNL and no
READ_ONCE(), then uses it to reach dev->link_topo. Can
phy_detach_internal() clear attached_dev between that check and the use?

A later patch in this series, "net: phy: phy_link_topology: Add a helper
to retrieve ports", says that "the caller must hold rtnl until it's done
with it", and phy_link_topo_get_port() does ASSERT_RTNL(). The xa_erase()
and kfree() in phy_remove() run without RTNL.

Could an ethtool port dump that holds RTNL xa_load() a port that is then
freed underneath it?

>  	pdn = xa_erase(&topo->phys, phy->phyindex);
>  
>  	/* We delete the PHY from the topology, however we don't re-set the

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927133619.955236-1-maxime.chevallier%40bootlin.com