Re: [PATCH RESEND net-next v17 03/10] net: phylink: Register a phy_port for MAC-driven SFP cages

[netdev-bot+sashiko@kernel.org]

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

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

- [High] phylink now mutates the rtnl-protected PHY link topology from
  contexts that explicitly do not hold rtnl, and frees a topology-…
- [Low] In `phylink_create_sfp_cage_port()`
  (drivers/net/phy/phylink.c:1773-1800) the newly allocated `struct…
- [Low] Concern (invalidated): Documentation/networking/phy-port.rst
  'Next steps' still claims that only PHY-controlled ports are supported…

--

net: phylink: Register a phy_port for MAC-driven SFP cages

This adds a phy_port object representing a MAC-driven SFP cage, allocated
and registered into the netdev's PHY link topology from
phylink_register_sfp(), and removed/freed from phylink_destroy(). The port
is only created for phylink instances that have a netdev.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 390d3eb3e6f7e..bc81ce7f766fa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1767,6 +1770,46 @@ static void phylink_fixed_poll(struct timer_list *t)
>  
>  static const struct sfp_upstream_ops sfp_phylink_ops;
>  
> +static int phylink_create_sfp_cage_port(struct phylink *pl)
> +{
> +	struct phy_port *port;
> +	int ret = 0;
> +
> +	if (!pl->netdev || !pl->sfp_bus)
> +		return 0;
> +
> +	port = phy_port_alloc();
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->is_sfp = true;
> +	port->is_mii = true;
> +	port->active = true;

[Severity: Low]
Should this port also describe its parent?

phy_port_alloc() uses kzalloc_obj(), and enum phy_port_parent in
include/linux/phy_port.h currently only has PHY_PORT_PHY, which is 0:

	enum phy_port_parent {
		PHY_PORT_PHY,
	};

So this cage port ends up self-describing as "driven by a PHY device" while
port->phy stays NULL. Every other producer sets both fields together, for
example in phy_device.c:

	port->parent_type = PHY_PORT_PHY;
	port->phy = phydev;

and port_phydev() returns port->phy unconditionally, so a consumer keying
off parent_type would resolve this port to a NULL phy_device. Nothing
dereferences it today since this port has ops == NULL and the ethtool
reader added later in the series does not look at parent_type, but would it
make sense to add an enum value for a MAC/phylink-owned port (something
like PHY_PORT_MAC) and set it here, rather than leaving the zero value
meaning "PHY parent"?

> +
> +	phy_interface_and(port->interfaces, pl->config->supported_interfaces,
> +			  phylink_sfp_interfaces);
> +	phy_port_update_supported(port);
> +
> +	ret = phy_link_topo_add_port(pl->netdev, port);
> +	if (ret)
> +		phy_port_destroy(port);
> +	else
> +		pl->sfp_cage_port = port;
> +
> +	return ret;
> +}
> +
> +static void phylink_destroy_sfp_cage_port(struct phylink *pl)
> +{
> +	if (pl->netdev && pl->sfp_cage_port)
> +		phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);
> +
> +	if (pl->sfp_cage_port)
> +		phy_port_destroy(pl->sfp_cage_port);
> +
> +	pl->sfp_cage_port = NULL;
> +}
> +

[Severity: High]
What serializes this xa_erase() plus kfree() against topology readers?

phy_link_topo_del_port() erases the port from dev->link_topo->ports and
phy_port_destroy() immediately kfree()s it, with no rtnl, no RCU grace
period and no refcount. The topology is otherwise documented as being
rtnl-protected: phy_link_topo_add_phy() states "rtnl_lock is what currently
protects the PHY topology", and the reader added later in this series
carries an ASSERT_RTNL() plus "Caller must hold RTNL while handling the
phy_port".

The free side runs from phylink_destroy(), whose own kernel-doc says the
opposite:

drivers/net/phy/phylink.c:
	/*
	 * Note: the rtnl lock must not be held when calling this function.
	 */
	void phylink_destroy(struct phylink *pl)
	{
		sfp_bus_del_upstream(pl->sfp_bus);
		phylink_destroy_sfp_cage_port(pl);

Can this then race with an ethtool netlink reader on a live netdev?

CPU0 (ETHTOOL_MSG_PORT_GET, rtnl held)
net/ethtool/port.c:port_prepare_data()
	port = phy_link_topo_get_port(req_info->dev, request->port_id);
	...
	linkmode_copy(reply->supported, port->supported);

CPU1 (dpaa2 DPNI "endpoint changed" threaded IRQ, no rtnl)
dpaa2_eth_disconnect_mac() -> dpaa2_mac_disconnect() -> phylink_destroy()
	phylink_destroy_sfp_cage_port()
		phy_link_topo_del_port()	/* xa_erase */
		phy_port_destroy()		/* kfree */

dpaa2_mac_disconnect() drops rtnl before calling phylink_destroy():

drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c:
	rtnl_lock();
	phylink_disconnect_phy(mac->phylink);
	rtnl_unlock();

	phylink_destroy(mac->phylink);

and MAC-driven SFP cages on dpmac nodes do exist in tree, e.g.
fsl-ls1088a-ten64.dts has "&dpmac2 { sfp = <&sfp_xg0>; }", so pl->sfp_bus
is non-NULL and the cage port really is created and destroyed on that
runtime connect/disconnect path.

There is a second, related question on the add side: phylink_create() also
runs without rtnl in that driver, and phy_link_topo_get_or_alloc() does an
unlocked check-then-allocate with a plain store of dev->link_topo:

drivers/net/phy/phy_link_topology.c:
	if (dev->link_topo)
		return dev->link_topo;
	...
	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
	topo->next_port_index = 1;

	dev->link_topo = topo;

Is there something that prevents an rtnl-holding reader from observing
dev->link_topo before the xarrays are initialized, given there is no lock
and no release barrier on the publish? The comment in that function argues
races cannot happen based on ordering of PHY versus SFP registration, but
that reasoning does not seem to cover a concurrent reader on an
already-registered netdev.

>  static int phylink_register_sfp(struct phylink *pl,
>  				const struct fwnode_handle *fwnode)
>  {

[ ... ]

> @@ -2069,6 +2121,7 @@ EXPORT_SYMBOL_GPL(phylink_create);
>  void phylink_destroy(struct phylink *pl)
>  {
>  	sfp_bus_del_upstream(pl->sfp_bus);
> +	phylink_destroy_sfp_cage_port(pl);
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>

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