Re: [PATCH net] net: calxeda: cancel timeout work before freeing rings

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete: tx_timeout_work can be queued again after
  the new cancel_work_sync() in xgmac_stop()…
- [High] Two more paths leave the worker lifetime bug reachable.
- [High] The patch moves netif_tx_disable() before napi_disable() in
  xgmac_stop() (line 1049 vs 1052).
- [Medium] The patch removes the `if (readl(priv->base +
  XGMAC_DMA_INTR_ENA)) napi_disable()` guard and calls…

--

net: calxeda: cancel timeout work before freeing rings

This commit reorders xgmac_stop(). Transmit is now stopped and the DMA
interrupts are masked first, then tx_timeout_work is cancelled, and only
then is NAPI disabled and the rings freed. It also removes the check on
XGMAC_DMA_INTR_ENA that decided whether napi_disable() was called.

> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
> index a2410fba6be2..63296469bd85 100644
> --- a/drivers/net/ethernet/calxeda/xgmac.c
> +++ b/drivers/net/ethernet/calxeda/xgmac.c
> @@ -1046,12 +1046,10 @@ static int xgmac_stop(struct net_device *dev)
>  {
>  	struct xgmac_priv *priv = netdev_priv(dev);
>  
> -	if (readl(priv->base + XGMAC_DMA_INTR_ENA))
> -		napi_disable(&priv->napi);
> -

[Severity: Medium]
Is it safe to drop this guard? napi_disable() now runs every time, but
xgmac_open() can fail and leave NAPI disabled:

xgmac_open() {
    ...
	xgmac_hw_init(dev);     <- writes 0 to XGMAC_DMA_INTR_ENA
    ...
	ret = xgmac_dma_desc_rings_init(dev);
	if (ret < 0)
		return ret;
    ...
	napi_enable(&priv->napi);
    ...
}

On the xgmac_change_mtu() path, xgmac_stop() has already disabled NAPI.
If xgmac_dma_desc_rings_init() then returns -ENOMEM, netif_set_mtu_ext()
returns the error and the device stays up.

The next ifdown or MTU change calls xgmac_stop() again. Would that
napi_disable() on an already disabled NAPI spin forever in
napi_disable_locked() while holding RTNL and netdev_lock?

The old readl() check saw 0 in INTR_ENA in this state and skipped the
second disable.

> -	writel(0, priv->base + XGMAC_DMA_INTR_ENA);
> -
>  	netif_tx_disable(dev);
> +	writel(0, priv->base + XGMAC_DMA_INTR_ENA);

[Severity: High]
Can XGMAC_DMA_INTR_ENA be turned back on after this write? This is the
only place xgmac_stop() masks the interrupts. Both contexts it then waits
for write the mask back.

A poll that is still running when the mask is written finishes with:

xgmac_poll() {
    ...
	if (work_done < budget) {
		napi_complete_done(napi, work_done);
		__raw_writel(DMA_INTR_DEFAULT_MASK, priv->base + XGMAC_DMA_INTR_ENA);
	}
    ...
}

With the old order, napi_disable() ran before writel(0), so a poll could
not undo the mask.

A tx_timeout_work that is already running, which cancel_work_sync() waits
for, ends with:

xgmac_tx_timeout_work() {
    ...
	netif_wake_queue(priv->dev);

	napi_enable(&priv->napi);

	/* Enable interrupts */
	writel(DMA_INTR_DEFAULT_MASK, priv->base + XGMAC_DMA_STATUS);
	writel(DMA_INTR_DEFAULT_MASK, priv->base + XGMAC_DMA_INTR_ENA);
}

An xgmac_interrupt() still running on another CPU can also write
DMA_INTR_ABNORMAL to INTR_ENA, or call schedule_work() itself. There is no
synchronize_irq() in xgmac_stop().

Both masks include DMA_INTR_ENA_TSE. xgmac_mac_disable() then clears
DMA_CONTROL_ST, which stops the TX process and latches DMA_STATUS_TPS.
The IRQ is still registered, so:

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

This queues the work again after the cancel, just before
xgmac_free_dma_desc_rings() frees the rings.

The late worker calls napi_disable() on the NAPI that xgmac_stop() has
just disabled. napi_disable_locked() then waits forever for
NAPIF_STATE_SCHED and NAPIF_STATE_NPSVC to clear, and it holds
netdev_lock the whole time. After that, xgmac_open()->napi_enable()
blocks under RTNL.

If the interface stays down and the device is removed, the pending work
can also outlive priv. See the comment on cancel_work_sync() below.

Would this need the interrupts masked, synchronize_irq(), NAPI disabled
and the mask written again before the work is cancelled or disabled with
disable_work_sync()?

> +	cancel_work_sync(&priv->tx_timeout_work);

[Severity: High]
Can the watchdog queue tx_timeout_work again after this cancel?
xgmac_change_mtu() calls stop and open directly on a running device:

xgmac_change_mtu() {
    ...
	/* Bring interface down, change mtu and bring interface back up */
	xgmac_stop(dev);
	WRITE_ONCE(dev->mtu, new_mtu);
	return xgmac_open(dev);
}

This path skips dev_deactivate_many(). The dev_watchdog() timer stays
armed, netif_running() stays true, and the carrier stays on.
netif_tx_disable() stops the queue but does not refresh trans_start.

If TX has been idle for longer than watchdog_timeo (5*HZ here, because the
driver doesn't set it), this can run between cancel_work_sync() and the
end of xgmac_open():

dev_watchdog()->xgmac_tx_timeout()->schedule_work(&priv->tx_timeout_work)

The late worker either hangs in napi_disable() on the NAPI that
xgmac_stop() disabled, or races with the napi_disable() in xgmac_stop().
Both cases end in the same netdev_lock/RTNL deadlock.

Separately, xgmac_remove() never cancels or disables tx_timeout_work:

xgmac_remove() {
    ...
	unregister_netdev(ndev);
	netif_napi_del(&priv->napi);
    ...
	free_netdev(ndev);
}

netif_close_many() skips ndo_stop for a device that is already down. Work
queued after the last xgmac_stop(), from the watchdog or from the TPS
path above, can therefore still be pending when free_netdev() frees priv.
priv embeds the work_struct.

Should xgmac_remove() also do a final cancel_work_sync() or
disable_work_sync()?

> +	napi_disable(&priv->napi);

[Severity: High]
netif_tx_disable() now runs before napi_disable(). Can a poll that runs
in between wake the queue again? That poll goes through:

xgmac_poll()->xgmac_tx_complete() {
    ...
	if (unlikely(netif_queue_stopped(priv->dev) &&
	    (tx_dma_ring_space(priv) > MAX_SKB_FRAGS)))
		netif_wake_queue(priv->dev);
}

xgmac_tx_timeout_work(), which cancel_work_sync() waits for, also calls
netif_wake_queue(). Nothing later in xgmac_stop() stops the queue again.

When xgmac_stop() is called from xgmac_change_mtu(), the device is still
running and the qdisc is still active. After the wake, xgmac_xmit() can
run on another CPU while xgmac_free_dma_desc_rings() frees priv->dma_tx
and priv->tx_skbuff, or after it has set them to NULL.

Would this lead to a use-after-free or a NULL dereference at
priv->tx_skbuff[entry] = skb in xgmac_xmit()? It would also write
XGMAC_DMA_TX_POLL to start DMA on freed descriptors.

With the old order, napi_disable() ran before netif_tx_disable(), so a
poll could not wake the queue here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928134115.1957-1-getshell%40seu.edu.cn