Re: [PATCH net v2 1/2] net: ethtool: don't match a hwtstamp provider on a device without a PHC

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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] The fix is incomplete.
- [Medium] The patch deals with the 0xFFFFFFFF/-1 aliasing through an
  open-coded check at one comparison site instead of rejecting the value…

--

net: ethtool: don't match a hwtstamp provider on a device without a PHC

This patch adds a phc_index >= 0 check to ethtool_net_get_ts_info_by_phc().
A requested provider index of 0xFFFFFFFF becomes -1 when stored in an int,
and with this check it no longer matches a netdev that reports no PHC.

> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 23db40618fed1..23e8282563cc3 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -980,7 +980,7 @@ int ethtool_net_get_ts_info_by_phc(struct net_device *dev,
>  	if (err)
>  		return err;
>  
> -	if (info->phc_index == hwprov_desc->index &&
> +	if (info->phc_index >= 0 && info->phc_index == hwprov_desc->index &&
>  	    net_support_hwtstamp_qualifier(dev, hwprov_desc->qualifier))
>  		return 0;
>  

[Severity: High]
Can the same -1 == -1 match still happen in the PHY half of the lookup?

ethtool_phy_get_ts_info_by_phc() in net/ethtool/common.c still compares on
equality alone. It does this in the link topology loop:

	xa_for_each(&dev->link_topo->phys, phy_index, pdn) {
		...
		ethtool_init_tsinfo(info);
		err = phy_ts_info(pdn->phy, info);
		...
		if (info->phc_index == hwprov_desc->index)
			return pdn->phy;
	}

It does the same in the dev->phydev fallback:

	if (info->phc_index == hwprov_desc->index)
		return dev->phydev;

ethtool_init_tsinfo() sets phc_index to -1. So a PHY whose ts_info callback
succeeds without reporting a PHC would match a request for 0xFFFFFFFF.

With this change, the netdev matcher always returns -ENODEV for index -1.
tsconfig_set_hwprov_from_desc() then falls back to the PHY matcher:

	ret = ethtool_net_get_ts_info_by_phc(dev, &ts_info, hwprov_desc);
	if (!ret) {
		/* Found */
		source = HWTSTAMP_SOURCE_NETDEV;
	} else {
		phy = ethtool_phy_get_ts_info_by_phc(dev, &ts_info, hwprov_desc);
		...
		source = HWTSTAMP_SOURCE_PHYLIB;
	}

That builds a hwprov with desc.index = -1, source = HWTSTAMP_SOURCE_PHYLIB,
and phydev pointing at the PHY that has no PHC. ethnl_set_tsconfig() then
installs it in dev->hwprov.

Wouldn't a provider still be selected on hardware that has no PHC, just a
PHYLIB one instead of a NETDEV one? The commit message says "only match a
real PHC index".

One in-tree example looks to be ines_ts_info() in drivers/ptp/ptp_ines.c.
It never writes phc_index and returns 0. The INES timestamper is attached
as phy->mii_ts from the device tree in fwnode_mdiobus_register_phy(), so
phy_has_tsinfo() is true for that PHY.

With CONFIG_PTP_1588_CLOCK=n, bcm-phy-ptp and mscc_ptp also install mii_ts
and report ptp_clock_index(NULL), which is -1.

micrel's lan8841_ts_info() can also report -1. However, its probe only sets
phydev->mii_ts when ptp_clock is non-NULL, so it doesn't seem to be a
trigger.

Here is a possible sequence on a MAC with a real PHC N and a PHY with an
INES mii_ts:

  ETHTOOL_MSG_TSCONFIG_SET provider {N, PRECISE}
    dev->hwprov = {N, PRECISE, NETDEV}
  ETHTOOL_MSG_TSCONFIG_SET provider {0xFFFFFFFF, PRECISE}
    ethnl_update_u32() sets hwprov_mod since N != 0xFFFFFFFF
    ethtool_net_get_ts_info_by_phc() returns -ENODEV
    ethtool_phy_get_ts_info_by_phc() matches -1 == -1
    dev->hwprov = {-1, PRECISE, PHYLIB, INES phy}

TSCONFIG GET only emits the provider when index >= 0, so userspace would
not see this selection afterwards.

The next patch in this series, "net: ethtool: let tsconfig reach a
PHY-only timestamp provider", doesn't change either PHY match site or the
policy. It also drops the ndo_hwtstamp_set/get requirement, so more
devices could reach this fallback.

Should the same >= 0 check also go into both PHY match sites?

[Severity: Medium]
Would it be better to reject 0xFFFFFFFF at the netlink boundary instead of
adding an open-coded check at one comparison site?

The policy in net/ethtool/ts.h leaves the index unbounded:

	[ETHTOOL_A_TS_HWTSTAMP_PROVIDER_INDEX] = { .type = NLA_U32 },

Documentation/netlink/specs/ethtool.yaml also describes the index as a
plain u32. ts_parse_hwtst_provider() copies it into the int
hwtstamp_provider_desc.index, where -1 is also the "no provider requested"
sentinel.

This aliasing is still visible in ETHTOOL_MSG_TSINFO_GET after the patch.
tsinfo_parse_request() starts with:

	req->hwprov_desc.index = -1;

A user-supplied index of 0xFFFFFFFF overwrites that with the same value, so
tsinfo_prepare_data() skips the provider lookup:

	if (req->hwprov_desc.index != -1) {
		ret = ethtool_get_ts_info_by_phc(dev, &data->ts_info,
						 &req->hwprov_desc);
		...
	}

It then returns the default provider's tsinfo instead of an error, even
though the request named a provider.

NLA_POLICY_MAX(NLA_U32, INT_MAX) for ETHTOOL_A_TS_HWTSTAMP_PROVIDER_INDEX
would stop -1 from ever arriving from userspace, for both tsconfig SET and
tsinfo GET. It would cover the PHY match sites too, and the bad attribute
would be reported through extack.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923121644.2194183-2-nb%40tipi-net.de