Re: [PATCH net 2/2] net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

["Karumanchi, Vineeth" <vineeth@amd.com>]

Replying to Sashiko :
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001074718.3944521-1-vineeth.karumanchi%40amd.com

On 10/5/2026 1:37 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 0 · 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] xgmiitorgmii_remove()
>   (drivers/net/phy/xilinx_gmii2rgmii.c:147-149) always writes the driver…
> - [High] The fix relies on every reader of phydev->drv holding
>   phydev->lock, and that is not true.
> 
> --
> 
> net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove
> 
> This adds a remove callback to the GMII-to-RGMII converter. The callback
> restores the attached PHY's original phydev->drv under phydev->lock, so
> phydev->drv no longer points at the devres-freed conv_phy_drv copy after
> the converter is unbound. It also drops the device reference taken by
> of_phy_find_device().
> 
>> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
>> index 61f71e977a574..b9aa5515577b5 100644
>> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
>> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
>> @@ -128,10 +128,29 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
> 
> [ ... ]
> 
>> +static void xgmiitorgmii_remove(struct mdio_device *mdiodev)
>> +{
>> +	struct gmii2rgmii *priv = mdiodev_get_drvdata(mdiodev);
>> +
>> +	/*
>> +	 * The attached PHY is a separate, still-bound device whose state
>> +	 * machine keeps running and dispatches ->read_status / ->set_loopback
>> +	 * under phydev->lock. Restore its original driver under that lock so
>> +	 * the swap cannot race an in-flight dispatch; the restored driver is
>> +	 * the PHY's own static phy_driver, not the devres-freed conv_phy_drv.
>> +	 */
>> +	mutex_lock(&priv->phy_dev->lock);
>> +	priv->phy_dev->drv = priv->phy_drv;
> 
> [Severity: High]
> Is it safe to write priv->phy_drv back here without first checking that
> phydev->drv still points at &priv->conv_phy_drv?
> 
> The only thing the converter holds is the device reference from
> of_phy_find_device(). There is no device link and no module reference on
> the PHY driver. That means the PHY can be unbound or rebound while the
> converter stays bound, and in that case phylib has already changed
> phydev->drv:
> 
> drivers/net/phy/phy_device.c:phy_remove() {
>     ...
> 	phydev->drv = NULL;
> 
> 	return 0;
> }
> 
> A rebind does the same thing: phy_probe() installs a new driver with
> phydev->drv = phydrv.
> 
> If the converter is unbound after that, xgmiitorgmii_remove() writes back
> the stale pointer that xgmiitorgmii_probe() saved. Three outcomes look
> possible.
> 
> First, an unbound PHY gets a non-NULL drv again. That passes the
> "if (!phydev->drv)" guards in phy.c and phy_device.c. The old driver's
> callbacks then run against a phydev->priv that devres already freed. If
> the PHY driver module was unloaded, they run in freed module text.
> 
> Second, if the PHY was rebound to another driver (for example Generic
> PHY), the old driver's ops get installed over the new binding. They then
> see the new driver's phydev->priv.
> 
> Third, if the converter is probed again, it passes the
> !priv->phy_dev->drv check in xgmiitorgmii_probe() and memcpy()s from the
> stale driver struct.
> 
> The comment above assumes the PHY is still bound and that the restored
> driver is the PHY's own phy_driver. Nothing seems to enforce either
> assumption.
> 
> Would it be better to restore only when phydev->drv == &priv->conv_phy_drv,
> and to tie the converter's lifetime to the PHY binding?

OK thanks, I will update in v2.

> 
>> +	mutex_unlock(&priv->phy_dev->lock);
> 
> [Severity: High]
> Does taking phydev->lock here actually prevent the use-after-free on
> conv_phy_drv? The commit message says:
> 
>     Restore the attached PHY's original driver while
>     holding phydev->lock so that the update cannot race with an active PHY
>     callback.
> 
> The lock only excludes readers that also hold phydev->lock, and not every
> reader of phydev->drv does. Once xgmiitorgmii_remove() returns,
> devres_release_all() frees priv, and conv_phy_drv along with it.
> 
> The state machine drops the lock before its suspend step:
> 
> drivers/net/phy/phy.c:phy_state_machine() {
>     ...
> 	mutex_lock(&phydev->lock);
> 	state_work = _phy_state_machine(phydev);
> 	mutex_unlock(&phydev->lock);
> 
> 	_phy_state_machine_post_work(phydev, state_work);
> }
> 
> phy_stop() does the same. _phy_state_machine_post_work() then calls
> phy_suspend(), which copies phydev->drv into a local and dereferences it
> with no lock held:
> 
> drivers/net/phy/phy_device.c:phy_suspend() {
>     ...
> 	const struct phy_driver *phydrv = phydev->drv;
>     ...
> 	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
> 		return -EBUSY;
> 
> 	if (!phydrv->suspend)
> 		return 0;
> 
> 	ret = phydrv->suspend(phydev);
>     ...
> }
> 
> phy_ethtool_get_sset_count() also dereferences the driver before it takes
> the mutex. It is reachable from the ethtool ioctl through
> ethtool_get_phy_stats_phydev():
> 
> drivers/net/phy/phy.c:phy_ethtool_get_sset_count() {
>     ...
> 	if (phydev->drv->get_sset_count &&
> 	    phydev->drv->get_strings &&
> 	    phydev->drv->get_stats) {
> 		mutex_lock(&phydev->lock);
>     ...
> }
> 
> Could this sequence happen?
> 
> CPU0                                  CPU1
> phy_suspend()
>   phydrv = &priv->conv_phy_drv
>                                       xgmiitorgmii_remove()
>                                         mutex_lock(&phydev->lock)
>                                         phydev->drv = priv->phy_drv
>                                         mutex_unlock(&phydev->lock)
>                                       devres_release_all()
>                                         kfree(priv)
>   phydrv->flags / phydrv->suspend
> 
> If so, CPU0 would read freed memory and might call through it. That is the
> same kind of use-after-free this patch is meant to fix.
> 

I agree that phydev->lock does not cover all readers of phydev->drv, so
the concurrent removal race you described remains possible.

The GMII2RGMII driver is unusual in that it substitutes a copied driver
for a separate, still-bound PHY. Given that this behavior is specific to
this converter, I would prefer to avoid introducing phylib changes
solely to address its teardown.


Thanks,
-- 
🙏 Vineeth


pw-bot: cr