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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] phylink_create_sfp_cage_port() calls phy_link_topo_add_port()
  without RTNL.
- [Low] phylink_create_sfp_cage_port() publishes a phy_port into the
  netdev's link topology without setting parent_type.

--

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

This commit registers a phy_port for SFP cages that the MAC drives
directly through phylink, and attaches it to the netdev's link
topology. The port is only created for phylink instances that have a
netdev. It is created in phylink_register_sfp() and removed in
phylink_destroy().

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a7d086cdc9b25..19090916af75f 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1767,6 +1770,50 @@ 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]
parent_type is never set here. Should it be?

phy_port_alloc() uses kzalloc, so this port ends up with
parent_type == PHY_PORT_PHY and port->phy == NULL. The only parent
type defined in include/linux/phy_port.h is:

enum phy_port_parent {
	PHY_PORT_PHY,
};

and its kernel-doc says "Indicates that the port is driven by a PHY
device". This looks like the first phy_port that is driven by the MAC
or phylink instead of a PHY.

The other initializers in phy_device.c, such as phy_setup_sfp_port(),
always set the two fields together:

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

Nothing reads parent_type today, so this does not cause a NULL
dereference right now. Would it make sense to add a phylink/MAC
parent type to enum phy_port_parent and set it here? That way the
published port would not claim a PHY parent it lacks.

The same applies to phylink_add_sfp_mod_port() later in the series.

> +
> +	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);

[Severity: Medium]
This runs from phylink_create(), which is documented as "the rtnl lock
must not be held when calling this function". Can this race with
ethtool readers of dev->link_topo?

If the netdev has no topology yet, phy_link_topo_add_port() allocates
and publishes it with a plain store:

drivers/net/phy/phy_link_topology.c:netdev_alloc_phy_link_topology() {
    ...
	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
	topo->next_port_index = 1;

	dev->link_topo = topo;
    ...
}

This store has no RTNL, WRITE_ONCE() or release barrier.

phylink_create() can be called on a netdev that is already registered.
For example, dpaa2 handles a runtime endpoint change like this:

dpni_irq0_handler_thread()
  dpaa2_eth_connect_mac()
    dpaa2_mac_connect()
      phylink_create()               <- no RTNL held
        phylink_register_sfp()
          phylink_create_sfp_cage_port()
            phy_link_topo_add_port()
              netdev_alloc_phy_link_topology()

Only the later phylink_fwnode_phy_connect() call in dpaa2_mac_connect()
is inside rtnl_lock(). Before this patch, that later call was the one
that first allocated the topology, through
phy_attach_direct()->phy_link_topo_add_phy().

The ethtool netlink readers read dev->link_topo and dereference it
while holding RTNL, not via RCU. For example,
ethnl_tsinfo_dump_one_net_topo() does:

	if (!dev->link_topo) {
	...
	xa_for_each_start(&dev->link_topo->phys, ctx->pos_phyindex, pdn,

phy_link_topo_get_phy(), ethnl_req_get_phydev() and the PHY dump in
net/ethtool/netlink.c follow the same pattern.

On arm64, can such a reader see the new pointer before the
kzalloc/xa_init_flags() stores are visible?

The comment in phy_link_topo_get_or_alloc() explains the ordering
between writers, but it does not seem to cover RTNL-holding readers.
Also, phylink_destroy_sfp_cage_port() takes rtnl_lock() around
phy_link_topo_del_port() with the comment "The port may still be
accessed under RTNL". Should the insertion side use the same locking?

> +	if (ret)
> +		phy_port_destroy(port);
> +	else
> +		pl->sfp_cage_port = port;
> +
> +	return ret;
> +}

[ ... ]

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