[PATCH net v3 0/7] net: ethernet: release managed IRQs before freeing netdevs

[PATCH net v3 0/7] net: ethernet: release managed IRQs before freeing netdevs

[Jiale Yao]

Several Ethernet platform drivers request interrupts with
devm_request_irq() but allocate and free their netdevs manually.
Device-managed resources are released only after the driver's remove
callback returns, so these callbacks can free the IRQ data while the
interrupt handlers can still be invoked. A late or shared interrupt in
this window can dereference freed memory.

For six drivers, make the netdev allocation device managed. Since each IRQ
is requested after its netdev is allocated, devres ordering releases the
IRQ before the netdev. SXGBE also keeps its hardware operations object
alive through the same ordering because its handlers dereference that
object directly.

FEC additionally masks its hardware interrupt sources and disables the
Linux IRQs before unregistering the netdev, preventing handlers from
accessing registers after the clocks and other resources are released.

RAVB keeps its netdev manually managed because its remove callback has an
existing runtime PM error path which can return before unregistering it.
Instead, place its IRQs in a dedicated devres group and release that group
after unregistering the netdev and on probe failures. The runtime PM error
path is intentionally left unchanged and will be addressed separately
after this series.

These issues were found by a static analysis method used in our research.
Each patch handles one driver and is independently buildable.

Changes in v3:
- Target the net tree and document how the issues were found.
- Mask and disable FEC interrupts before dependent resources are released,
  and remove the obsolete failed_ioremap label.
- Limit the RAVB change to IRQ/netdev teardown ordering, correct its Fixes
  tag, and defer the separate runtime PM error-path change.

Changes in v2:
- Keep commit message tags together without blank lines between them, as
  requested by Francesco.

Jiale Yao (7):
  net: macb: manage the netdev lifetime with devres
  net: fec: release IRQs before dependent resources
  net: hip04: manage the netdev lifetime with devres
  net: hisi_femac: manage the netdev lifetime with devres
  net: hix5hd2: manage the netdev lifetime with devres
  net: ravb: release managed IRQs before freeing netdev
  net: sxgbe: manage IRQ data lifetimes with devres

 drivers/net/ethernet/cadence/macb_main.c      | 17 +++++------
 drivers/net/ethernet/freescale/fec_main.c     | 28 +++++++++++--------
 drivers/net/ethernet/hisilicon/hip04_eth.c    |  4 +--
 drivers/net/ethernet/hisilicon/hisi_femac.c   | 15 ++++------
 drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 15 ++++------
 drivers/net/ethernet/renesas/ravb_main.c      | 18 +++++++++---
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 26 ++++++-----------
 7 files changed, 60 insertions(+), 63 deletions(-)

-- 
2.34.1

[PATCH net v3 1/7] net: macb: manage the netdev lifetime with devres

[Jiale Yao]

macb_remove() frees the netdev while its managed IRQs are only
released after the remove callback returns. An interrupt in that window
can dereference the freed netdev or queue data.

Allocate the netdev with devres as well. Since the IRQs are registered
later, devres releases them before freeing the netdev and closes the
lifetime gap.

This issue was found by a static analysis method used in our research.

Fixes: 0a4acf08ea62 ("net: macb: Use devm_request_irq()")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b8234ac4b602..ebf6ffb1cc4f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5812,7 +5812,8 @@ static int macb_probe(struct platform_device *pdev)
 		goto err_disable_clocks;
 	}
 
-	netdev = alloc_etherdev_mq(sizeof(*bp), num_queues);
+	netdev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*bp),
+					 num_queues, num_queues);
 	if (!netdev) {
 		err = -ENOMEM;
 		goto err_disable_clocks;
@@ -5859,7 +5860,7 @@ static int macb_probe(struct platform_device *pdev)
 			IS_ENABLED(CONFIG_MACB_USE_HWSTAMP)) {
 		dev_err(&pdev->dev, "Timer adjust mode is not supported\n");
 		err = -EINVAL;
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 	}
 
 	/* By default we set to partial store and forward mode for zynqmp.
@@ -5893,7 +5894,7 @@ static int macb_probe(struct platform_device *pdev)
 		err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(44));
 		if (err) {
 			dev_err(&pdev->dev, "failed to set DMA mask\n");
-			goto err_out_free_netdev;
+			goto err_disable_clocks;
 		}
 		bp->caps |= MACB_CAPS_DMA_64B;
 	}
@@ -5903,7 +5904,7 @@ static int macb_probe(struct platform_device *pdev)
 	netdev->irq = platform_get_irq(pdev, 0);
 	if (netdev->irq < 0) {
 		err = netdev->irq;
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 	}
 
 	/* MTU range: 68 - 1518 or 10240 */
@@ -5932,7 +5933,7 @@ static int macb_probe(struct platform_device *pdev)
 
 	err = of_get_ethdev_address(np, bp->netdev);
 	if (err == -EPROBE_DEFER)
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 	else if (err)
 		macb_get_hwaddr(bp);
 
@@ -5946,7 +5947,7 @@ static int macb_probe(struct platform_device *pdev)
 	/* IP specific init */
 	err = macb_init(pdev, macb_config);
 	if (err)
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 
 	err = macb_mii_init(bp);
 	if (err)
@@ -5988,9 +5989,6 @@ static int macb_probe(struct platform_device *pdev)
 err_out_phy_exit:
 	phy_exit(bp->phy);
 
-err_out_free_netdev:
-	free_netdev(netdev);
-
 err_disable_clocks:
 	macb_clks_disable(pclk, hclk, tx_clk, rx_clk, tsu_clk);
 	pm_runtime_disable(&pdev->dev);
@@ -6024,7 +6022,6 @@ static void macb_remove(struct platform_device *pdev)
 		pm_runtime_dont_use_autosuspend(&pdev->dev);
 		pm_runtime_set_suspended(&pdev->dev);
 		phylink_destroy(bp->phylink);
-		free_netdev(netdev);
 	}
 }
 
-- 
2.34.1

Re: [PATCH net v3 1/7] net: macb: manage the netdev lifetime with devres

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v3 1/7] net: macb: manage the netdev lifetime with devres

[Théo Lebrun]

Hello Jiale,

Those LLM bugs are code churn, that's why you are seeing pushback.
Please don't ignore the pushback. For example on V2 you got asked to
reply to an automated message, which you didn't do.

https://lore.kernel.org/netdev/20260927153020.5311dba6@kernel.org/

On Sat Oct 3, 2026 at 10:59 AM CEST, Jiale Yao wrote:
> macb_remove() frees the netdev while its managed IRQs are only
> released after the remove callback returns. An interrupt in that window
> can dereference the freed netdev or queue data.

Please indicate how an interrupt could land in that window.
Thinking about it for a brief instant, I cannot think of one.

> Allocate the netdev with devres as well. Since the IRQs are registered
> later, devres releases them before freeing the netdev and closes the
> lifetime gap.
>
> This issue was found by a static analysis method used in our research.
>
> Fixes: 0a4acf08ea62 ("net: macb: Use devm_request_irq()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

I still give my Rb because the patch is valid. The wasted time is on net
maintainers though; they'll decide if they want it or not.

For this MACB patch, it could land in net-next as I don't see a
practical bug here (in light of the recent pushback about the # of
fixes in net).

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net v3 1/7] net: macb: manage the netdev lifetime with devres

[jiale yao]

At 2026-10-04 16:45:58, "Théo Lebrun" <theo.lebrun@bootlin.com> wrote:
>Hello Jiale,
>
>Those LLM bugs are code churn, that's why you are seeing pushback.
>Please don't ignore the pushback. For example on V2 you got asked to
>reply to an automated message, which you didn't do.
>
>https://lore.kernel.org/netdev/20260927153020.5311dba6@kernel.org/
>
>On Sat Oct 3, 2026 at 10:59 AM CEST, Jiale Yao wrote:
>> macb_remove() frees the netdev while its managed IRQs are only
>> released after the remove callback returns. An interrupt in that window
>> can dereference the freed netdev or queue data.
>
>Please indicate how an interrupt could land in that window.
>Thinking about it for a brief instant, I cannot think of one.

I reproduced this on QEMU aarch64 virt + KASAN:
I added a macb node via an extended device tree, with its interrupt
line shared with virtio-rng. Keeping the rng busy triggers a steady
stream of interrupts during unbind/rebind, and macb_interrupt() hits the
window after `free_netdev()` on the first attempt. 

---
[    4.457077] BUG: KASAN: use-after-free in macb_interrupt+0xc40/0x1074
[    4.457569] Read of size 8 at addr ffff00000f8acac8 by task poc/82
[    4.457614] 
[    4.457938] CPU: 0 UID: 0 PID: 82 Comm: poc Not tainted 7.3.0-rc4 #1 PREEMPT 
[    4.458025] Hardware name: linux,dummy-virt (DT)
[    4.458167] Call trace:
[    4.458258]  show_stack+0x18/0x24 (C)
[    4.458321]  dump_stack_lvl+0x78/0x90
[    4.458340]  print_report+0x114/0x5cc
[    4.458353]  kasan_report+0xa4/0xf0
[    4.458363]  __asan_report_load8_noabort+0x20/0x2c
[    4.458374]  macb_interrupt+0xc40/0x1074
[    4.458385]  __handle_irq_event_percpu+0xc8/0x340
[    4.458398]  handle_irq_event+0xb0/0x1d8
[    4.458407]  handle_fasteoi_irq+0x298/0x6a0
[    4.458419]  handle_irq_desc+0xc4/0x104
[    4.458454]  generic_handle_domain_irq+0x18/0x24
[    4.458485]  gic_handle_irq+0x54/0x194
[    4.458498]  call_on_irq_stack+0x30/0x48
[    4.458511]  do_interrupt_handler+0xf0/0x130
[    4.458523]  el1_interrupt+0x3c/0x60
[    4.458538]  el1h_64_irq_handler+0x18/0x24
[    4.458550]  el1h_64_irq+0x6c/0x70
[    4.458619]  get_pfnblock_migratetype+0xd0/0x144 (P)
[    4.458638]  __free_frozen_pages+0x2d8/0xec4
[    4.458650]  free_frozen_pages+0x14/0x20
[    4.458661]  free_large_kmalloc+0xa0/0x120
[    4.458674]  kfree+0x84/0x424
[    4.458684]  kvfree+0x3c/0x4c
[    4.458694]  netdev_release+0x70/0x98
[    4.458707]  device_release+0x104/0x210
[    4.458720]  kobject_put+0x140/0x240
[    4.458731]  put_device+0x14/0x24
[    4.458740]  free_netdev+0x414/0x6c4
[    4.458752]  macb_remove+0x14c/0x19c
[    4.458763]  platform_remove+0x58/0x78
[    4.458774]  device_remove+0xb0/0x14c
[    4.458785]  device_release_driver_internal+0x2fc/0x468
[    4.458795]  device_driver_detach+0x3c/0x54
[    4.458804]  unbind_store+0xec/0x100
[    4.458814]  drv_attr_store+0x60/0x9c
[    4.458824]  sysfs_kf_write+0x170/0x1e8
[    4.458838]  kernfs_fop_write_iter+0x298/0x404
[    4.458848]  vfs_write+0x648/0x8cc
[    4.458859]  ksys_write+0xf0/0x1e0
[    4.458868]  __arm64_sys_write+0x70/0xa0
[    4.458877]  invoke_syscall+0x70/0x24c
[    4.458887]  el0_svc_common.constprop.0+0xa8/0x22c
[    4.458896]  do_el0_svc+0x44/0x5c
[    4.458905]  el0_svc+0x58/0xd0
[    4.458917]  el0t_64_sync_handler+0xa0/0xe4
[    4.458929]  el0t_64_sync+0x198/0x19c
[    4.459007] 
[    4.459053] The buggy address belongs to the physical page:
[    4.459261] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x4f8ac
[    4.459384] flags: 0x3fffe0000000000(node=0|zone=0|lastcpupid=0x1ffff)
[    4.459721] raw: 03fffe0000000000 0000000000000000 dead000000000122 0000000000000000
[    4.459737] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[    4.459781] page dumped because: kasan: bad access detected
[    4.459792] 
[    4.459799] Memory state around the buggy address:
[    4.459912]  ffff00000f8ac980: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[    4.459935]  ffff00000f8aca00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[    4.459951] >ffff00000f8aca80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[    4.459962]                                               ^
[    4.459996]  ffff00000f8acb00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[    4.460002]  ffff00000f8acb80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[    4.460020] ==================================================================
---
With the fix applied, several thousand cycles produce no report at all.
>
>> Allocate the netdev with devres as well. Since the IRQs are registered
>> later, devres releases them before freeing the netdev and closes the
>> lifetime gap.
>>
>> This issue was found by a static analysis method used in our research.
>>
>> Fixes: 0a4acf08ea62 ("net: macb: Use devm_request_irq()")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 17 +++++++----------
>>  1 file changed, 7 insertions(+), 10 deletions(-)
>
>Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
>
>I still give my Rb because the patch is valid. The wasted time is on net
>maintainers though; they'll decide if they want it or not.
>
>For this MACB patch, it could land in net-next as I don't see a
>practical bug here (in light of the recent pushback about the # of
>fixes in net).
>
>Thanks,
>
>--
>Théo Lebrun, Bootlin
>Embedded Linux and Kernel engineering
>https://bootlin.com

Re: [PATCH net v3 1/7] net: macb: manage the netdev lifetime with devres

[Théo Lebrun]

Hello Jiale,

On Sun Oct 4, 2026 at 2:14 PM CEST, jiale yao wrote:
> At 2026-10-04 16:45:58, "Théo Lebrun" <theo.lebrun@bootlin.com> wrote:
>>Hello Jiale,
>>
>>Those LLM bugs are code churn, that's why you are seeing pushback.
>>Please don't ignore the pushback. For example on V2 you got asked to
>>reply to an automated message, which you didn't do.
>>
>>https://lore.kernel.org/netdev/20260927153020.5311dba6@kernel.org/

You've skipped over part of my message.

>>On Sat Oct 3, 2026 at 10:59 AM CEST, Jiale Yao wrote:
>>> macb_remove() frees the netdev while its managed IRQs are only
>>> released after the remove callback returns. An interrupt in that window
>>> can dereference the freed netdev or queue data.
>>
>>Please indicate how an interrupt could land in that window.
>>Thinking about it for a brief instant, I cannot think of one.
>
> I reproduced this on QEMU aarch64 virt + KASAN:
> I added a macb node via an extended device tree, with its interrupt
> line shared with virtio-rng. Keeping the rng busy triggers a steady
> stream of interrupts during unbind/rebind, and macb_interrupt() hits the
> window after `free_netdev()` on the first attempt. 

But that never happens in the real world. We use shared interrupts
because we want to supports boards with a single interrupt lane for
all queues.

I've been thinking about removing the IRQF_SHARED flag recently, because
it tricks Sashiko (and you) into thinking that we might share our IRQ
lane with anything and anyone.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net v3 2/7] net: fec: release IRQs before dependent resources

[Jiale Yao]

fec_drv_remove() leaves the managed IRQs active until after the remove
callback returns. The handler can then run after the device has been
unregistered, its clocks have been disabled, and its other resources have
been torn down.

Mask the hardware interrupt sources and disable each IRQ before
unregistering the netdev. Mask the sources again afterwards because
fec_stop() restores the default interrupt mask. Also perform the same
cleanup on probe failures, and manage the netdev with devres so it remains
alive until the IRQ resources are released.

This issue was found by a static analysis method used in our research.

Fixes: 0d9b2ab1c376 ("fec: Use devm_request_irq()")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 28 ++++++++++++++---------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 794ec427b0ee..b0fc5b39c748 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5219,8 +5219,9 @@ fec_probe(struct platform_device *pdev)
 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
 
 	/* Init network device */
-	ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private) +
-				  FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
+	ndev = devm_alloc_etherdev_mqs(&pdev->dev,
+				       sizeof(struct fec_enet_private) +
+				       FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
 	if (!ndev)
 		return -ENOMEM;
 
@@ -5247,10 +5248,8 @@ fec_probe(struct platform_device *pdev)
 	pinctrl_pm_select_default_state(&pdev->dev);
 
 	fep->hwp = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(fep->hwp)) {
-		ret = PTR_ERR(fep->hwp);
-		goto failed_ioremap;
-	}
+	if (IS_ERR(fep->hwp))
+		return PTR_ERR(fep->hwp);
 
 	fep->pdev = pdev;
 	fep->dev_id = dev_id++;
@@ -5455,6 +5454,9 @@ fec_probe(struct platform_device *pdev)
 	fec_enet_mii_remove(fep);
 failed_mii_init:
 failed_irq:
+	fec_irqs_disable(ndev);
+	while (i--)
+		disable_irq(fep->irq[i]);
 	fec_enet_deinit(ndev);
 failed_init:
 	if (fep->bufdesc_ex)
@@ -5479,9 +5481,6 @@ fec_probe(struct platform_device *pdev)
 failed_ipc_init:
 failed_phy:
 	dev_id--;
-failed_ioremap:
-	free_netdev(ndev);
-
 	return ret;
 }
 
@@ -5491,7 +5490,7 @@ fec_drv_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct device_node *np = pdev->dev.of_node;
-	int ret;
+	int i, irq_cnt, ret;
 
 	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
@@ -5502,7 +5501,15 @@ fec_drv_remove(struct platform_device *pdev)
 	cancel_work_sync(&fep->tx_timeout_work);
 	if (fep->bufdesc_ex)
 		fec_ptp_stop(pdev);
+	if (ret >= 0)
+		fec_irqs_disable(ndev);
+	irq_cnt = fec_enet_get_irq_cnt(pdev);
+	for (i = 0; i < irq_cnt; i++)
+		disable_irq(fep->irq[i]);
 	unregister_netdev(ndev);
+	/* fec_stop() enables the default interrupt mask. */
+	if (ret >= 0)
+		fec_irqs_disable(ndev);
 	fec_enet_mii_remove(fep);
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
@@ -5522,7 +5529,6 @@ fec_drv_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 
 	fec_enet_deinit(ndev);
-	free_netdev(ndev);
 }
 
 static int fec_suspend(struct device *dev)
-- 
2.34.1

Re: [PATCH net v3 2/7] net: fec: release IRQs before dependent resources

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · Low: 1

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 leaves out the PPS/1588 timer IRQ, which is also devm-
  managed.
- [High] fec_drv_remove() still calls
  cancel_work_sync(&fep->tx_timeout_work) first.
- [Low] fec_drv_remove() bounds its new disable_irq() loop by calling
  fec_enet_get_irq_cnt(pdev) again, instead of using the number of IRQs…

--

net: fec: release IRQs before dependent resources

In fec_drv_remove(), this commit masks the FEC interrupt sources and calls
disable_irq() on the main FEC IRQs before unregister_netdev(). It adds the
same cleanup to the fec_probe() error path. It also switches to
devm_alloc_etherdev_mqs() so the netdev stays alive until the devm-managed
IRQs are released.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..b0fc5b39c748d 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c

[ ... ]

> @@ -5502,7 +5501,15 @@ fec_drv_remove(struct platform_device *pdev)
>  	cancel_work_sync(&fep->tx_timeout_work);
>  	if (fep->bufdesc_ex)
>  		fec_ptp_stop(pdev);
> +	if (ret >= 0)
> +		fec_irqs_disable(ndev);
> +	irq_cnt = fec_enet_get_irq_cnt(pdev);

[Severity: Low]
Should this reuse the number of IRQs that probe actually requested, instead
of computing it again?

fec_enet_get_irq_cnt() maps any platform_irq_count() result <= 0 to 1. That
includes -EPROBE_DEFER:

	else if (irq_cnt <= 0)
		irq_cnt = 1;	/* At least 1 irq is needed */

Say a later interrupt's parent deferred at probe time while index 0
resolved. Then probe requests only fep->irq[0] and still succeeds. At remove
time the count can be larger, so the loop below would call disable_irq() on
fep->irq[1] and fep->irq[2]. Both are still 0.

On ARM32 with SPARSE_IRQ, that raises the depth of descriptor 0 and calls
synchronize_irq(0). On arm64 it does nothing.

> +	for (i = 0; i < irq_cnt; i++)
> +		disable_irq(fep->irq[i]);

[Severity: High]
The commit message says the patch will "disable each IRQ" before
unregistering the netdev. Does this loop also cover the PPS interrupt?

fec_ptp_init() requests a separate devm-managed IRQ, and its number is never
stored in fep->irq[]:

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_init() {
    ...
	irq = platform_get_irq_byname_optional(pdev, "pps");
	if (irq < 0)
		irq = platform_get_irq_optional(pdev, irq_idx);
    ...
	if (irq >= 0) {
		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
				       0, pdev->name, ndev);
    ...
}

So this IRQ stays enabled until devres runs after fec_drv_remove() returns.
Writing 0 to FEC_IMASK in fec_irqs_disable() does not gate it either. The
timer channel interrupt is armed by FEC_T_TIE in FEC_TCSR(pps_channel).

fec_ptp_stop() also runs before any IRQ is disabled here. It calls
ptp_clock_unregister() without synchronizing the PPS IRQ. Meanwhile,
fec_pps_interrupt() checks pps_enable without taking a lock:

	if (fep->pps_enable) {
		event.type = PTP_CLOCK_PPS;
		ptp_clock_event(fep->ptp_clock, &event);

Could a handler already running on another CPU use the ptp_clock after it
has been freed?

In PTP_CLK_REQ_PEROUT mode, pps_enable is 0. fec_ptp_stop() then only
cancels the hrtimer, and FEC_TCSR keeps TIE set in toggle mode. On
FEC_QUIRK_HAS_MULTI_QUEUES parts, fec_stop() also turns EN1588 back on.

Could fec_pps_interrupt() still readl()/writel() FEC_TCSR/FEC_TCCR after
fec_drv_remove() disables clk_ahb and clk_ipg further down?

The new probe unwind has the same gap:

failed_irq:
	fec_irqs_disable(ndev);
	while (i--)
		disable_irq(fep->irq[i]);
	fec_enet_deinit(ndev);
failed_init:
	if (fep->bufdesc_ex)
		fec_ptp_stop(pdev);

fec_ptp_init() runs before fec_enet_init(). The PPS IRQ is therefore still
live through fec_ptp_stop() and through the clock disables at
failed_regulator.

Would it make sense to store the PPS IRQ number and disable_irq() it before
fec_ptp_stop()? FEC_TCSR could also be cleared for the PEROUT case. Nothing
later in the series seems to touch fec.

>  	unregister_netdev(ndev);

[Severity: High]
cancel_work_sync(&fep->tx_timeout_work) has already run at this point. The
IRQs are now disabled while the netdev is still registered and running. Can
the TX watchdog queue tx_timeout_work again in this window?

With the IRQs disabled, fec_enet_interrupt() can no longer schedule NAPI, so
TX descriptors are not reclaimed. Under traffic, fec_enet_start_xmit()
eventually stops the queue.

Suppose unregister_netdev() waits for RTNL longer than watchdog_timeo
(TX_TIMEOUT). The chain would then be:

dev_watchdog()
  fec_timeout()
    schedule_work(&fep->tx_timeout_work);

Nothing cancels or flushes the work after this. Once unregister_netdev()
drops RTNL, fec_enet_timeout_work() runs:

	rtnl_lock();
	if (netif_device_present(ndev) || netif_running(ndev)) {
		napi_disable(&fep->napi);
		netif_tx_lock_bh(ndev);
		fec_restart(ndev);

netif_device_present() is still true after unregister. The work therefore
calls napi_disable() on a NAPI that fec_enet_close() has already disabled.

Would napi_disable_locked() then spin forever with RTNL held?

The work could also run later still. The netdev is now freed by devres
(devm_alloc_etherdev_mqs()), so it would then use fep after the netdev has
been freed.

Before this patch, this path needed a real TX hang. Now normal TX traffic
plus RTNL contention looks like enough. Should tx_timeout_work be cancelled
again after unregister_netdev()? Alternatively, the IRQs could be disabled
only after the device has been closed.

> +	/* fec_stop() enables the default interrupt mask. */
> +	if (ret >= 0)
> +		fec_irqs_disable(ndev);
>  	fec_enet_mii_remove(fep);
>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003085940.493951-2-yaojiale02%40163.com

[PATCH net v3 3/7] net: hip04: manage the netdev lifetime with devres

[Jiale Yao]

hip04_remove() frees the netdev before the managed IRQ is released
after the remove callback. Since the IRQ handler receives the netdev as
its data pointer, a late interrupt can access freed memory.

Use a managed netdev allocation. Devres then releases the IRQ, which is
registered later, before releasing the netdev.

This issue was found by a static analysis method used in our research.

Fixes: a41ea46a9a12 ("net: hisilicon: new hip04 ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index fc2c47dcfaab..4a643dff22ab 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -905,7 +905,7 @@ static int hip04_mac_probe(struct platform_device *pdev)
 	int irq;
 	int ret;
 
-	ndev = alloc_etherdev(sizeof(struct hip04_priv));
+	ndev = devm_alloc_etherdev(d, sizeof(struct hip04_priv));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -1021,7 +1021,6 @@ static int hip04_mac_probe(struct platform_device *pdev)
 	hip04_free_ring(ndev, d);
 init_fail:
 	of_node_put(priv->phy_node);
-	free_netdev(ndev);
 	return ret;
 }
 
@@ -1038,7 +1037,6 @@ static void hip04_remove(struct platform_device *pdev)
 	unregister_netdev(ndev);
 	of_node_put(priv->phy_node);
 	cancel_work_sync(&priv->tx_timeout_task);
-	free_netdev(ndev);
 }
 
 static const struct of_device_id hip04_mac_match[] = {
-- 
2.34.1

[PATCH net v3 4/7] net: hisi_femac: manage the netdev lifetime with devres

[Jiale Yao]

hisi_femac_drv_remove() frees the netdev while the shared managed IRQ
remains registered until devres cleanup. Its handler uses the netdev as
private data, so an interrupt in this window can dereference freed
memory.

Manage the netdev allocation with devres so the later IRQ resource is
released before the netdev.

This issue was found by a static analysis method used in our research.

Fixes: 542ae60af24f ("net: hisilicon: Add Fast Ethernet MAC driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/hisilicon/hisi_femac.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index d244a40df430..d369824fa4e8 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -774,7 +774,7 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 	struct phy_device *phy;
 	int ret;
 
-	ndev = alloc_etherdev(sizeof(*priv));
+	ndev = devm_alloc_etherdev(dev, sizeof(*priv));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -788,26 +788,26 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 	priv->port_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->port_base)) {
 		ret = PTR_ERR(priv->port_base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->glb_base = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(priv->glb_base)) {
 		ret = PTR_ERR(priv->glb_base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "failed to get clk\n");
 		ret = -ENODEV;
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	ret = clk_prepare_enable(priv->clk);
 	if (ret) {
 		dev_err(dev, "failed to enable clk %d\n", ret);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->mac_rst = devm_reset_control_get(dev, "mac");
@@ -887,9 +887,7 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 	phy_disconnect(phy);
 out_disable_clk:
 	clk_disable_unprepare(priv->clk);
-out_free_netdev:
-	free_netdev(ndev);
-
+out_return:
 	return ret;
 }
 
@@ -903,7 +901,6 @@ static void hisi_femac_drv_remove(struct platform_device *pdev)
 
 	phy_disconnect(ndev->phydev);
 	clk_disable_unprepare(priv->clk);
-	free_netdev(ndev);
 }
 
 #ifdef CONFIG_PM
-- 
2.34.1

[PATCH net v3 5/7] net: hix5hd2: manage the netdev lifetime with devres

[Jiale Yao]

hix5hd2_dev_remove() manually frees the netdev before devres releases
the IRQ. The interrupt handler receives that netdev as its data pointer
and can access it during this teardown window.

Allocate the netdev through devres so its later registered IRQ is
released first.

This issue was found by a static analysis method used in our research.

Fixes: 57c5bc9ad7d7 ("net: hisilicon: add hix5hd2 mac driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
index 02282dc86faf..ffcb281230eb 100644
--- a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
+++ b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
@@ -1100,7 +1100,7 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
 	struct mii_bus *bus;
 	int ret;
 
-	ndev = alloc_etherdev(sizeof(struct hix5hd2_priv));
+	ndev = devm_alloc_etherdev(dev, sizeof(struct hix5hd2_priv));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -1115,26 +1115,26 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
 	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base)) {
 		ret = PTR_ERR(priv->base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->ctrl_base = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(priv->ctrl_base)) {
 		ret = PTR_ERR(priv->ctrl_base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->mac_core_clk = devm_clk_get(&pdev->dev, "mac_core");
 	if (IS_ERR(priv->mac_core_clk)) {
 		netdev_err(ndev, "failed to get mac core clk\n");
 		ret = -ENODEV;
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	ret = clk_prepare_enable(priv->mac_core_clk);
 	if (ret < 0) {
 		netdev_err(ndev, "failed to enable mac core clk %d\n", ret);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->mac_ifc_clk = devm_clk_get(&pdev->dev, "mac_ifc");
@@ -1271,9 +1271,7 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
 	clk_disable_unprepare(priv->mac_ifc_clk);
 out_disable_mac_core_clk:
 	clk_disable_unprepare(priv->mac_core_clk);
-out_free_netdev:
-	free_netdev(ndev);
-
+out_return:
 	return ret;
 }
 
@@ -1291,7 +1289,6 @@ static void hix5hd2_dev_remove(struct platform_device *pdev)
 	hix5hd2_destroy_hw_desc_queue(priv);
 	of_node_put(priv->phy_node);
 	cancel_work_sync(&priv->tx_timeout_task);
-	free_netdev(ndev);
 }
 
 static const struct of_device_id hix5hd2_of_match[] = {
-- 
2.34.1

[PATCH net v3 6/7] net: ravb: release managed IRQs before freeing netdev

[Jiale Yao]

ravb_remove() frees the netdev before devres releases the managed IRQs.
The handlers use the netdev as their data pointer, so an interrupt during
that window can access freed memory. Probe error paths have the same
ordering problem.

Keep the netdev manually managed and place only the IRQ resources in a
dedicated devres group. Release the group after unregistering the netdev
and before freeing it, and release it on probe failures as well. This
keeps the existing runtime PM error handling unchanged.

This issue was found by a static analysis method used in our research.

Fixes: 32f012b8c01c ("net: ravb: Move getting/requesting IRQs in the probe() method")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ea1c7e536791..ab4703888778 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
 		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
 	}
 
+	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
+		error = -ENOMEM;
+		goto out_reset_assert;
+	}
+
 	error = ravb_setup_irqs(priv);
 	if (error)
-		goto out_reset_assert;
+		goto out_release_irq_group;
+
+	devres_close_group(&pdev->dev, priv);
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		error = PTR_ERR(priv->clk);
-		goto out_reset_assert;
+		goto out_release_irq_group;
 	}
 
 	if (info->gptp_ref_clk) {
 		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
 		if (IS_ERR(priv->gptp_clk)) {
 			error = PTR_ERR(priv->gptp_clk);
-			goto out_reset_assert;
+			goto out_release_irq_group;
 		}
 	}
 
 	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
 	if (IS_ERR(priv->refclk)) {
 		error = PTR_ERR(priv->refclk);
-		goto out_reset_assert;
+		goto out_release_irq_group;
 	}
 	clk_prepare(priv->refclk);
 
@@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	clk_unprepare(priv->refclk);
+out_release_irq_group:
+	devres_release_group(&pdev->dev, priv);
 out_reset_assert:
 	reset_control_assert(rstc);
 out_free_netdev:
@@ -3144,6 +3153,7 @@ static void ravb_remove(struct platform_device *pdev)
 		return;
 
 	unregister_netdev(ndev);
+	devres_release_group(dev, priv);
 	if (info->nc_queues)
 		netif_napi_del(&priv->napi[RAVB_NC]);
 	netif_napi_del(&priv->napi[RAVB_BE]);
-- 
2.34.1

Re: [PATCH net v3 6/7] net: ravb: release managed IRQs before freeing netdev

[Niklas Söderlund]

Hi Jiale,

On 2026-10-03 16:59:37 +0800, Jiale Yao wrote:
> ravb_remove() frees the netdev before devres releases the managed IRQs.
> The handlers use the netdev as their data pointer, so an interrupt during
> that window can access freed memory. Probe error paths have the same
> ordering problem.
> 
> Keep the netdev manually managed and place only the IRQ resources in a
> dedicated devres group. Release the group after unregistering the netdev
> and before freeing it, and release it on probe failures as well. This
> keeps the existing runtime PM error handling unchanged.
> 
> This issue was found by a static analysis method used in our research.

What happened to switching to use devm_alloc_etherdev_mqs() instead of 
adding this complex thing, as we discussed in v2?

Nacked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> Fixes: 32f012b8c01c ("net: ravb: Move getting/requesting IRQs in the probe() method")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791..ab4703888778 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>  	}
>  
> +	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
> +		error = -ENOMEM;
> +		goto out_reset_assert;
> +	}
> +
>  	error = ravb_setup_irqs(priv);
>  	if (error)
> -		goto out_reset_assert;
> +		goto out_release_irq_group;
> +
> +	devres_close_group(&pdev->dev, priv);
>  
>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		error = PTR_ERR(priv->clk);
> -		goto out_reset_assert;
> +		goto out_release_irq_group;
>  	}
>  
>  	if (info->gptp_ref_clk) {
>  		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
>  		if (IS_ERR(priv->gptp_clk)) {
>  			error = PTR_ERR(priv->gptp_clk);
> -			goto out_reset_assert;
> +			goto out_release_irq_group;
>  		}
>  	}
>  
>  	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
>  	if (IS_ERR(priv->refclk)) {
>  		error = PTR_ERR(priv->refclk);
> -		goto out_reset_assert;
> +		goto out_release_irq_group;
>  	}
>  	clk_prepare(priv->refclk);
>  
> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	clk_unprepare(priv->refclk);
> +out_release_irq_group:
> +	devres_release_group(&pdev->dev, priv);
>  out_reset_assert:
>  	reset_control_assert(rstc);
>  out_free_netdev:
> @@ -3144,6 +3153,7 @@ static void ravb_remove(struct platform_device *pdev)
>  		return;
>  
>  	unregister_netdev(ndev);
> +	devres_release_group(dev, priv);
>  	if (info->nc_queues)
>  		netif_napi_del(&priv->napi[RAVB_NC]);
>  	netif_napi_del(&priv->napi[RAVB_BE]);
> -- 
> 2.34.1
> 

-- 
Kind Regards,
Niklas Söderlund

Re:Re: [PATCH net v3 6/7] net: ravb: release managed IRQs before freeing netdev

[jiale yao]

At 2026-10-03 17:59:21, "Niklas Söderlund" <niklas.soderlund@ragnatech.se> wrote:
>Hi Jiale,
>
>On 2026-10-03 16:59:37 +0800, Jiale Yao wrote:
>> ravb_remove() frees the netdev before devres releases the managed IRQs.
>> The handlers use the netdev as their data pointer, so an interrupt during
>> that window can access freed memory. Probe error paths have the same
>> ordering problem.
>> 
>> Keep the netdev manually managed and place only the IRQ resources in a
>> dedicated devres group. Release the group after unregistering the netdev
>> and before freeing it, and release it on probe failures as well. This
>> keeps the existing runtime PM error handling unchanged.
>> 
>> This issue was found by a static analysis method used in our research.
>
>What happened to switching to use devm_alloc_etherdev_mqs() instead of 
>adding this complex thing, as we discussed in v2?

You're right. I missed this point while working through a large number
of patches...>_<
I should take a break and post an updated revision after the
24-hour waiting period.

>
>Nacked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
>> 
>> Fixes: 32f012b8c01c ("net: ravb: Move getting/requesting IRQs in the probe() method")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index ea1c7e536791..ab4703888778 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
>>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>>  	}
>>  
>> +	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
>> +		error = -ENOMEM;
>> +		goto out_reset_assert;
>> +	}
>> +
>>  	error = ravb_setup_irqs(priv);
>>  	if (error)
>> -		goto out_reset_assert;
>> +		goto out_release_irq_group;
>> +
>> +	devres_close_group(&pdev->dev, priv);
>>  
>>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>>  	if (IS_ERR(priv->clk)) {
>>  		error = PTR_ERR(priv->clk);
>> -		goto out_reset_assert;
>> +		goto out_release_irq_group;
>>  	}
>>  
>>  	if (info->gptp_ref_clk) {
>>  		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
>>  		if (IS_ERR(priv->gptp_clk)) {
>>  			error = PTR_ERR(priv->gptp_clk);
>> -			goto out_reset_assert;
>> +			goto out_release_irq_group;
>>  		}
>>  	}
>>  
>>  	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
>>  	if (IS_ERR(priv->refclk)) {
>>  		error = PTR_ERR(priv->refclk);
>> -		goto out_reset_assert;
>> +		goto out_release_irq_group;
>>  	}
>>  	clk_prepare(priv->refclk);
>>  
>> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>>  	pm_runtime_disable(&pdev->dev);
>>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>>  	clk_unprepare(priv->refclk);
>> +out_release_irq_group:
>> +	devres_release_group(&pdev->dev, priv);
>>  out_reset_assert:
>>  	reset_control_assert(rstc);
>>  out_free_netdev:
>> @@ -3144,6 +3153,7 @@ static void ravb_remove(struct platform_device *pdev)
>>  		return;
>>  
>>  	unregister_netdev(ndev);
>> +	devres_release_group(dev, priv);
>>  	if (info->nc_queues)
>>  		netif_napi_del(&priv->napi[RAVB_NC]);
>>  	netif_napi_del(&priv->napi[RAVB_BE]);
>> -- 
>> 2.34.1
>> 
>
>-- 
>Kind Regards,
>Niklas Söderlund

Re: [PATCH net v3 6/7] net: ravb: release managed IRQs before freeing netdev

[netdev-bot+sashiko]

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

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

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 commit message says the probe error paths are fixed.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch.
- [Low] This problem wasn't introduced by this patch.

--

net: ravb: release managed IRQs before freeing netdev

This commit puts the IRQs requested by ravb_setup_irqs() into their own
devres group. ravb_remove() releases the group after unregister_netdev()
and before free_netdev(). The probe error paths also release it before
free_netdev(), so the IRQ handlers should no longer see a freed netdev.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791..ab4703888778 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>  	}
>  
> +	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
> +		error = -ENOMEM;
> +		goto out_reset_assert;
> +	}
> +
>  	error = ravb_setup_irqs(priv);
>  	if (error)
> -		goto out_reset_assert;
> +		goto out_release_irq_group;
> +
> +	devres_close_group(&pdev->dev, priv);
>  
>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		error = PTR_ERR(priv->clk);
> -		goto out_reset_assert;
> +		goto out_release_irq_group;
>  	}

[Severity: High]

The commit message says:

  "Probe error paths have the same ordering problem."

Can ravb_interrupt() still crash on the probe error paths that fail early?

The netdev is no longer freed before free_irq(). However, the IRQs are
still requested before priv->addr, priv->lock and runtime PM are set up.

The single IRQ variants (ravb_gen2_hw_info and gbeth_hw_info) do not set
multi_irqs. For them, ravb_setup_irq() requests a shared IRQ and passes
ndev as dev_id:

	} else {
		irq_num = platform_get_irq(pdev, 0);
		flags = IRQF_SHARED;
	}

With CONFIG_DEBUG_SHIRQ, __free_irq() calls the handler once for
IRQF_SHARED actions, so releasing the group here runs it:

ravb_probe()
  devres_release_group()
    devm_irq_release()
      free_irq()
        __free_irq()
          ravb_interrupt(irq, ndev)

A real interrupt from another device on the shared line can also run the
handler at any point in this window.

The only guard in ravb_interrupt() is:

	if (unlikely(!pm_runtime_active(dev)))
		goto out_rpm_put;

	spin_lock(&priv->lock);
	/* Get interrupt status */
	iss = ravb_read(ndev, ISS);

pm_runtime_active() returns true whenever dev->power.disable_depth is
nonzero. That is the case both before pm_runtime_enable() and after
pm_runtime_disable().

The handler then calls spin_lock() on priv->lock before spin_lock_init()
has run. It also calls ravb_read() through an invalid priv->addr:

- The devm_clk_get() failures here (including -EPROBE_DEFER), the gptp
  and refclk clock failures, and a pm_runtime_resume_and_get() failure
  all leave priv->addr NULL.

- A devm_platform_get_and_ioremap_resource() failure leaves an ERR_PTR
  in priv->addr.

Before this patch, the same handler call ran from devres_release_all()
and hit freed memory. With this patch it reads MMIO through NULL or an
ERR_PTR.

The ordering where IRQs are requested before this state is initialised
came from 32f012b8c01c, the commit in the Fixes: tag.

Could the IRQs be requested only after priv->addr, priv->lock and
runtime PM are valid, while still releasing the group before
free_netdev()? Alternatively, could ravb_interrupt() return early until
probe has finished initialising?

[ ... ]

> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	clk_unprepare(priv->refclk);
> +out_release_irq_group:
> +	devres_release_group(&pdev->dev, priv);
>  out_reset_assert:
>  	reset_control_assert(rstc);
>  out_free_netdev:

[Severity: Low]

This is a pre-existing issue, not one introduced by this patch. The
out_rpm_put path just above this hunk does an async put and then
disables runtime PM straight away:

out_rpm_put:
	pm_runtime_put(&pdev->dev);
out_rpm_disable:
	pm_runtime_disable(&pdev->dev);

Autosuspend is enabled with a 100 ms delay. The barrier inside
pm_runtime_disable() cancels the queued idle/autosuspend request, so the
driver never suspends the device itself.

Would pm_runtime_put_sync() fit better here?

The effect on the hardware is mostly limited. After a failed probe,
device_unbind_cleanup() calls dev_pm_domain_detach() with
detach_power_off. Through the Renesas CPG detach callbacks this reaches
pm_clk_destroy(), which disables the module clock, and the genpd power
off gets queued.

The commit message also says the runtime PM error handling is left
unchanged on purpose.

> @@ -3144,6 +3153,7 @@ static void ravb_remove(struct platform_device *pdev)
>  		return;
>  
>  	unregister_netdev(ndev);
> +	devres_release_group(dev, priv);

[Severity: Medium]

This isn't a bug introduced by this patch. What happens when
pm_runtime_resume_and_get() fails at the top of ravb_remove()?

	error = pm_runtime_resume_and_get(dev);
	if (error < 0)
		return;

The early return skips all of the following:

- unregister_netdev()
- the new devres_release_group()
- netif_napi_del()
- ravb_mdio_release()
- dma_free_coherent() of desc_bat
- pm_runtime_disable()
- clk_unprepare()
- reset_control_assert()
- free_netdev()

Remove returns void, so the driver core unbinds anyway, and
devres_release_all() frees the IRQs and unmaps priv->addr.

Does this leak the netdev, the MDIO bus and the desc_bat coherent DMA
buffer? Does it also leave the netdev registered on hardware that is no
longer mapped?

The ndev is never freed on this path, so the patch does not turn this
into a use-after-free.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003085940.493951-2-yaojiale02%40163.com

[PATCH net v3 7/7] net: sxgbe: manage IRQ data lifetimes with devres

[Jiale Yao]

sxgbe_drv_remove() frees the netdev and hardware operations while
managed IRQs remain registered until the remove callback returns. The
handlers dereference these objects, so an interrupt in that window can
access freed memory.

Allocate both objects with devres. They are acquired before the IRQs and
are consequently released only after the IRQ resources have been
removed.

This issue was found by a static analysis method used in our research.

Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 26 +++++++------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index 70cf3619555f..ada851477302 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -2007,7 +2007,7 @@ static int sxgbe_hw_init(struct sxgbe_priv_data * const priv)
 {
 	u32 ctrl_ids;
 
-	priv->hw = kmalloc_obj(*priv->hw);
+	priv->hw = devm_kmalloc(priv->device, sizeof(*priv->hw), GFP_KERNEL);
 	if(!priv->hw)
 		return -ENOMEM;
 
@@ -2058,7 +2058,7 @@ static int sxgbe_sw_reset(void __iomem *addr)
  * @plat_dat: platform data pointer
  * @addr: iobase memory address
  * Description: this is the main probe function used to
- * call the alloc_etherdev, allocate the priv structure.
+ * allocate the netdev and priv structure.
  */
 struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 					struct sxgbe_plat_data *plat_dat,
@@ -2069,8 +2069,8 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 	int ret;
 	u8 queue_num;
 
-	ndev = alloc_etherdev_mqs(sizeof(struct sxgbe_priv_data),
-				  SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
+	ndev = devm_alloc_etherdev_mqs(device, sizeof(struct sxgbe_priv_data),
+				       SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
 	if (!ndev)
 		return NULL;
 
@@ -2086,7 +2086,7 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 
 	ret = sxgbe_sw_reset(priv->ioaddr);
 	if (ret)
-		goto error_free_netdev;
+		goto error_return;
 
 	/* Verify driver arguments */
 	sxgbe_verify_args();
@@ -2094,16 +2094,16 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 	/* Init MAC and get the capabilities */
 	ret = sxgbe_hw_init(priv);
 	if (ret)
-		goto error_free_netdev;
+		goto error_return;
 
 	/* allocate memory resources for Descriptor rings */
 	ret = txring_mem_alloc(priv);
 	if (ret)
-		goto error_free_hw;
+		goto error_return;
 
 	ret = rxring_mem_alloc(priv);
 	if (ret)
-		goto error_free_hw;
+		goto error_return;
 
 	ndev->netdev_ops = &sxgbe_netdev_ops;
 
@@ -2191,11 +2191,7 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 	clk_put(priv->sxgbe_clk);
 error_napi_del:
 	netif_napi_del(&priv->napi);
-error_free_hw:
-	kfree(priv->hw);
-error_free_netdev:
-	free_netdev(ndev);
-
+error_return:
 	return NULL;
 }
 
@@ -2229,10 +2225,6 @@ void sxgbe_drv_remove(struct net_device *ndev)
 	clk_put(priv->sxgbe_clk);
 
 	netif_napi_del(&priv->napi);
-
-	kfree(priv->hw);
-
-	free_netdev(ndev);
 }
 
 #ifdef CONFIG_PM
-- 
2.34.1

Re: [PATCH net v3 7/7] net: sxgbe: manage IRQ data lifetimes with devres

[netdev-bot+sashiko]

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

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

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 does not close the bug class its commit message
  describes: IRQ handlers can still run against freed memory in the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but any re-open of
  the interface fails, and the MTU-change path can then wedge rtnl.
- [High] This problem wasn't introduced by this patch, but the EEE
  control timer can be re-armed after sxgbe_release() cancels it, and…
- [Medium] This problem wasn't introduced by this patch, but
  sxgbe_platform_probe() only allocates plat_dat when pdev->dev.of_node…
- [Medium] This problem wasn't introduced by this patch, but
  sxgbe_drv_probe() calls register_netdev(), which makes the interface…
- [Low] This problem wasn't introduced by this patch, but init_tx_ring()
  allocates tx_ring->tx_skbuff_dma and tx_ring->tx_skbuff with…

--

net: sxgbe: manage IRQ data lifetimes with devres

This commit switches the netdev and priv->hw allocations in the sxgbe
driver to devres. They are then released after the devm-managed IRQs
requested in sxgbe_open(). The goal is to close a window where an
interrupt arriving after sxgbe_drv_remove() could dereference freed
memory.

> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> index 70cf3619555f9..ada851477302e 100644
> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c

[ ... ]

> @@ -2069,8 +2069,8 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
>  	int ret;
>  	u8 queue_num;
>  
> -	ndev = alloc_etherdev_mqs(sizeof(struct sxgbe_priv_data),
> -				  SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
> +	ndev = devm_alloc_etherdev_mqs(device, sizeof(struct sxgbe_priv_data),
> +				       SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
>  	if (!ndev)
>  		return NULL;
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but can priv->plat be NULL
in sxgbe_drv_probe()?

sxgbe_platform_probe() only allocates plat_dat when pdev->dev.of_node is
set, but it always passes plat_dat to sxgbe_drv_probe():

	priv = sxgbe_drv_probe(&(pdev->dev), plat_dat, addr);

If a platform device without an OF node gets bound, either by name
("sam_sxgbeeth") or through driver_override, sxgbe_drv_probe()
dereferences the NULL pointer here:

	if (!priv->plat->riwt_off) {

[ ... ]

> @@ -2191,11 +2191,7 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
>  	clk_put(priv->sxgbe_clk);
>  error_napi_del:
>  	netif_napi_del(&priv->napi);
> -error_free_hw:
> -	kfree(priv->hw);
> -error_free_netdev:
> -	free_netdev(ndev);
> -
> +error_return:
>  	return NULL;
>  }

[Severity: Medium]
This is a pre-existing issue, but is the netdev published before its
IRQ numbers are set?

sxgbe_drv_probe() calls register_netdev() and returns. Only after that
does sxgbe_platform_probe() fill in the IRQ numbers:

	priv = sxgbe_drv_probe(&(pdev->dev), plat_dat, addr);
	...
	priv->irq  = irq_of_parse_and_map(node, 0);

The txq/rxq structures come from devm_kmalloc() and are not zeroed. If
udev or NetworkManager calls dev_open() in that window, sxgbe_open()
runs with priv->irq == 0, or with uninitialized txq[i]->irq_no and
rxq[i]->irq_no values.

> @@ -2229,10 +2225,6 @@ void sxgbe_drv_remove(struct net_device *ndev)
>  	clk_put(priv->sxgbe_clk);
>  
>  	netif_napi_del(&priv->napi);
> -
> -	kfree(priv->hw);
> -
> -	free_netdev(ndev);
>  }

[Severity: High]
Does this fully close the window described in the commit message? The
netdev and priv->hw now outlive the IRQs. However, the IRQs themselves
stay live after the interface is closed.

sxgbe_open() requests the common, LPI, per-TX and per-RX IRQs with
devm_request_irq(priv->device, ...). Neither sxgbe_release() nor the
sxgbe_open() error path frees, masks or synchronizes them.

unregister_netdev() in sxgbe_drv_remove() runs sxgbe_release(), which
does:

	priv->hw->dma->stop_tx(priv->ioaddr, SXGBE_TX_QUEUES);
	...
	free_dma_desc_resources(priv);
	...
	clk_disable_unprepare(priv->sxgbe_clk);

It never writes SXGBE_DMA_CHA_INT_ENABLE_REG and never frees the IRQs.
The handlers stay registered until devres teardown after remove
returns. The same state exists after every ifdown and every
sxgbe_change_mtu().

In that window, sxgbe_tx_interrupt() has no interface-up check:

	if (unlikely((status & tx_hard_error)))
		sxgbe_restart_tx_queue(priv, txq->queue_no);

and sxgbe_restart_tx_queue() does:

	tx_free_ring_skbufs(tx_ring);
	...
	priv->hw->dma->start_tx_queue(priv->ioaddr, queue_num);
	...
	netif_tx_wake_queue(dev_txq);

Can tx_free_ring_skbufs() race with dma_free_tx_skbufs() and
free_tx_ring() in close? That could unmap and free the same skbs twice,
or read freed dma_tx descriptors through get_tx_len().

Can start_tx_queue() set SXGBE_TX_ENABLE again on a channel whose
descriptor base still points at the ring freed with
dma_free_coherent()? The device could then DMA into reallocated memory.

The trigger might not need a stray error. SXGBE_DMA_ENA_INT includes
SXGBE_DMA_INT_ENA_TSE, and sxgbe_tx_dma_int_status() maps
SXGBE_DMA_INT_STATUS_TPS to tx_hard_error. If the hardware sets TPS on
a software stop, the stop_tx() calls in sxgbe_drv_remove() and
sxgbe_release() could raise the very interrupt that restarts TX DMA.
That hardware behaviour can't be confirmed from the code alone.

All three handlers also access CSRs through priv->ioaddr after
clk_disable_unprepare(). The common and LPI lines are IRQF_SHARED. So
another device on the line, or the extra CONFIG_DEBUG_SHIRQ call in
free_irq() during devres teardown, can run them while the clock is
gated.

Would it be better to mask the channel interrupts and free the IRQs in
ndo_stop, before freeing the rings and gating the clock? This could use
request_irq()/free_irq() or devm_free_irq(). That alone would also fix
the netdev/priv->hw use-after-free, so it would replace the devres
ordering here rather than build on it.

[Severity: High]
This isn't a bug introduced by this patch, but can the EEE control
timer still be armed when devres frees the netdev that contains it?

sxgbe_release() cancels the timer first. It then calls phy_stop() and
phy_disconnect(), which can sleep, and only later calls napi_disable():

	if (priv->eee_enabled)
		timer_delete_sync(&priv->eee_ctrl_timer);
	...
	napi_disable(&priv->napi);

A NAPI poll in that gap, sxgbe_poll()->sxgbe_tx_all_clean(), can re-arm
the timer:

	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
		sxgbe_enable_eee_mode(priv);
		mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer));
	}

sxgbe_eee_ctrl_timer() re-arms itself on every expiry, and nothing on
the close or remove path deletes it again.

After close, the timer writes MAC registers with the clock gated. After
remove, the timer_list sits in netdev private memory that devres has
freed, so the next expiry is a use-after-free. After module unload, it
runs freed module text.

[Severity: High]
This is a pre-existing issue, but does every re-open of the interface
fail because the IRQs from the previous sxgbe_open() are never freed?

Consider a second sxgbe_open(), from ifdown/ifup or from
sxgbe_change_mtu() on a running interface (which calls sxgbe_release()
and then sxgbe_open()):

- The IRQF_SHARED common and LPI requests succeed and add duplicate
  actions with the same dev_id.

- The first per-TX-queue request is exclusive (flags 0). It fails with
  -EBUSY in __setup_irq() because the earlier action is still
  installed:

	ret = devm_request_irq(priv->device,
			       (priv->txq[queue_num])->irq_no,
			       sxgbe_tx_interrupt, 0,
			       dev->name, priv->txq[queue_num]);

- init_error frees the rings, disconnects the PHY and disables the
  clock. The duplicate shared actions are left in place.

After an MTU change on a running interface, the device stays IFF_UP
with NAPI disabled and the rings freed. A later ifdown, or
unregister_netdev() in sxgbe_drv_remove(), runs sxgbe_release() again.

Would that second napi_disable() spin forever in napi_disable_locked()
while rtnl is held, since NAPIF_STATE_SCHED is already set? Even if it
returned, free_dma_desc_resources() would free the coherent rings and
the rx_skbuff arrays a second time.

[Severity: Low]
This isn't a bug introduced by this patch, but init_tx_ring() allocates
tx_ring->tx_skbuff_dma and tx_ring->tx_skbuff with devm_kcalloc() on
every ndo_open:

	tx_ring->tx_skbuff_dma = devm_kcalloc(dev, tx_rsize,
					      sizeof(dma_addr_t), GFP_KERNEL);
	...
	tx_ring->tx_skbuff = devm_kcalloc(dev, tx_rsize,
					  sizeof(struct sk_buff *), GFP_KERNEL);

free_tx_ring() only releases the coherent ring:

	dma_free_coherent(dev, tx_rsize * sizeof(struct sxgbe_tx_norm_desc),
			  tx_ring->dma_tx, tx_ring->dma_tx_phy);

Does each open attempt, including the failing re-opens above, leave
another pair of arrays attached to the device until unbind?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003085940.493951-2-yaojiale02%40163.com