Re: [PATCH net-next v19 05/10] net: phy: Represent PHY-less SFP modules with phy_port

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · 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] The module port's ->supported mask is computed by the new
  phy_caps_linkmode_filter_ifaces(), and the result does not match the…
- [Medium] phy_detach_internal() always calls phy_link_topo_del_port(dev,
  phydev->mod_port) when mod_port is non-NULL, even on the…
- [Medium] phydev->mod_port and phydev->attached_dev are accessed from
  two contexts that share no lock.
- [Medium] The new topology mutations from the SFP
  module_start/module_stop callbacks run without rtnl and without any…
- [Low] phy_add_sfp_mod_port() (phy_device.c) and
  phylink_add_sfp_mod_port() (phylink.c) leave the module port only…

--

net: phy: Represent PHY-less SFP modules with phy_port

This commit creates a phy_port to represent a PHY-less SFP module. The
port is created in the SFP module_start callback and destroyed in
module_stop, for both the phylib (media converter PHY) and phylink
upstreams. It also adds phy_caps_linkmode_filter_ifaces(), which computes
the module port's supported link modes from the cage port's interfaces.

> diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
> index 942d43191561e..558e4df4d63c0 100644
> --- a/drivers/net/phy/phy_caps.c
> +++ b/drivers/net/phy/phy_caps.c
> @@ -445,3 +445,29 @@ u32 phy_caps_mediums_from_linkmodes(unsigned long *linkmodes)

[ ... ]

> +void phy_caps_linkmode_filter_ifaces(unsigned long *to,
> +				     const unsigned long *from,
> +				     const unsigned long *interfaces)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(ifaces_supported) = {};
> +	unsigned int ifaces_caps = 0;
> +	phy_interface_t interface;
> +
> +	for_each_set_bit(interface, interfaces, PHY_INTERFACE_MODE_MAX)
> +		ifaces_caps |= phy_caps_from_interface(interface);
> +
> +	phy_caps_linkmodes(ifaces_caps, ifaces_supported);
> +
> +	linkmode_and(to, from, ifaces_supported);

[Severity: Medium]
Does this filter always clear Autoneg, Pause and Asym_Pause from the
module port's supported mask?

ifaces_supported is built only from link_caps[].linkmodes.
phy_caps_init() fills that array only for link modes that have a valid
speed and duplex. sfp_module_parse_support() always sets these bits in
caps->link_modes:

drivers/net/phy/sfp-bus.c:sfp_module_parse_support() {
    ...
	phylink_set(modes, Autoneg);
	phylink_set(modes, Pause);
	phylink_set(modes, Asym_Pause);
    ...
}

so linkmode_and() drops them. In contrast, phy_caps_medium_get_supported()
keeps these special bits on purpose ("will be masked away by the port
parent"), and phylink_validate_mask_caps() keeps Autoneg and the port
modes. Is it intended that they are dropped here?

There is a second issue. In phylink_add_sfp_mod_port() the interface
mask comes from pl->sfp_cage_port->interfaces, which is
config->supported_interfaces & phylink_sfp_interfaces:

	phy_interface_and(interfaces, pl->sfp_cage_port->interfaces,
			  caps->interfaces);
	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
					interfaces);

This skips the mac_capabilities/PCS validation that
phylink_sfp_config_optical() applies to pl->sfp_support through
phylink_validate_mask(). For example, phy_caps_from_interface(SGMII)
includes 1000HD. A PHY-less 1000BASE-T module could then report
1000baseT_Half on a MAC without MAC_1000HD.

Can the port list modes that phylink will never configure? Later in the
series this mask is reported to userspace as
ETHTOOL_A_PORT_SUPPORTED_MODES through port_fill_reply().

> +}
> +EXPORT_SYMBOL_GPL(phy_caps_linkmode_filter_ifaces);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 7713418a6734e..13eb1d40badb5 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1527,6 +1539,78 @@ static void phy_sfp_link_down(void *upstream)
>  		port->ops->link_down(port);
>  }
>  
> +static int phy_add_sfp_mod_port(struct phy_device *phydev)
> +{
> +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> +	const struct sfp_module_caps *caps;
> +	struct phy_port *port;
> +	int ret = 0;
> +
> +	/* Create mod port */
> +	port = phy_port_alloc();
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->active = true;

[Severity: Low]
Is this port initialized enough before it is published to the topology?

Only ->active and ->supported are set here, and the same is true of
phylink_add_sfp_mod_port(). ->mediums and ->pairs stay 0, because
neither phy_caps_mediums_from_linkmodes() nor
phy_port_update_supported() is called, and caps->port from the EEPROM
is not used.

With empty mediums, phy_port_get_type() would return PORT_OTHER for a
fibre or DAC module, and phy_port_is_fiber() and phy_port_is_copper()
would both return false. phy_port_restrict_mediums() documents:

	/* We forbid ending-up with a port with empty mediums */

and phy_default_setup_single_port() does:

	port->mediums = phy_caps_mediums_from_linkmodes(port->supported);

->parent_type is also left at its zero value, PHY_PORT_PHY, while ->phy
stays NULL. port_phydev() therefore returns NULL for a port that claims
a PHY parent. phy_setup_sfp_port() sets:

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

In the phylink case there is no PHY parent at all.

Nothing reads these fields on the mod port yet. net/ethtool/port.c
port_prepare_data() only uses supported, interfaces, id, is_mii, is_sfp
and upstream_port, so this is latent for now.

Should the mediums be derived from the supported modes, and the parent
fields be set consistently?

> +
> +	caps = sfp_get_module_caps(phydev->sfp_bus);
> +
> +	phy_interface_and(interfaces, phydev->sfp_cage_port->interfaces,
> +			  caps->interfaces);
> +	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
> +					interfaces);
> +
> +	if (phydev->attached_dev) {
> +		ret = phy_link_topo_add_port(phydev->attached_dev, port);
> +		if (ret) {
> +			phy_port_destroy(port);
> +			return ret;
> +		}
> +	}
> +
> +	/* we don't use phy_add_port() here as the module port isn't a direct
> +	 * interface from the PHY, but rather an extension to the sfp-bus, that
> +	 * is already represented by its own phy_port
> +	 */
> +	phydev->mod_port = port;
> +
> +	return 0;
> +}
> +
> +static void phy_del_sfp_mod_port(struct phy_device *phydev)
> +{
> +	if (!phydev->mod_port)
> +		return;
> +
> +	if (phydev->attached_dev)
> +		phy_link_topo_del_port(phydev->attached_dev, phydev->mod_port);
> +
> +	phy_port_destroy(phydev->mod_port);
> +	phydev->mod_port = NULL;

[Severity: Medium]
What serializes phydev->mod_port and phydev->attached_dev between this
SFP state machine path and phy_attach_direct()/phy_detach_internal()?

phy_sfp_module_start() and phy_sfp_module_stop() run under sm_mutex, and
under rtnl when called from sfp_timeout() or sfp_check_state().
phy_attach_direct() and phy_detach_internal() do not assert rtnl, and
driver .remove() paths often call phy_disconnect() without it.

phy_disconnect() only calls phy_stop() when phy_is_started() is true.
For a PHY in PHY_ERROR, the SFP upstream therefore stays started and
mod_port is still alive when phy_detach() runs. If the module is pulled
at the same time:

CPU1 (driver .remove)              CPU2 (SFP state machine)
phy_detach_internal()
  if (phydev->mod_port)
                                   sfp_check_state()
                                     phy_sfp_module_stop()
                                       phy_del_sfp_mod_port()
                                         phy_port_destroy(phydev->mod_port);
                                         phydev->mod_port = NULL;
    phy_link_topo_del_port(dev, phydev->mod_port);
      xa_erase(&topo->ports, port->id);

Could this read port->id from the freed port and erase an arbitrary
index?

The add side has a similar window. phy_add_sfp_mod_port() checks
attached_dev and only then publishes mod_port, while phy_attach_direct()
sets attached_dev and then checks mod_port. Depending on the
interleaving, the port is either missing from the topology or added
twice. In the second case xa_insert() returns -EBUSY and the attach
fails.

> +}
> +
> +static int phy_sfp_module_start(void *upstream)
> +{

[ ... ]

> +static void phy_sfp_module_stop(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +
> +	/* Called upon module removal or upstream removal */
> +	if (!phydev->has_sfp_mod_phy)
> +		phy_del_sfp_mod_port(phydev);

[Severity: Medium]
Can this erase and free a topology port without rtnl held?

phy_stop() takes only phydev->lock and runs the SFP stop synchronously:

phy_stop()
  sfp_upstream_stop()
    sfp_stop()
      sfp_sm_event(SFP_E_DEV_DOWN)
        sfp_sm_main()
          sfp_module_stop()
            phy_sfp_module_stop()
              phy_del_sfp_mod_port()
                phy_link_topo_del_port()
                phy_port_destroy()

phylib MAC drivers often call phy_stop() from .suspend(), or through
phy_disconnect() in .remove(), without rtnl. phy_link_topo_add_phy()
says "rtnl_lock is what currently protects the PHY topology", and
phy_link_topo_get_or_alloc() describes the SFP state machine as "rtnl
held or not".

phy_port_destroy() is a plain kfree(). An ETHTOOL_MSG_PORT_GET dump
holding rtnl (port_dump_one_dev() walking topo->ports, added later in
the series) could still be using the port when it is freed.

phylink_add_sfp_mod_port() and phylink_del_sfp_mod_port() also call
phy_link_topo_add_port() and phy_link_topo_del_port() without any rtnl
assertion. phylink_create_sfp_cage_port() in this same patch wraps the
same call:

	rtnl_lock();
	ret = phy_link_topo_add_port(pl->netdev, port);
	rtnl_unlock();

Should these paths assert or take rtnl?

> +}
> +

[ ... ]

> @@ -1744,6 +1833,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
>  		phy_link_topo_del_phy(dev, phydev);
> +		if (phydev->mod_port)
> +			phy_link_topo_del_port(dev, phydev->mod_port);
>  	}
>  
>  	phydev->phy_link_change = NULL;
> @@ -1876,6 +1967,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  		err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
>  		if (err)
>  			goto error;
> +
> +		if (phydev->mod_port) {
> +			err = phy_link_topo_add_port(dev, phydev->mod_port);
> +			if (err)
> +				goto error;

[Severity: Medium]
If phy_link_topo_add_port() fails here, or phy_link_topo_add_phy() fails
just above, goto error lands in phy_detach_internal(). That function
calls phy_link_topo_del_port(dev, phydev->mod_port) even though mod_port
was never inserted into dev's topology.

Can this remove some other port from the topology?

phy_link_topo_del_port() does a blind xa_erase(&topo->ports, port->id).
mod_port and its id survive a detach, for example for a PHY in PHY_ERROR
where phy_disconnect() skips phy_stop(), or when phy_detach() is called
on a started PHY. phy_link_topo_add_port() reuses a non-zero id with
xa_insert().

Suppose the PHY is then attached to a different netdev whose topology
already has a port at that index. xa_insert() returns -EBUSY, and the
error path erases that unrelated live port.

phy_link_topo_add_phy() only undoes the ports it actually added, using
list_for_each_entry_continue_reverse(). Should the mod_port unwind also
track whether its insert succeeded?

> +		}
>  	}
>  
>  	/* Some Ethernet drivers try to connect to a PHY device before

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