Re: [PATCH net-next v12 02/15] phylib: Add support for PHYs with broken forced mode

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0 (1)

- [High] NULL pointer dereference of `phydev->drv` in
  phy_ethtool_ksettings_set() (drivers/net/phy/phy.c:1193-1195).
- [Medium] Incomplete gating of PHY_BROKEN_FORCED: the new flag is
  consulted only in phy_ethtool_ksettings_set()…

Pre-existing issues:
- [Low] Concern that PHY_BROKEN_FORCED has no in-tree user, making the
  patch behaviorally inert (dead branch in phy_ethtool_ksettings_set())…

--

phylib: Add support for PHYs with broken forced mode

Adds a PHY_BROKEN_FORCED driver flag and makes
phy_ethtool_ksettings_set() reject AUTONEG_DISABLE requests with
-EOPNOTSUPP for drivers that set it.  The stated goal is to avoid the
WARN and stack trace in the PHY state machine when config_aneg()
returns -EOPNOTSUPP for forced modes.

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index fce9bc7be3304..b83f938fe7c3b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1190,6 +1190,10 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  	      duplex != DUPLEX_FULL)))
>  		return -EINVAL;
>  
> +	if (autoneg == AUTONEG_DISABLE &&
> +	    (phydev->drv->flags & PHY_BROKEN_FORCED))
> +		return -EOPNOTSUPP;
> +

[Severity: High]
Can phydev->drv be NULL here?

The rest of phylib treats a NULL drv as a reachable state while the
phy_device is still attached to a netdev.  phy_remove() in
phy_device.c ends with:

	phydev->drv = NULL;

without detaching the PHY from the netdev, so ndev->phydev and
phydev->attached_dev stay set after a driver unbind through
/sys/bus/mdio_bus/drivers/<drv>/unbind.

The callee a few lines below in this same function guards for exactly
that:

drivers/net/phy/phy.c:_phy_start_aneg() {
	lockdep_assert_held(&phydev->lock);

	if (!phydev->drv)
		return -EIO;
	...
}

and so do phy_config_inband() and phy_ethtool_nway_reset().  The entry
point does not:

drivers/net/phy/phy.c:phy_ethtool_set_link_ksettings() {
	if (!phydev)
		return -ENODEV;

	return phy_ethtool_ksettings_set(phydev, cmd);
}

So for "ethtool -s <dev> autoneg off speed 100 duplex full" after a PHY
driver unbind, does this new test dereference a NULL phydev->drv and turn
the previous -EIO return into an oops?

There is a second aspect: the read is also outside mutex_lock(&phydev->lock),
and phy_remove() clears the pointer without holding phydev->lock or rtnl, so
moving the test under the mutex would not close the window either.  Would an
explicit check be preferable, e.g.

	if (autoneg == AUTONEG_DISABLE && phydev->drv &&
	    (phydev->drv->flags & PHY_BROKEN_FORCED))
		return -EOPNOTSUPP;

or an early "if (!phydev->drv) return -EIO;" matching the convention used
elsewhere in this file?

[Severity: Medium]
Is phy_ethtool_ksettings_set() the only path that commits AUTONEG_DISABLE?

The SIOCSMIIREG handler in phy_mii_ioctl() also writes forced mode with no
PHY_BROKEN_FORCED test:

drivers/net/phy/phy.c:phy_mii_ioctl() {
	...
		case MII_BMCR:
			if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
				if (phydev->autoneg == AUTONEG_ENABLE)
					change_autoneg = true;
				phydev->autoneg = AUTONEG_DISABLE;
	...
		mdiobus_write(phydev->mdio.bus, prtad, devad, val);
	...
		if (change_autoneg)
			return phy_start_aneg(phydev);
}

For a PHY flagged PHY_BROKEN_FORCED whose config_aneg() returns
-EOPNOTSUPP (asix_ax88279_config_aneg(), added later in this series),
phy_start_aneg() -> _phy_start_aneg() -> phy_config_aneg() fails and the
error reaches the ioctl caller, but neither the BMCR write nor
phydev->autoneg = AUTONEG_DISABLE is rolled back.

On the next transition to PHY_UP (ifdown/ifup, link flap, cable test exit),
does the state machine then hit the same WARN this patch aims to remove?

drivers/net/phy/phy.c:_phy_state_machine() {
	if (state_work == PHY_STATE_WORK_ANEG) {
		err = _phy_start_aneg(phydev);
		func = &_phy_start_aneg;
	}
	...
	if (err < 0)
		phy_error_precise(phydev, func, err);
}

phy_mii_ioctl() is exported and wired as ndo_eth_ioctl by a number of MAC
drivers, directly or via phy_do_ioctl_running().  For the device in this
series the MAC routes ndo_eth_ioctl to phylink_mii_ioctl(), whose
SIOCSMIIREG case uses phylink_phy_write() and does not touch
phydev->autoneg, so that particular combination does not reach the WARN
today.  Would placing the gate where autoneg is actually committed, in
_phy_start_aneg() or phy_config_aneg(), cover both entry points?

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 5f8d65868e0ff..d5f8e527ce8df 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -64,6 +64,7 @@ extern const int phy_basic_ports_array[3];
>  #define PHY_RST_AFTER_CLK_EN	0x00000002
>  #define PHY_POLL_CABLE_TEST	0x00000004
>  #define PHY_ALWAYS_CALL_SUSPEND	0x00000008
> +#define PHY_BROKEN_FORCED	0x00000010

[Severity: Low]
This isn't a bug introduced by this patch, but a note on flag adoption.

The value 0x10 does not collide with any existing PHY_* flag, and there is
an in-tree user by the end of the series: drivers/net/phy/ax88796b.c sets

	.flags		= PHY_IS_INTERNAL | PHY_BROKEN_FORCED,

on the AX88279 entry, whose asix_ax88279_config_aneg() returns -EOPNOTSUPP
for AUTONEG_DISABLE, so the new branch is not dead code.

Two existing drivers already reject forced mode in their config_aneg()
without setting the new flag, bcm84881_config_aneg() and
en8811h_config_aneg().  That is pre-existing behaviour and not changed
here; would converting them be a reasonable follow-up?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-ax88179a-v12-0-60c04c9924a2%40birger-koblitz.de