Re: [PATCH net-next v18 07/10] net: phy: phy_link_topology: Add a helper to retrieve ports

[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] phy_link_topo_get_port() (include/linux/phy_link_topology.h)
  returns a raw, unreferenced struct phy_port pointer from topo->ports.

--

net: phy: phy_link_topology: Add a helper to retrieve ports

This adds phy_link_topo_get_port(), which returns a phy_port from the
netdev's link topology xarray. Callers must hold RTNL. The commit also
makes phy_link_topo_add_port() warn and return -EOPNOTSUPP for ops-locked
devices.

> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> index 296ee514ba463..f2e493c433fff 100644
> --- a/include/linux/phy_link_topology.h
> +++ b/include/linux/phy_link_topology.h

[ ... ]

> @@ -71,6 +72,24 @@ phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
>  	return NULL;
>  }
>  
> +static inline struct phy_port *
> +phy_link_topo_get_port(struct net_device *dev, u32 port_id)
> +{
> +	struct phy_link_topology *topo = dev->link_topo;
> +
> +	if (!topo)
> +		return NULL;
> +
> +	/* ports in the topo are RTNL protected, but may be accessed under
> +	 * netdev_lock for ops-locked devices. For now keep them under rtnl
> +	 * protection, as no ops-locked devices have phy_port capabilities
> +	 * yet.
> +	 */
> +	ASSERT_RTNL();
> +
> +	return xa_load(&topo->ports, port_id);
> +}

[Severity: High]
No reference is taken on the returned port. The only thing keeping it
alive is RTNL, and ASSERT_RTNL() only warns. The commit message states the
rule as "the caller must hold rtnl until it's done with it".

Do all the paths that erase and free ports hold RTNL?

phy_remove() is the phy_driver .remove callback. It runs from sysfs driver
unbind or MDIO bus device removal, and it does not hold RTNL:

phy_remove()
  phy_sfp_release() / phy_cleanup_ports()
    phy_del_port()
      if (phydev->attached_dev)
          phy_link_topo_del_port(phydev->attached_dev, port);  <- xa_erase()
    phy_port_destroy(port);                                    <- kfree()

This path holds no RTNL and waits for no RCU grace period.

In contrast, phylink_destroy_sfp_cage_port() does wrap its delete in RTNL:

	/* The port may still be accessed under RTNL */
	rtnl_lock();
	phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);
	rtnl_unlock();

The teardown paths in phy_device.c do not do the same.

Later in the series, port_prepare_data() in net/ethtool/port.c adds a
reader. It holds RTNL, calls phy_link_topo_get_port(), and then reads
port->supported, port->interfaces, port->id, port->is_mii and port->is_sfp.
It also follows port->upstream_port->id.

The device_link created in phy_attach_direct() is DL_FLAG_STATELESS, so the
MAC is not unbound first and attached_dev stays set. Could an
ETHTOOL_MSG_PORT_GET that races with a PHY driver unbind then read a freed
struct phy_port?

This still looks present at the end of the series. Would it be possible to
take RTNL around the del and destroy steps in phy_sfp_release() and
phy_cleanup_ports()? Another option would be kfree_rcu() or refcounting for
the port.

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