[PATCH net 1/3] net: atl1: disable work before freeing the netdev

[PATCH net 1/3] net: atl1: disable work before freeing the netdev

[BeaCox]

atl1_remove() frees the netdev without disabling reset_dev_task or
link_chg_task.  reset_dev_task is queued from the interrupt handler for
link-down and DMA errors and from atlx_tx_timeout(); link_chg_task is
queued from the interrupt handler when ISR_GPHY reports a link event.
Both callbacks dereference the adapter and the netdev, so a work that is
still pending or running after free_netdev() touches freed memory.

The original driver never cancelled these works, and the current
reset_dev_task was introduced when the two reset tasks were merged in
03662e41c7cf ("atl1: fix kernel panic in case of DMA errors").

Disable both works synchronously before unregistering the interface,
then delete the PHY configuration timer.  disable_work_sync() drains a
queued or running instance and rejects a later queue_work(), which a
plain cancel_work_sync() would not.  Deleting the timer after the works
have stopped also prevents a running link work from rearming it.
atl2_remove() cancels both works and deletes the timer before
unregistering for the same reason.

Fixes: f3cc28c79760 ("Add Attansic L1 ethernet driver.")
Cc: Chris Snook <chris.snook@gmail.com>
Cc: stable@vger.kernel.org # 6.10+
Signed-off-by: BeaCox <0xbeac@gmail.com>
---
 drivers/net/ethernet/atheros/atlx/atl1.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c b/drivers/net/ethernet/atheros/atlx/atl1.c
index 2883e9462f..2bd35c3cfc 100644
--- a/drivers/net/ethernet/atheros/atlx/atl1.c
+++ b/drivers/net/ethernet/atheros/atlx/atl1.c
@@ -3132,6 +3132,18 @@ static void atl1_remove(struct pci_dev *pdev)
 
 	adapter = netdev_priv(netdev);
 
+	/*
+	 * reset_dev_task is queued from the interrupt handler and from the
+	 * tx watchdog; link_chg_task is queued from the interrupt handler
+	 * when ISR_GPHY reports a link event.  Disable both before
+	 * unregistering the interface so that neither a pending nor a late
+	 * queue attempt can run after free_netdev().
+	 */
+	disable_work_sync(&adapter->reset_dev_task);
+	disable_work_sync(&adapter->link_chg_task);
+	timer_delete_sync(&adapter->phy_config_timer);
+	adapter->phy_timer_pending = false;
+
 	/*
 	 * Some atl1 boards lack persistent storage for their MAC, and get it
 	 * from the BIOS during POST.  If we've been messing with the MAC
-- 
2.43.0

Re: [PATCH net 1/3] net: atl1: disable work before freeing the netdev

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · 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 commit message offers atl2_remove() as a correct sibling:
  "atl2_remove() cancels both works and deletes the timer before…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  atl1_reset_dev_task()…

--

net: atl1: disable work before freeing the netdev

This patch changes atl1_remove() to call disable_work_sync() on
reset_dev_task and link_chg_task before unregister_netdev(). After that,
it deletes the PHY config timer. The goal is to stop either work from
running after free_netdev().

> Disable both works synchronously before unregistering the interface,
> then delete the PHY configuration timer.  disable_work_sync() drains a
> queued or running instance and rejects a later queue_work(), which a
> plain cancel_work_sync() would not.  Deleting the timer after the works
> have stopped also prevents a running link work from rearming it.
> atl2_remove() cancels both works and deletes the timer before
> unregistering for the same reason.

[Severity: Medium]
Is atl2_remove() a correct precedent here? By the reasoning in this
message, it looks like atl2 still has the same use-after-free:

drivers/net/ethernet/atheros/atlx/atl2.c:atl2_remove() {
    ...
	set_bit(__ATL2_DOWN, &adapter->flags);

	timer_delete_sync(&adapter->watchdog_timer);
	timer_delete_sync(&adapter->phy_config_timer);
	cancel_work_sync(&adapter->reset_task);
	cancel_work_sync(&adapter->link_chg_task);

	unregister_netdev(netdev);
    ...
}

When the cancel_work_sync() calls run, the IRQ is still requested. It is
only freed by atl2_close(), from inside unregister_netdev(), and
dev_watchdog is still active as well. That leaves these paths able to
queue work again after the cancels:

  atl2_intr()
    schedule_work(&adapter->reset_task);      (link-down and DMA errors)
  atl2_intr() -> atl2_check_for_link()
    schedule_work(&adapter->link_chg_task);   (ISR_PHY or ISR_MANUAL)
  dev_watchdog -> atl2_tx_timeout()
    schedule_work(&adapter->reset_task);

None of these check __ATL2_DOWN. atl2_close() and atl2_down() never
cancel the works, so anything queued in that window can still be pending
at free_netdev().

The ordering is also the reverse of what the message says is needed.
atl2 deletes the timers before it cancels the works, and relies on
__ATL2_DOWN to block rearming. But atl2_reset_task() ->
atl2_reinit_locked() -> atl2_up() runs:

	clear_bit(__ATL2_DOWN, &adapter->flags);

atl2_up() also sets MASTER_CTRL_MANUAL_INT, which can raise a manual
interrupt that queues link_chg_task again.

No other patch in the series touches atl2.c. Should the message stop
describing atl2_remove() as equivalent, or should atl2_remove() get the
same conversion?

> diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c b/drivers/net/ethernet/atheros/atlx/atl1.c
> index 2883e9462f6fd..2bd35c3cfc820 100644
> --- a/drivers/net/ethernet/atheros/atlx/atl1.c
> +++ b/drivers/net/ethernet/atheros/atlx/atl1.c
> @@ -3132,6 +3132,18 @@ static void atl1_remove(struct pci_dev *pdev)
>  
>  	adapter = netdev_priv(netdev);
>  
> +	/*
> +	 * reset_dev_task is queued from the interrupt handler and from the
> +	 * tx watchdog; link_chg_task is queued from the interrupt handler
> +	 * when ISR_GPHY reports a link event.  Disable both before
> +	 * unregistering the interface so that neither a pending nor a late
> +	 * queue attempt can run after free_netdev().
> +	 */
> +	disable_work_sync(&adapter->reset_dev_task);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can
atl1_reset_dev_task() race with atl1_close() and the ethtool
reconfiguration paths? It takes no lock and does not check
netif_running():

drivers/net/ethernet/atheros/atlx/atl1.c:atl1_reset_dev_task() {
    ...
	netif_device_detach(netdev);
	atl1_down(adapter);
	atl1_up(adapter);
	netif_device_attach(netdev);
}

atl1_close() (ndo_stop), atl1_change_mtu(), atl1_set_link_ksettings(),
atl1_set_ringparam() and atl1_nway_reset() all run atl1_down() and
atl1_up() under RTNL. Nothing serializes them against this work, and
atl1_close() does not cancel it:

drivers/net/ethernet/atheros/atlx/atl1.c:atl1_close() {
    ...
	atl1_down(adapter);
	atl1_free_ring_resources(adapter);
	return 0;
}

Here is one possible sequence:

  atlx_tx_timeout() or atl1_intr()
    queue reset_dev_task
  ip link set dev ethX down
    __dev_close_many()           (no netif_device_present() check)
      atl1_close()
        atl1_down()              napi_disable(), free_irq(), ring cleanup
        atl1_free_ring_resources()
  kworker: atl1_reset_dev_task()
    atl1_down()
      napi_disable()
        netdev_lock(n->dev);
        napi_disable_locked(n);  spins forever, SCHED never clears

When that happens, the kworker blocks while holding the netdev instance
lock. A later atl1_open() -> atl1_up() -> napi_enable() -> netdev_lock()
would then deadlock with RTNL held.

If the work instead runs at the same time as close or ethtool -G, both
CPUs end up in atl1_down(). That can call free_irq() twice and double
free skbs in atl1_clean_rx_ring() and atl1_clean_tx_ring(). It can also
let atl1_set_ringparam() or atl1_close() free the descriptor rings while
the worker's atl1_up() is refilling them in atl1_alloc_rx_buffers().

With this patch applied, this disable_work_sync() would also wait forever
on a worker stuck that way. Should the reset work take rtnl_lock() and
check netif_running(), as other drivers do in their reset tasks?

> +	disable_work_sync(&adapter->link_chg_task);
> +	timer_delete_sync(&adapter->phy_config_timer);
> +	adapter->phy_timer_pending = false;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104063245.23402-1-0xbeac%40gmail.com

[PATCH net 2/3] net: enc28j60: disable work before freeing the netdev

[BeaCox]

enc28j60_remove() unregisters the interface, frees the IRQ and frees the
netdev, but tx_work, setrx_work and restart_work can still be pending or
running.  tx_work is queued from enc28j60_send_packet(), setrx_work from
enc28j60_set_multicast_list(), and restart_work from
enc28j60_tx_timeout().  The handlers access the private data, the SPI
device and the netdev, all of which are freed with the netdev.

Unregister the interface first so the xmit and set_rx_mode paths can no
longer queue work, then disable the three works synchronously before
freeing the IRQ and the netdev.  disable_work_sync() drains a queued or
running instance and rejects a later queue_work(), which a plain
cancel_work_sync() would not.

Fixes: 3ec9c11da033 ("add driver for enc28j60 ethernet chip")
Cc: stable@vger.kernel.org # 6.10+
Signed-off-by: BeaCox <0xbeac@gmail.com>
---
 drivers/net/ethernet/microchip/enc28j60.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c
index d6c9491537..bb29a94a1e 100644
--- a/drivers/net/ethernet/microchip/enc28j60.c
+++ b/drivers/net/ethernet/microchip/enc28j60.c
@@ -1600,7 +1600,16 @@ static void enc28j60_remove(struct spi_device *spi)
 {
 	struct enc28j60_net *priv = spi_get_drvdata(spi);
 
+	/*
+	 * Stop the interface first so the xmit and set_rx_mode paths can no
+	 * longer queue new work.  Then drain the works that were already
+	 * queued; the IRQ handler does not queue work, so the IRQ can be
+	 * released after the drain and before free_netdev().
+	 */
 	unregister_netdev(priv->netdev);
+	disable_work_sync(&priv->tx_work);
+	disable_work_sync(&priv->setrx_work);
+	disable_work_sync(&priv->restart_work);
 	free_irq(spi->irq, priv);
 	free_netdev(priv->netdev);
 }
-- 
2.43.0

Re: [PATCH net 2/3] net: enc28j60: disable work before freeing the netdev

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] The skb in flight is leaked when the device is removed, and
  the new teardown makes this worse.
- [Medium] net: enc28j60: tx_work and setrx_work run after
  enc28j60_lowpower() sleep
- [Low] The commit message justifies disable_work_sync() over
  cancel_work_sync() because it "rejects a later queue_work(), which a…

--

net: enc28j60: disable work before freeing the netdev

This commit adds disable_work_sync() calls for tx_work, setrx_work and
restart_work in enc28j60_remove(). They run after unregister_netdev() and
before free_irq() and free_netdev(). The goal is to stop the work
handlers from touching freed private data when they are pending or
running at remove time.

> Unregister the interface first so the xmit and set_rx_mode paths can no
> longer queue work, then disable the three works synchronously before
> freeing the IRQ and the netdev.  disable_work_sync() drains a queued or
> running instance and rejects a later queue_work(), which a plain
> cancel_work_sync() would not.
>
> Fixes: 3ec9c11da033 ("add driver for enc28j60 ethernet chip")
> Cc: stable@vger.kernel.org # 6.10+

[Severity: Low]
Is the "rejects a later queue_work()" justification accurate for this
driver? The only schedule_work() callers are ndo callbacks:

    enc28j60_send_packet()        -> tx_work      (ndo_start_xmit)
    enc28j60_tx_timeout()         -> restart_work (ndo_tx_timeout)
    enc28j60_set_multicast_list() -> setrx_work   (ndo_set_rx_mode)

None of these can run once unregister_netdev() has gone through
dev_close() and dev_deactivate(). The threaded IRQ handler queues no
work, and none of the three work handlers queues another work.
enc28j60_restart_work_handler() also checks netif_running() under rtnl.

The new comment in enc28j60_remove() says the same thing: after
unregister_netdev() the xmit and set_rx_mode paths "can no longer queue
new work". So it looks like cancel_work_sync() would be just as correct
here.

Because the patch uses disable_work_sync(), it carries "# 6.10+", while
the Fixes: tag points at the original driver. The 5.10, 5.15, 6.1 and
6.6 stable trees have the same use-after-free on remove.

Could the commit message be corrected? Would using cancel_work_sync()
let the fix be backported to those older trees too?

> diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c
> index d6c9491537e47..bb29a94a1e2c1 100644
> --- a/drivers/net/ethernet/microchip/enc28j60.c
> +++ b/drivers/net/ethernet/microchip/enc28j60.c
> @@ -1600,7 +1600,16 @@ static void enc28j60_remove(struct spi_device *spi)
>  {
>  	struct enc28j60_net *priv = spi_get_drvdata(spi);
>  
> +	/*
> +	 * Stop the interface first so the xmit and set_rx_mode paths can no
> +	 * longer queue new work.  Then drain the works that were already
> +	 * queued; the IRQ handler does not queue work, so the IRQ can be
> +	 * released after the drain and before free_netdev().
> +	 */
>  	unregister_netdev(priv->netdev);
> +	disable_work_sync(&priv->tx_work);

[Severity: Medium]
Does draining tx_work and setrx_work only after unregister_netdev()
still leave a window where they touch the chip after it has been put
to sleep?

When the interface is up, unregister_netdev() goes through dev_close()
and calls enc28j60_net_close() before any of the new
disable_work_sync() calls run:

    enc28j60_hw_disable(priv);
    enc28j60_lowpower(priv, true);
    netif_stop_queue(dev);

enc28j60_lowpower() clears ECON1_RXEN and waits for ESTAT_RXBUSY and
ECON1_TXRTS to clear. Then it sets ECON2_PWRSV. Nothing in
enc28j60_net_close() cancels or waits for tx_work or setrx_work. An
instance queued by enc28j60_send_packet() or
enc28j60_set_multicast_list() just before the interface went down can
still run after enc28j60_lowpower() has returned.

In that case enc28j60_tx_work_handler() calls enc28j60_hw_tx(). That
writes the frame into the TX buffer and sets ECON1_TXRTS on a chip that
is in power-save mode with EIE cleared. This is the state that
enc28j60_lowpower() polled to avoid. enc28j60_setrx_work_handler()
likewise writes ERXFCON after the chip went to sleep. No TX interrupt
follows, so priv->tx_skb is also left behind, as in the leak noted
above.

The disable_work_sync() calls in enc28j60_remove() come too late for
this. They cannot undo a handler that already ran between ndo_stop and
the disable. The same window exists on a plain ifdown, and on the
close/open cycle in enc28j60_restart_work_handler().

Would it be better to cancel tx_work and setrx_work in
enc28j60_net_close() before enc28j60_hw_disable(), and free any pending
priv->tx_skb there? Those two handlers appear to take only priv->lock,
so cancel_work_sync() under rtnl looks safe for them. That is not true
of restart_work, which takes rtnl_lock itself.

[Severity: Medium]
Can this leak the skb held in priv->tx_skb?

After enc28j60_send_packet() hands the skb to tx_work and returns
NETDEV_TX_OK, the driver owns it:

    priv->tx_skb = skb;
    schedule_work(&priv->tx_work);

    return NETDEV_TX_OK;

The skb is freed in only one place, enc28j60_tx_clear(). That function is
reached only from the TXIF/TXERIF handling in enc28j60_irq():

    dev_kfree_skb(priv->tx_skb);
    priv->tx_skb = NULL;

By the time disable_work_sync() runs, this call chain has already masked
all chip interrupts:

  unregister_netdev()->dev_close()->enc28j60_net_close()->enc28j60_hw_disable()

    nolock_regb_write(priv, EIE, 0x00);

If tx_work is still pending, disable_work_sync() cancels it without
running it. If tx_work runs during or after close, the frame is written
to the chip with interrupts masked, so no TX-complete interrupt follows.

Either way, free_netdev() frees priv while priv->tx_skb is still set.
Nothing frees the skb, and its socket wmem charge is never released.

The new comment says this code will "drain the works that were already
queued". But disable_work_sync() cancels a pending instance and only
waits for one that is already running.

The skb could be freed after the disable_work_sync() calls and after
free_irq(), since the IRQ thread can still free it and clear the
pointer. For example, just before free_netdev():

    dev_kfree_skb(priv->tx_skb);
    priv->tx_skb = NULL;

Would that work?

> +	disable_work_sync(&priv->setrx_work);
> +	disable_work_sync(&priv->restart_work);
>  	free_irq(spi->irq, priv);
>  	free_netdev(priv->netdev);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104063245.23402-1-0xbeac%40gmail.com

[PATCH net 3/3] net: xgmac: disable timeout work before teardown

[BeaCox]

xgmac_remove() frees the IRQ lines and unmaps the registers before
freeing the netdev, but tx_timeout_work can still be pending or running.
The work is queued from ndo_tx_timeout() and from the DMA error
interrupt path, and its handler disables NAPI, programs the DMA
registers and walks the TX ring.

xgmac_stop() has the same lifetime problem: it frees the DMA descriptor
rings while a queued timeout work can still run.

Disable the work in probe after INIT_WORK() so open/resume can enable it,
disable it synchronously in xgmac_stop() before the rings are freed and
in xgmac_suspend() before NAPI/interrupts are torn down, enable it in
xgmac_open() and xgmac_resume(), and disable it in xgmac_remove() before
the IRQs and MMIO are released.  The enable/disable pairing avoids the
underflow warning that enable_work() emits when the work was never
disabled.  disable_work_sync() also rejects queue_work() from a late
interrupt, which cancellation alone would not.

The patch does not change the existing free_irq()/unregister_netdev()
ordering in xgmac_remove(); it only prevents the timeout work from
outliving the netdev.

Fixes: 8746f671ef04 ("net: calxedaxgmac: fix race between xgmac_tx_complete and xgmac_tx_err")
Cc: stable@vger.kernel.org # 6.10+
Signed-off-by: BeaCox <0xbeac@gmail.com>
---
 drivers/net/ethernet/calxeda/xgmac.c | 30 ++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
index a2410fba6b..9b7aee56f2 100644
--- a/drivers/net/ethernet/calxeda/xgmac.c
+++ b/drivers/net/ethernet/calxeda/xgmac.c
@@ -1023,6 +1023,9 @@ static int xgmac_open(struct net_device *dev)
 	if (ret < 0)
 		return ret;
 
+	/* xgmac_stop() disables this work before freeing the rings. */
+	enable_work(&priv->tx_timeout_work);
+
 	/* Enable the MAC Rx/Tx */
 	xgmac_mac_enable(ioaddr);
 
@@ -1046,6 +1049,13 @@ static int xgmac_stop(struct net_device *dev)
 {
 	struct xgmac_priv *priv = netdev_priv(dev);
 
+	/*
+	 * The timeout work programs the DMA rings that are freed below, and
+	 * it toggles NAPI.  Disable it synchronously before touching either;
+	 * xgmac_open() and xgmac_resume() enable it again.
+	 */
+	disable_work_sync(&priv->tx_timeout_work);
+
 	if (readl(priv->base + XGMAC_DMA_INTR_ENA))
 		napi_disable(&priv->napi);
 
@@ -1713,6 +1723,12 @@ static int xgmac_probe(struct platform_device *pdev)
 	ndev->ethtool_ops = &xgmac_ethtool_ops;
 	spin_lock_init(&priv->stats_lock);
 	INIT_WORK(&priv->tx_timeout_work, xgmac_tx_timeout_work);
+	/*
+	 * The work is enabled while the interface is up.  Start it disabled
+	 * so the enable_work() in xgmac_open()/xgmac_resume() has a matching
+	 * disable and never underflows.
+	 */
+	disable_work(&priv->tx_timeout_work);
 
 	priv->device = &pdev->dev;
 	priv->dev = ndev;
@@ -1823,6 +1839,13 @@ static void xgmac_remove(struct platform_device *pdev)
 	struct xgmac_priv *priv = netdev_priv(ndev);
 	struct resource *res;
 
+	/*
+	 * The timeout work can be queued from ndo_tx_timeout() and from the
+	 * DMA error interrupt.  Disable it before the IRQs are released and
+	 * the registers are unmapped.
+	 */
+	disable_work_sync(&priv->tx_timeout_work);
+
 	xgmac_mac_disable(priv->base);
 
 	/* Free the IRQ lines */
@@ -1862,6 +1885,12 @@ static int xgmac_suspend(struct device *dev)
 		return 0;
 
 	netif_device_detach(ndev);
+	/*
+	 * The timeout work touches the DMA registers and toggles NAPI.
+	 * Disable it before NAPI and the interrupts are torn down;
+	 * xgmac_resume() enables it again.
+	 */
+	disable_work_sync(&priv->tx_timeout_work);
 	napi_disable(&priv->napi);
 	writel(0, priv->base + XGMAC_DMA_INTR_ENA);
 
@@ -1891,6 +1920,7 @@ static int xgmac_resume(struct device *dev)
 
 	/* Enable the MAC and DMA */
 	xgmac_mac_enable(ioaddr);
+	enable_work(&priv->tx_timeout_work);
 	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_STATUS);
 	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_INTR_ENA);
 
-- 
2.43.0

Re: [PATCH net 3/3] net: xgmac: disable timeout work before teardown

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 2 · 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] xgmac_resume() can deadlock against the timeout work.
- [Medium] The workqueue disable count can get stuck above zero and stay
  that way, so TX-hang recovery is lost for good.
- [Medium] In xgmac_suspend(), netif_device_detach() (line 1887) runs
  before disable_work_sync() (line 1893).
- [Low] The new comments describe the enable/disable pairing wrongly.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits on the
  same stop/failed-open path.

--

net: xgmac: disable timeout work before teardown

This patch disables tx_timeout_work at probe and re-enables it in
xgmac_open() and xgmac_resume(). It also synchronously disables the work
in xgmac_stop(), xgmac_suspend() and xgmac_remove(). The goal is to keep
the TX timeout work from running against freed DMA rings, released IRQs
or unmapped registers.

> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
> index a2410fba6be2e..9b7aee56f2610 100644
> --- a/drivers/net/ethernet/calxeda/xgmac.c
> +++ b/drivers/net/ethernet/calxeda/xgmac.c
> @@ -1023,6 +1023,9 @@ static int xgmac_open(struct net_device *dev)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* xgmac_stop() disables this work before freeing the rings. */
> +	enable_work(&priv->tx_timeout_work);
> +

[Severity: Medium]
Can the disable count on tx_timeout_work get stuck above zero here?

disable_work() and enable_work() nest, so every xgmac_stop() needs a
matching xgmac_open() that succeeds. xgmac_change_mtu() does:

	xgmac_stop(dev);
	WRITE_ONCE(dev->mtu, new_mtu);
	return xgmac_open(dev);

If xgmac_dma_desc_rings_init() fails with -ENOMEM, xgmac_open() returns
before enable_work() and the count stays at 1. A failed ndo_change_mtu
does not close the device, so it stays running.

A later ifdown calls xgmac_stop() again, and disable_work_sync() raises
the count to 2. The next ifup succeeds, but its enable_work() only brings
the count back to 1.

>From then on, schedule_work() from xgmac_tx_timeout() and from the
DMA_STATUS_TPS path in xgmac_interrupt() does nothing until the driver
is rebound. Without this patch, a down/up cycle fully recovered the
device.

Would enabling the work on the open failure path, or tracking whether it
is currently enabled, avoid this?

[Severity: High]
This isn't a bug introduced by this patch, but it is on the same
xgmac_change_mtu() stop/failed-open path. The error labels in
xgmac_dma_desc_rings_init() free the buffers without clearing the
pointers:

err_dma_tx:
	kfree(priv->tx_skbuff);
err_tx_skb:
	dma_free_coherent(priv->device,
			  DMA_RX_RING_SZ * sizeof(struct xgmac_dma_desc),
			  priv->dma_rx, priv->dma_rx_phy);
err_dma_rx:
	kfree(priv->rx_skbuff);
	return -ENOMEM;

When xgmac_change_mtu()->xgmac_open() fails here, the interface stays
running while priv->rx_skbuff, priv->dma_rx and priv->tx_skbuff can
point at freed memory.

The next ndo_stop then goes through
xgmac_stop()->xgmac_free_dma_desc_rings(). There,
xgmac_free_rx_skbufs() walks the freed rx_skbuff[] array, and
dma_free_coherent() on priv->dma_rx and kfree() on
priv->rx_skbuff/priv->tx_skbuff can run a second time.

Could these error labels set the pointers to NULL after freeing them?

>  	/* Enable the MAC Rx/Tx */
>  	xgmac_mac_enable(ioaddr);
>  
> @@ -1046,6 +1049,13 @@ static int xgmac_stop(struct net_device *dev)
>  {
>  	struct xgmac_priv *priv = netdev_priv(dev);
>  
> +	/*
> +	 * The timeout work programs the DMA rings that are freed below, and
> +	 * it toggles NAPI.  Disable it synchronously before touching either;
> +	 * xgmac_open() and xgmac_resume() enable it again.
> +	 */

[Severity: Low]
This isn't a bug, but the comment here and the one added in
xgmac_probe() describe a pairing that doesn't match the code. This
comment says "xgmac_open() and xgmac_resume() enable it again". The
probe comment says the probe-time disable_work() is the matching disable
for "the enable_work() in xgmac_open()/xgmac_resume()".

xgmac_resume() returns early when !netif_running(ndev), and the device
is not running after dev_close()->ndo_stop. So the enable in resume can
never balance a disable done in xgmac_stop() or probe. It is balanced by
the disable_work_sync() in xgmac_suspend().

Could the comments describe the actual pairs, probe/stop with open and
suspend with resume?

> +	disable_work_sync(&priv->tx_timeout_work);
> +

[ ... ]

> @@ -1862,6 +1885,12 @@ static int xgmac_suspend(struct device *dev)
>  		return 0;
>  
>  	netif_device_detach(ndev);
> +	/*
> +	 * The timeout work touches the DMA registers and toggles NAPI.
> +	 * Disable it before NAPI and the interrupts are torn down;
> +	 * xgmac_resume() enables it again.
> +	 */
> +	disable_work_sync(&priv->tx_timeout_work);

[Severity: Medium]
Should disable_work_sync() come before netif_device_detach() here?

If xgmac_tx_timeout_work() is already running when xgmac_suspend() is
called, disable_work_sync() waits for it to finish. The work ends with:

	netif_tx_unlock(priv->dev);
	netif_wake_queue(priv->dev);

That undoes the netif_tx_stop_all_queues() done by netif_device_detach().
Nothing later in xgmac_suspend() stops the queue again. xgmac_stop(), by
contrast, calls netif_tx_disable().

The qdisc path (sch_direct_xmit()->dev_hard_start_xmit()) doesn't check
netif_device_present(). Can xgmac_xmit() then write descriptors and
XGMAC_DMA_TX_POLL while NAPI, interrupts and the MAC/DMA are being shut
down for suspend?

>  	napi_disable(&priv->napi);
>  	writel(0, priv->base + XGMAC_DMA_INTR_ENA);
>  
> @@ -1891,6 +1920,7 @@ static int xgmac_resume(struct device *dev)
>  
>  	/* Enable the MAC and DMA */
>  	xgmac_mac_enable(ioaddr);
> +	enable_work(&priv->tx_timeout_work);
>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_STATUS);
>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_INTR_ENA);
>  

[Severity: High]
Can system resume deadlock against the timeout work here?

At this point NAPI is still disabled from xgmac_suspend(). The work is
already enabled, and DMA_INTR_DEFAULT_MASK (which includes TSE) is
unmasked before napi_enable() runs. A "transmit process stopped"
interrupt in this window takes this path in xgmac_interrupt():

	if (intr_status & DMA_STATUS_TPS) {
		...
		schedule_work(&priv->tx_timeout_work);
	}

The work then starts with napi_disable() on a NAPI that is already
disabled:

worker
xgmac_tx_timeout_work()
  napi_disable()
    netdev_lock(n->dev)
    napi_disable_locked()
      loops in usleep_range() while NAPIF_STATE_SCHED is set

resume
xgmac_resume()
  napi_enable()
    netdev_lock(n->dev)    <- blocks on the worker

Only napi_enable_locked() clears SCHED, and it needs the netdev_lock the
worker holds. Neither side can make progress.

This window existed before the patch. However, the suspend hunk is meant
to keep the work from toggling NAPI while it is down, and the
enable_work() placement here leaves it open. xgmac_open() also enables
the work before napi_enable(), but there the DMA interrupts stay masked
until after napi_enable().

Would moving enable_work() and the interrupt unmask after napi_enable()
in xgmac_resume() avoid this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104063245.23402-1-0xbeac%40gmail.com