[PATCH net] net: macb: init workqueues before register_netdev()

[PATCH net] net: macb: init workqueues before register_netdev()

[Théo Lebrun]

register_netdev() exposes the interface to userspace which might trigger
operations like close on it. Those access the HRESP/LPI tasks and
might therefore use them uninitialised.

Fix this race by initialising both `struct work_struct` before
register_netdev().

Theoretical bugfix. The main reason for fix is to avoid future Sashiko
reports which triggers if we grow the race condition (by touching those
workqueues at open for example). The likeliness of this bug sounds
tiny, but I've not spent any time trying to reproduce it.

Fixes: c5092ba3155e ("net: macb: Convert tasklet API to new bottom half workqueue mechanism")
Cc: stable@vger.kernel.org
Reported-by: sashiko <sashiko@sashiko.dev>
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-macb-close-v1-0-05e32ce98813%40bootlin.com
Link: https://lore.kernel.org/netdev/179010942347.2160803.5970158668197373074@kernel.org/
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8e5c034dc3a4..76260b97a07b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5968,15 +5968,15 @@ static int macb_probe(struct platform_device *pdev)
 	if (err)
 		goto err_out_unregister_mdio;
 
+	INIT_WORK(&bp->hresp_err_bh_work, macb_hresp_error_task);
+	INIT_DELAYED_WORK(&bp->tx_lpi_work, macb_tx_lpi_work_fn);
+
 	err = register_netdev(netdev);
 	if (err) {
 		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
 		goto err_out_free_tieoff;
 	}
 
-	INIT_WORK(&bp->hresp_err_bh_work, macb_hresp_error_task);
-	INIT_DELAYED_WORK(&bp->tx_lpi_work, macb_tx_lpi_work_fn);
-
 	netdev_info(netdev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
 		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
 		    netdev->base_addr, netdev->irq, netdev->dev_addr);

---
base-commit: c15c41239b491c7fa380e7ffd115ba037f2d4b11
change-id: 20260925-macb-netdev-register-race-2b4125dd5645

Best regards,
--  
Théo Lebrun <theo.lebrun@bootlin.com>

Re: [PATCH net] net: macb: init workqueues before register_netdev()

[Nicolai Buchwitz]

Hi Théo

On 25.9.2026 15:15, Théo Lebrun wrote:
> register_netdev() exposes the interface to userspace which might 
> trigger
> operations like close on it. Those access the HRESP/LPI tasks and
> might therefore use them uninitialised.
> 
> Fix this race by initialising both `struct work_struct` before
> register_netdev().
> 
> Theoretical bugfix. The main reason for fix is to avoid future Sashiko
> reports which triggers if we grow the race condition (by touching those
> workqueues at open for example). The likeliness of this bug sounds
> tiny, but I've not spent any time trying to reproduce it.
> 
> Fixes: c5092ba3155e ("net: macb: Convert tasklet API to new bottom half 
> workqueue mechanism")

IMHO the "bug" was introduced in 032dc41ba6e2? But this would generate 
more
backporting without any real use. So let's keep it as is.

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

Re: [PATCH net] net: macb: init workqueues before register_netdev()

[Théo Lebrun]

On Fri Sep 25, 2026 at 3:42 PM CEST, Nicolai Buchwitz wrote:
> On 25.9.2026 15:15, Théo Lebrun wrote:
>> register_netdev() exposes the interface to userspace which might 
>> trigger
>> operations like close on it. Those access the HRESP/LPI tasks and
>> might therefore use them uninitialised.
>> 
>> Fix this race by initialising both `struct work_struct` before
>> register_netdev().
>> 
>> Theoretical bugfix. The main reason for fix is to avoid future Sashiko
>> reports which triggers if we grow the race condition (by touching those
>> workqueues at open for example). The likeliness of this bug sounds
>> tiny, but I've not spent any time trying to reproduce it.
>> 
>> Fixes: c5092ba3155e ("net: macb: Convert tasklet API to new bottom half 
>> workqueue mechanism")
>
> IMHO the "bug" was introduced in 032dc41ba6e2? But this would generate 
> more
> backporting without any real use. So let's keep it as is.

I used this command to find the introduction of hresp_err_bh_work and
didn't notice it was only a tasklet to workqueue conversion patch.
You are correct.

   git log --oneline -Shresp_err_bh_work drivers/net/ethernet/cadence/

> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks!

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

Re: [PATCH net] net: macb: init workqueues before register_netdev()

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The Fixes tag points at the wrong commit.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but macb_close()
  never cancels or flushes bp->hresp_err_bh_work before it resets the…
- [High] This problem wasn't introduced by this patch, but macb_init()
  requests each queue IRQ with `devm_request_irq(&pdev->dev, queue->irq,…

--

net: macb: init workqueues before register_netdev()

This patch moves the INIT_WORK() for hresp_err_bh_work and the
INIT_DELAYED_WORK() for tx_lpi_work in macb_probe() ahead of
register_netdev(). Both work items are then initialised before the
interface becomes visible to userspace.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-macb-close-v1-0-05e32ce98813%40bootlin.com
Link: https://lore.kernel.org/netdev/179010942347.2160803.5970158668197373074@kernel.org/

> Theoretical bugfix. The main reason for fix is to avoid future Sashiko
> reports which triggers if we grow the race condition (by touching those
> workqueues at open for example). The likeliness of this bug sounds
> tiny, but I've not spent any time trying to reproduce it.
>
> Fixes: c5092ba3155e ("net: macb: Convert tasklet API to new bottom half workqueue mechanism")
> Cc: stable@vger.kernel.org

[Severity: Medium]
Does the Fixes tag point at the right commit? In macb_probe(),
c5092ba3155e only swapped tasklet_setup() on hresp_err_tasklet for
INIT_WORK() on hresp_err_bh_work. The call was already after
register_netdev() and stayed there.

The HRESP handler has been set up after register_netdev() since
032dc41ba6e2 ("net: macb: Handle HRESP error"). That commit added
tasklet_init() right after the register_netdev() error check, and added
tasklet_schedule() in macb_interrupt() at the same time.

The tx_lpi_work half, which the commit message names, comes from
0cc425f18f59 ("net: cadence: macb: implement EEE TX LPI support").

With Cc: stable, the Fixes tag decides where this lands. Older LTS trees
that have the tasklet version of the same ordering would not get the fix.
In those trees, a zeroed tasklet could reach t->func(t->data) with a NULL
func, or tasklet_init() could re-initialise a tasklet that is already
queued.

Trees that have c5092ba3155e but not 0cc425f18f59 would also get a hunk
that moves an INIT_DELAYED_WORK(&bp->tx_lpi_work, ...) line they don't
have.

Would Fixes: 032dc41ba6e2 for the HRESP work plus Fixes: 0cc425f18f59 for
the LPI work be more accurate? Alternatively, could the message say which
trees it is meant to cover?

The message also describes this as a theoretical fix that hasn't been
reproduced, meant mainly to avoid future Sashiko reports. The stable kernel
rules usually don't accept theoretical races unless a trigger is described.
Is the Cc: stable intended?

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 8e5c034dc3a48..76260b97a07b6 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -5968,15 +5968,15 @@ static int macb_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_out_unregister_mdio;
>  
> +	INIT_WORK(&bp->hresp_err_bh_work, macb_hresp_error_task);

[Severity: High]
This isn't a bug introduced by this patch, but it concerns the same work
item: does macb_close() wait for hresp_err_bh_work before it frees the
rings?

macb_close() only cancels tx_lpi_work:

macb_close() {
    ...
	cancel_delayed_work_sync(&bp->tx_lpi_work);
    ...
	spin_lock_irqsave(&bp->lock, flags);
	macb_reset_hw(bp);
	netif_carrier_off(netdev);
	spin_unlock_irqrestore(&bp->lock, flags);

	macb_free(bp);
    ...
}

Suppose an HRESP interrupt arrives just before dev_close() clears
__LINK_STATE_START. macb_interrupt_misc() then does:

	queue_work(system_bh_wq, &bp->hresp_err_bh_work);

macb_hresp_error_task() takes no lock and does not check netif_running().
If it runs during or after macb_free(), it does this:

	bp->macbgem_ops.mog_init_rings(bp);

	/* Initialize TX and RX buffers */
	macb_init_buffers(bp);

By then macb_free() has already released the descriptor rings with
dma_free_coherent(). The task then sets RE/TE and IER again, and calls
netif_carrier_on() and netif_tx_start_all_queues() on a closed device.

Can this write into the freed DMA descriptor memory, and restart DMA with
the controller's queue pointers still pointing at it?

dev_deactivate_many() calls synchronize_net() only when sync_needed is set.
With a noqueue root qdisc there is no grace period at all. Even with
synchronize_net(), a BH work that is queued but hasn't started yet is not
waited for (softirq deferred to ksoftirqd, threadirqs, PREEMPT_RT).

The only cancel_work_sync() for this work is in macb_remove(). It runs
after unregister_netdev() has already gone through macb_close() and
macb_free(), so the remove path has the same window:

macb_remove() {
    ...
	unregister_netdev(netdev);
    ...
	cancel_work_sync(&bp->hresp_err_bh_work);
    ...
}

> +	INIT_DELAYED_WORK(&bp->tx_lpi_work, macb_tx_lpi_work_fn);
> +
>  	err = register_netdev(netdev);
>  	if (err) {
>  		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
>  		goto err_out_free_tieoff;
>  	}

[Severity: High]
This is a pre-existing issue, but the err_out_free_tieoff path ends in
free_netdev() while the queue IRQs requested by macb_init() are still
registered. Can macb_interrupt() then run with a dev_id that has been
freed?

macb_init() requests each queue IRQ as a devm-managed shared IRQ, with a
dev_id inside netdev_priv():

	err = devm_request_irq(&pdev->dev, queue->irq, macb_interrupt,
			       IRQF_SHARED, netdev->name, queue);

Here queue is &bp->queues[q]. Devres releases the IRQs only after probe or
remove returns. Before that, free_netdev() runs on two kinds of paths:

  - the probe error labels after macb_init(): err_out_phy_exit and
    err_out_free_netdev, plus err_out_free_tieoff, which falls through to
    them
  - macb_remove()

With CONFIG_DEBUG_SHIRQ, __free_irq() calls the handler for IRQF_SHARED
actions:

	action->handler(irq, dev_id);

The first thing macb_interrupt() does is queue_readl(queue, ISR), which
expands to:

	(queue)->bp->macb_reg_readl((queue)->bp, (queue)->reg)

That is an indirect call through a function pointer loaded from freed
memory.

Without DEBUG_SHIRQ, the same can happen if another device on the shared
line raises an interrupt in that window. If ISR reads as non-zero,
spin_lock(&bp->lock) also writes into freed memory.

Should the IRQs be freed or disabled on these paths before free_netdev(),
for example with devm_free_irq()?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-macb-netdev-register-race-v1-1-752f90ce1c39%40bootlin.com

Re: [PATCH net] net: macb: init workqueues before register_netdev()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 25 Sep 2026 15:15:08 +0200 you wrote:
> register_netdev() exposes the interface to userspace which might trigger
> operations like close on it. Those access the HRESP/LPI tasks and
> might therefore use them uninitialised.
> 
> Fix this race by initialising both `struct work_struct` before
> register_netdev().
> 
> [...]

Here is the summary with links:
  - [net] net: macb: init workqueues before register_netdev()
    https://git.kernel.org/netdev/net-next/c/0ac396fbc25c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html