[PATCH net 0/2] net: phy: xilinx-gmii2rgmii: Fix PHY data ownership and removal

[PATCH net 0/2] net: phy: xilinx-gmii2rgmii: Fix PHY data ownership and removal

[Vineeth Karumanchi]

The Xilinx GMII-to-RGMII converter copies the attached PHY driver and
replaces its read_status and set_loopback callbacks. This series addresses
two problems in that arrangement: overwriting driver data belonging to
the external PHY, and leaving phydev->drv pointing at freed converter
memory after converter removal.

Patch 1 retrieves the converter private data from its embedded phy_driver
using container_of_const(), preserving the external PHY's MDIO driver-data
field. It fixes the overwrite introduced by commit 168f7a161608 ("net: phy:
gmii2rgmii: Dont use priv field in phy device").

Patch 2 stores private data on the converter's own MDIO device and adds a
remove callback. It restores the original PHY driver under phydev->lock
and releases the reference acquired by of_phy_find_device(). This addresses
the stale pointer observed during PHY state-machine polling after
converter-only unbind. The missing removal cleanup dates back to commit
f411a6160bd4 ("net: phy: Add gmiitorgmii converter support"). Apply the
patches in order.

Vineeth Karumanchi (2):
  net: phy: xilinx-gmii2rgmii: Avoid overwriting PHY drvdata
  net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

 drivers/net/phy/xilinx_gmii2rgmii.c | 31 +++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)


base-commit: 7375d38364a9aa66fb31716bcefef38aecad75d8
-- 
2.43.0

[PATCH net 1/2] net: phy: xilinx-gmii2rgmii: Avoid overwriting PHY drvdata

[Vineeth Karumanchi]

The GMII-to-RGMII converter wraps the attached PHY driver by copying
its phy_driver structure and replacing the read_status and set_loopback
callbacks.

To access the converter private data from these callbacks, the driver
currently stores it in the attached PHY's MDIO driver-data field. This
field belongs to the underlying PHY driver and may already contain its
private data. Overwriting it can therefore cause the PHY driver to
retrieve an unexpected pointer and behave incorrectly.

The converter-specific phy_driver is embedded in struct gmii2rgmii and
installed as phydev->drv. Use container_of_const() to retrieve the
enclosing gmii2rgmii structure from phydev->drv instead of using the
PHY's driver-data field.

Update xgmiitorgmii_configure() to accept a const pointer accordingly.

Fixes: 168f7a161608 ("net: phy: gmii2rgmii: Dont use priv field in phy device")
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 2024d8ef36d9..61f71e977a57 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -28,7 +28,7 @@ struct gmii2rgmii {
 	struct mdio_device *mdio;
 };
 
-static void xgmiitorgmii_configure(struct gmii2rgmii *priv, int speed)
+static void xgmiitorgmii_configure(const struct gmii2rgmii *priv, int speed)
 {
 	struct mii_bus *bus = priv->mdio->bus;
 	int addr = priv->mdio->addr;
@@ -49,7 +49,9 @@ static void xgmiitorgmii_configure(struct gmii2rgmii *priv, int speed)
 
 static int xgmiitorgmii_read_status(struct phy_device *phydev)
 {
-	struct gmii2rgmii *priv = mdiodev_get_drvdata(&phydev->mdio);
+	const struct gmii2rgmii *priv = container_of_const(phydev->drv,
+							   struct gmii2rgmii,
+							   conv_phy_drv);
 	int err;
 
 	if (priv->phy_drv->read_status)
@@ -67,7 +69,9 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable,
 				     int speed)
 {
-	struct gmii2rgmii *priv = mdiodev_get_drvdata(&phydev->mdio);
+	const struct gmii2rgmii *priv = container_of_const(phydev->drv,
+							   struct gmii2rgmii,
+							   conv_phy_drv);
 	int err;
 
 	if (priv->phy_drv->set_loopback)
@@ -123,7 +127,6 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
 	       sizeof(struct phy_driver));
 	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
 	priv->conv_phy_drv.set_loopback = xgmiitorgmii_set_loopback;
-	mdiodev_set_drvdata(&priv->phy_dev->mdio, priv);
 	priv->phy_dev->drv = &priv->conv_phy_drv;
 
 	return 0;
-- 
2.43.0

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

[Vineeth Karumanchi]

The GMII-to-RGMII converter replaces phydev->drv with a modified copy
of the attached PHY driver. This copied driver is embedded in the
converter's private data and is released when the converter is
removed.

Without a remove callback, phydev->drv continues to point to the freed
copy after the converter is unbound. A subsequent PHY operation can
dereference this stale pointer and result in a use-after-free.

With Generic KASAN enabled, unbinding only the converter while the
external PHY remains active produces the following report (abridged):

  BUG: KASAN: slab-use-after-free in phy_check_link_status+0x2d8/0x338
  Read of size 8 at addr ffff000006c359b0 by task kworker/2:0/27
  Workqueue: events_power_efficient phy_state_machine
  Call trace:
   phy_check_link_status+0x2d8/0x338
   _phy_state_machine+0xdc/0xa4c
   phy_state_machine+0x2c/0x70
   process_one_work+0x554/0xe44
   worker_thread+0x6d0/0x1180
   kthread+0x2e8/0x5d4
   ret_from_fork+0x10/0x20

  Allocated by task 55:
   ...
   devm_kmalloc+0xac/0x2ac
   xgmiitorgmii_probe+0xa0/0x37c
   mdio_probe+0x68/0xb4
   ...

  Freed by task 642:
   ...
   kfree+0x14c/0x38c
   release_nodes+0xb4/0x1e0
   devres_release_all+0x140/0x1f4
   device_unbind_cleanup+0x20/0x190
   device_release_driver_internal+0x344/0x460
   device_driver_detach+0x3c/0x54
   unbind_store+0xe0/0xf8
   ...

Store the converter private data in its own MDIO device and add a
remove callback. Restore the attached PHY's original driver while
holding phydev->lock so that the update cannot race with an active PHY
callback.

Also release the device reference acquired by of_phy_find_device().

Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 61f71e977a57..b9aa5515577b 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)
 	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
 	priv->conv_phy_drv.set_loopback = xgmiitorgmii_set_loopback;
 	priv->phy_dev->drv = &priv->conv_phy_drv;
+	mdiodev_set_drvdata(mdiodev, priv);
 
 	return 0;
 }
 
+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;
+	mutex_unlock(&priv->phy_dev->lock);
+
+	put_device(&priv->phy_dev->mdio.dev);
+}
+
 static const struct of_device_id xgmiitorgmii_of_match[] = {
 	{ .compatible = "xlnx,gmii-to-rgmii-1.0" },
 	{},
@@ -140,6 +159,7 @@ MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
 
 static struct mdio_driver xgmiitorgmii_driver = {
 	.probe	= xgmiitorgmii_probe,
+	.remove	= xgmiitorgmii_remove,
 	.mdiodrv.driver = {
 		.name = "xgmiitorgmii",
 		.of_match_table = xgmiitorgmii_of_match,
-- 
2.43.0

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

[netdev-bot+sashiko]

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?

> +	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.

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

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

[Karumanchi, Vineeth]

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