Re: [PATCH RESEND net-next v17 09/10] net: ethtool: Introduce ethtool command to list ports

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 4

- [Medium] ETHTOOL_A_PORT_SUPPORTED_INTERFACES publishes bit indices that
  are the raw values of the kernel-internal `enum phy_interface_t` (0 ..…
- [Medium] `ctx->pos_portid` in struct port_dump_ctx is a per-device sub-
  cursor but is not bound to `ctx->pos_ifindex`, and it is reset only…
- [Medium] port_dump_one_dev() reads and dereferences dev->link_topo and
  walks the RTNL-protected link_topo->ports xarray with no lock at all:…
- [Low] The commit message describes the new command inaccurately in two
  ways.
- [Low] The YAML spec declares the port-get DUMP request as carrying only
  `header` (Documentation/netlink/specs/ethtool.yaml:2912-2916), but the…
- [Low] Concern (dismissed after verification):
  Documentation/networking/phy-port.rst was said to still claim that…
- [Low] The "List of message types" inventory tables in
  Documentation/networking/ethtool-netlink.rst are not updated: at the…

--

net: ethtool: Introduce ethtool command to list ports

This adds a new ETHTOOL_MSG_PORT_GET netlink command that exposes the
phy_port objects registered in a netdev's link topology, with a custom
dump implementation in the new net/ethtool/port.c.  MDI ports report
supported linkmodes, MII ports report supported PHY interface modes.

A couple of questions on the commit message and on the new dump code
below.

[Severity: Low]
The changelog says:

    "For MDI ports, we report the list of supported linkmodes based on
    what the PHY that drives this port says.  For MII ports, i.e. empty
    SFP cages, we report the MII linkmodes that we can output on this
    port."

Are both halves of that accurate for this series?

For the first half, phylink_add_sfp_mod_port() fills the MDI port's
supported mask from the module, not from a PHY:

	caps = sfp_get_module_caps(pl->sfp_bus);
	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
					pl->sfp_cage_port->interfaces);
	...
	port->upstream_port = pl->sfp_cage_port;

For the second half, the cage's MII port stays registered while a module
is inserted (pl->sfp_cage_port lives from sfp bus attach to detach), and
a PHY-less module registers a second, MDI port pointing back at the cage
via upstream_port.  So one populated SFP connector yields two dump
records, which makes "we can know how many ports are available on a given
interface" ambiguous.  Could the wording be adjusted to describe the
cage port and the module port separately?

[Severity: Low]
Should the message-type inventory tables in
Documentation/networking/ethtool-netlink.rst be extended too?  At the end
of the series the userspace-to-kernel table still ends at
``ETHTOOL_MSG_MSE_GET`` and the kernel-to-userspace table at
``ETHTOOL_MSG_MSE_GET_REPLY``, even though the PORT_GET section and the
ioctl translation table in the same file already mention the new command.

> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 1af395b54330e..c076c07d0a080 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -26,6 +26,8 @@ static u32 ethnl_bcast_seq;
>  			     ETHTOOL_FLAG_OMIT_REPLY)
>  #define ETHTOOL_FLAGS_STATS (ETHTOOL_FLAGS_BASIC | ETHTOOL_FLAG_STATS)
>  
> +char phy_interface_names[PHY_INTERFACE_MODE_MAX][ETH_GSTRING_LEN] __ro_after_init;
> +
>  const struct nla_policy ethnl_header_policy[] = {

[ ... ]

> @@ -1594,10 +1606,23 @@ static struct genl_family ethtool_genl_family __ro_after_init = {
>  
>  /* module setup */
>  
> +static void __init ethnl_phy_names_populate(void)
> +{
> +	const char *name;
> +	int i;
> +
> +	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
> +		name = phy_modes(i);
> +		strscpy(phy_interface_names[i], name, ETH_GSTRING_LEN);
> +	}
> +}

[Severity: Medium]
These names are built at init time from phy_modes() and are not exported
through any ETH_SS_* string set.  enum ethtool_stringset in
include/uapi/linux/ethtool.h ends at ETH_SS_TS_FLAGS, and
net/ethtool/strset.c info_template has no entry for them, so userspace
cannot resolve the bit indices via ETHTOOL_MSG_STRSET_GET the way it can
for ETH_SS_LINK_MODES.

Does that make the raw values of enum phy_interface_t part of the uAPI?
See the port_fill_reply() comment below.

> diff --git a/net/ethtool/port.c b/net/ethtool/port.c
> new file mode 100644
> index 0000000000000..0d6150e5f66a2
> --- /dev/null
> +++ b/net/ethtool/port.c
> @@ -0,0 +1,382 @@

[ ... ]

> +const struct nla_policy ethnl_port_get_policy[ETHTOOL_A_PORT_ID + 1] = {
> +	[ETHTOOL_A_PORT_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_PORT_ID] = NLA_POLICY_MIN(NLA_U32, 1),
> +};

[Severity: Low]
The YAML spec declares the port-get dump request as carrying only
"header", but this single policy is shared by both doit and dumpit, so a
dump request that carries ETHTOOL_A_PORT_ID passes validation.
ethnl_port_dump_start() only looks at tb[ETHTOOL_A_PORT_HEADER], so the
port id is silently ignored and every port is returned.

Would it be better to reject it explicitly, the way
ethnl_rss_dump_start() does for its do-only filter attribute?

	/* Filtering by context not supported */
	if (tb[ETHTOOL_A_RSS_CONTEXT]) {
		NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_CONTEXT]);
		return -EINVAL;
	}

[ ... ]

> +static int port_fill_reply(struct sk_buff *skb,
> +			   const struct ethnl_req_info *req_info,
> +			   const struct ethnl_reply_data *reply_data)
> +{

[ ... ]

> +	} else {
> +		ret = ethnl_put_bitset(skb, ETHTOOL_A_PORT_SUPPORTED_INTERFACES,
> +				       reply->interfaces, NULL,
> +				       PHY_INTERFACE_MODE_MAX,
> +				       phy_interface_names, compact);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Medium]
This publishes bit indices that are the raw values of the kernel-internal
enum phy_interface_t, and Documentation/netlink/specs/ethtool.yaml
declares supported-interfaces only as a plain bitset nest with no enum:

        name: supported-interfaces
        type: nest
        nested-attributes: bitset

A client that sets ETHTOOL_FLAG_COMPACT_BITSETS gets only
ETHTOOL_A_BITSET_VALUE, with no way to map bits to names, so it has to
hard-code the enum values.  Does that freeze the ordering of
enum phy_interface_t as ABI?

The enum has been edited in the middle before:
PHY_INTERFACE_MODE_100BASEX sits between TRGMII and 1000BASEX in the
enum while its phy_modes() case is appended near the end, which suggests
it was inserted rather than appended.  Any future insertion would
re-label already-published bits.

Would adding an ETH_SS_* string set for these names (or defining the
values in the YAML spec) avoid that?

[ ... ]

> +static int port_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
> +	struct net_device *dev;
> +	struct phy_port *port;
> +	int ret;
> +
> +	dev = ctx->req_info->base.dev;
> +
> +	if (!dev->link_topo)
> +		return 0;
> +
> +	xa_for_each_start(&dev->link_topo->ports, ctx->pos_portid, port,
> +			  ctx->pos_portid) {
> +		ctx->req_info->port_id = ctx->pos_portid;
> +
> +		ret = port_dump_one(skb, dev, cb);
> +		if (ret == -ENODEV)
> +			continue;
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ctx->pos_portid = 0;
> +
> +	return 0;
> +}

[Severity: Medium]
Can ctx->pos_portid be applied to the wrong device?  It is a per-device
sub-cursor but it is not tied to ctx->pos_ifindex, and it is only reset
after the port loop runs to completion.

In port_dump_all_dev(), the -EMSGSIZE break leaves pos_ifindex pointing
at the interrupted device (for_each_netdev_dump() increments only after
the body) and pos_portid non-zero.  No reference is held on that device
between two ->dumpit() invocations, so if it goes away, xa_find() in
for_each_netdev_dump() returns the next higher ifindex, and this loop
resumes a different device's xarray at the stale index.  Port ids start
at 1:

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;
	...
}

so ports 1..N-1 of that device would be dropped from the reply with no
error reported.

The "if (!dev->link_topo) return 0;" early exit also returns before
"ctx->pos_portid = 0;", so a stale cursor survives every topology-less
device visited in the same invocation.  Should the reset happen at the
top of the function, or should pos_portid be cleared whenever
pos_ifindex changes?

[Severity: Medium]
What protects dev->link_topo and the ports xarray during this walk?
port_dump_all_dev() calls rcu_read_unlock() before invoking this
function, the filtered path in ethnl_port_dumpit() holds nothing, and
RTNL is taken only inside the loop body by port_dump_one().

The accessor added in this same series documents the opposite contract:

include/linux/phy_link_topology.h:phy_link_topo_get_port() {
	/* ports in the topo are RTNL protected, ... */
	ASSERT_RTNL();

	/* Caller must hold RTNL while handling the phy_port */
	return xa_load(&topo->ports, port_id);
}

Two things follow from that.  First, dev->link_topo is published with a
plain store after the xarray is initialised:

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;
	...
}

and read here with a plain load.  Can a first port registration (for
instance phy_attach_direct() or phylink SFP cage creation from ndo_open)
racing with the dump expose a non-NULL topology whose xarray stores are
not yet visible on a weakly-ordered architecture?  At minimum this looks
like a data race KCSAN would flag.

Second, since RTNL is dropped between every port, every netdev and every
recvmsg resumption, ports added or removed mid-dump (for example
phylink_add_sfp_mod_port() / phylink_del_sfp_mod_port() on module
hotplug) can be skipped or repeated.  There is no generation counter and
NLM_F_DUMP_INTR is never set, so userspace cannot detect it.  Would
holding rtnl_lock() across the xa_for_each_start() loop, or snapshotting
the ids under RTNL, be preferable?

For what it is worth, the returned port pointer is never dereferenced
here and port_prepare_data() re-looks-up the id under
rtnl_lock()+netdev_lock_ops(), so a stale id turns into -ENODEV rather
than a use-after-free.  The same lockless pattern also already exists in
ethnl_perphy_dump_one_dev() for link_topo->phys.

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