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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · 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] Most concerning: the RTNL-only lifetime rule that this patch
  sets for phy_link_topo_get_port() is broken by the phylib teardown…
- [High] net: phy: NULL pointer dereference in netdev_need_ops_lock()

--

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

This adds phy_link_topo_get_port() so netlink can look up phy_ports
stored in the link topology. Callers must hold rtnl while they use the
returned port. phy_link_topo_add_port() now also warns and refuses to add
ports on 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]
Is RTNL really enough to keep the returned port alive?

This helper takes no reference on the port. It relies on every writer
holding RTNL when it erases and frees ports from topo->ports. The phylib
teardown path does not seem to do that.

phy_remove() is the driver-core remove callback and runs without RTNL:

drivers/net/phy/phy_device.c:phy_remove() {
    ...
	phy_sfp_release(phydev);
	phy_cleanup_ports(phydev);
    ...
}

Both of these go through phy_del_port() and then free the port:

drivers/net/phy/phy_device.c:phy_cleanup_ports() {
	list_for_each_entry_safe(port, tmp, &phydev->ports, head) {
		phy_del_port(phydev, port);
		phy_port_destroy(port);
	}
}

drivers/net/phy/phy_device.c:phy_del_port() {
    ...
	if (phydev->attached_dev)
		phy_link_topo_del_port(phydev->attached_dev, port);
    ...
}

phy_link_topo_del_port() does the xa_erase(), and phy_port_destroy() does
the kfree(). phy_sfp_release() takes and drops RTNL inside
sfp_bus_del_upstream(). Its later erase and free therefore also run
without RTNL.

phy_remove() can run while attached_dev is still set. One case is a sysfs
unbind of the PHY driver: try_module_get() in phy_attach_direct() only
blocks rmmod. The other is removal of the PHY device itself: the
device_link created in phy_attach_direct() is DL_FLAG_STATELESS, so it
does not force the MAC to unbind first.

Later in the series, port_prepare_data() in net/ethtool/port.c holds RTNL
and uses this helper:

	port = phy_link_topo_get_port(req_info->dev, request->port_id);
	if (!port)
		return -ENODEV;

	linkmode_copy(reply->supported, port->supported);

It then reads port->interfaces, port->id, port->is_mii, port->is_sfp and
port->upstream_port->id.

Could a PORT_GET doit or dump run concurrently with phy_remove() and
read the struct phy_port after phy_port_destroy() has freed it?

The phylink side already follows the intended rule:

drivers/net/phy/phylink.c:phylink_destroy_sfp_cage_port() {
    ...
		/* The port may still be accessed under RTNL */
		rtnl_lock();
		phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);
		rtnl_unlock();
    ...
}

The phy_device.c delete paths do not follow it. That includes the
mod_port removal in phy_detach_internal().

Should phy_del_port() and its callers take RTNL, or at least assert it,
around the topology removal and the free? This also appears to be
unchanged at the end of the series.

> +}
> +

[ ... ]

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -65,6 +65,14 @@ int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port)
>  	struct phy_link_topology *topo;
>  	int ret;
>  
> +	/* Ports can now be queried without rtnl for ops-locked devices, which
> +	 * we don't support now as port retrieval is done under rtnl.
> +	 * We don't have phy_port enabled devices yet, let's make sure
> +	 * we are loudly warned about that when it happens.
> +	 */
> +	if (WARN_ON_ONCE(dev->netdev_ops && netdev_need_ops_lock(dev)))
> +		return -EOPNOTSUPP;
> +
>  	topo = phy_link_topo_get_or_alloc(dev);
>  	if (IS_ERR(topo))
>  		return PTR_ERR(topo);

[Severity: High]
The dev->netdev_ops test here is needed because netdev_need_ops_lock()
dereferences dev->netdev_ops->net_shaper_ops when CONFIG_NET_SHAPER is
enabled. Doesn't phy_link_topo_add_phy() in the same file need the same
guard?

As far as I can see it does this, with no check on dev->netdev_ops:

	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
		return -EOPNOTSUPP;

phy_attach_direct() calls phy_link_topo_add_phy() for whatever
net_device the PHY is attached to. Some in-tree users attach a PHY to
a net_device that never gets netdev_ops. For example, gbe_probe() in
drivers/net/ethernet/ti/netcp_ethss.c passes gbe_dev->dummy_ndev to
of_phy_connect(). With CONFIG_NET_SHAPER=y that is a NULL pointer
dereference during probe.

Should phy_link_topo_add_phy() use the same
dev->netdev_ops && netdev_need_ops_lock(dev) test as
phy_link_topo_add_port()? Alternatively, netdev_need_ops_lock() could
tolerate a NULL netdev_ops itself, so callers don't each have to
remember the check. Which do you prefer?

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