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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 0 (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 patch closes the remove-time IRQ use-after-free for only two
  objects: the netdev and priv->hw.
- [High] The device is never quiesced before the remove window the commit
  claims to close.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  sxgbe_release() calls timer_delete_sync(&priv->eee_ctrl_timer) (line…
- [Medium] This problem wasn't introduced by this patch, but
  init_tx_ring() allocates tx_skbuff_dma and tx_skbuff with…

--

net: sxgbe: manage IRQ data lifetimes with devres

This commit switches the netdev and priv->hw allocations in
sxgbe_drv_probe() to devres, so they are freed after the managed IRQs.
It removes the matching kfree()/free_netdev() calls from the probe error
path and from sxgbe_drv_remove().

> 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);

[Severity: High]
This isn't a bug introduced by this patch, but eee_ctrl_timer lives in the
netdev private area and can still be pending when that memory is freed.
After this patch the free happens in devm_free_netdev() instead of
free_netdev().

sxgbe_release() deletes the timer before it disables NAPI:

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

The RX/TX interrupts are still enabled between those two calls, so
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));
    }

The phy_stop()/phy_disconnect() calls in between make the window wider.

sxgbe_eee_ctrl_timer() re-arms itself every time it runs. Nothing in
sxgbe_drv_remove() or devres deletes the timer again.

Can the timer fire on freed memory after unbind? Until unbind, it also keeps
calling sxgbe_enable_eee_mode(), which touches MMIO after
clk_disable_unprepare() in sxgbe_release().

>  	if (!ndev)
>  		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);
>  }

[Severity: High]
Does this close the window described in the commit message ("an interrupt
in that window can access freed memory")? The netdev and priv->hw are now
freed after the devres IRQ release. However, the IRQs are still only
released at unbind.

sxgbe_open() requests every line with devres:

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

Nothing in sxgbe_release() calls free_irq(), devm_free_irq() or
synchronize_irq(). So the handlers are still live when sxgbe_drv_remove()
tears down the rings:

sxgbe_drv_remove()
  unregister_netdev()
    sxgbe_release()
      priv->hw->dma->stop_tx()
      free_dma_desc_resources()

stop_tx() raises TPS because TSE is part of SXGBE_DMA_ENA_INT.
sxgbe_tx_dma_int_status() reports TPS as tx_hard_error, so
sxgbe_tx_interrupt() calls sxgbe_restart_tx_queue():

    /* free the skbuffs of the ring */
    tx_free_ring_skbufs(tx_ring);
    ...
    priv->hw->dma->start_tx_queue(priv->ioaddr, queue_num);

No lock separates tx_free_ring_skbufs() from
free_dma_desc_resources()->dma_free_tx_skbufs(). Can both of them call
dev_kfree_skb_any() on the same tx_skbuff[i]?

Now suppose the interrupt lands right after stop_tx(). Can start_tx_queue()
re-enable a channel whose descriptor ring free_dma_desc_resources() frees
next? The device would then DMA to freed coherent memory.

The devm IRQs also survive ifdown. A second ifup runs init_tx_ring() again
and allocates new devm tx_skbuff and tx_skbuff_dma arrays. These are newer
than the TX IRQ devres entries from the first open, so devres frees them
before those IRQs at unbind.

On that second open, devm_request_irq() on the non-shared TX/RX lines would
fail with -EBUSY. Each attempt also adds another shared common/LPI handler.

Would request_irq() in sxgbe_open() and free_irq() in sxgbe_release(),
before the rings are freed, be a more direct fix? It would also cover the
netdev and priv->hw window this patch targets, because unregister_netdev()
would free the IRQs before sxgbe_drv_remove() frees anything.

[Severity: High]
Related to the above, the device interrupt sources are never masked during
teardown. sxgbe_dma_channel_init() arms every channel:

    writel(SXGBE_DMA_ENA_INT,
           ioaddr + SXGBE_DMA_CHA_INT_ENABLE_REG(cha_num));

Neither sxgbe_release() nor sxgbe_drv_remove() clears these enables or the
MAC core interrupt sources. Only the RX ISR and NAPI toggle them, through
disable_dma_irq/enable_dma_irq.

So the stop_tx() calls in sxgbe_drv_remove() and sxgbe_release() trigger
the TPS interrupt themselves. sxgbe_restart_tx_queue() then sets
SXGBE_TX_ENABLE again, while the channel's descriptor base and tail
registers still point at the ring that free_dma_desc_resources() frees.

Could the device then fetch descriptors from, and write status back to,
freed coherent memory?

After sxgbe_release() calls clk_disable_unprepare(priv->sxgbe_clk), the
handlers are still registered and read status registers on the gated
block. sxgbe_common_interrupt() is on IRQF_SHARED lines, so another device
on the same line can invoke it.

This patch keeps netdev, hw and ioaddr alive, so these accesses become real
MMIO instead of freed-pointer dereferences. Is anything stopping them?

Should ndo_stop mask the per-channel and MAC interrupt enables, and free or
synchronize the IRQs, before it stops DMA, gates the clock and frees the
rings?

[Severity: Medium]
This is a pre-existing issue, but init_tx_ring() allocates tx_skbuff_dma
and 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 frees the coherent descriptors. Do these arrays pile up
on each ifup until unbind?

The devm IRQs from the first open are never freed, so each later ifup fails
with -EBUSY at the TX IRQ request. Each attempt still leaks a new set of
arrays.

Separately, the irq_of_parse_and_map() mappings from sxgbe_platform_probe()
are only disposed on the probe error path. sxgbe_platform_remove() only
calls sxgbe_drv_remove(ndev). Should it also call irq_dispose_mapping() for
the common, TX, RX and LPI mappings?

>  
>  #ifdef CONFIG_PM

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