[net-next,v3,2/2] net: stmmac: Avoid freeing and re-requesting IRQ during XDP set prog
Commit Message
Currently stmmac will run a full cycle of IRQ tear down and setup when
setting up a new XDP program. This makes tuning TSN systems difficult
because whenever a new XDP program is installed then the irq threads will
be stopped and restarted which will undo any thread tuning.
The problem is avoided by removing stmmac_free_irq()/stmmac_request_irq()
from stmmac_xdp_release()/stmmac_xdp_open().
stmmac_free_irq() implicitly synchronizes interrupts and, with that
removed, nothing else disables interrupts or makes later actions by
stmmac_xdp_release() safe when there are concurrent interrupts. Thus we
must replace the code to free the IRQ with code to disable (and sync) DMA
interrupts. We also need to avoid re-initializing the DMA channel. Happily
the reinit is redundant, it is sufficient to rely on the re-enabling
of interrupts at the end of stmmac_xdp_open().
Signed-off-by: Daniel Thompson <daniel@riscstar.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 65 +++++++++++++++++------
1 file changed, 48 insertions(+), 17 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: stmmac: Use-After-Free in stmmac_tx_err() due to active ISR after DMA descriptors freed
--
commit 4c2320440039ed3235615c60f92cbcfdc0584d5a
Author: Daniel Thompson <daniel@riscstar.com>
net: stmmac: Avoid freeing and re-requesting IRQ during XDP set prog
This commit changes the XDP setup process to avoid completely freeing and
re-requesting IRQs, which disrupts TSN thread tuning. Instead of calling
stmmac_free_irq(), it masks the DMA interrupts and synchronizes them during
XDP release.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 7b80fe66cf199..f1359637007e2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -7141,8 +7186,9 @@ void stmmac_xdp_release(struct net_device *dev)
> for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
>
> - /* Free the IRQ lines */
> - stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
> + /* Silence DMA interrupts */
> + stmmac_disable_all_dma_irq(priv);
> + stmmac_synchronize_irq(dev);
>
> /* Stop TX/RX DMA channels */
> stmmac_stop_all_dma(priv);
[Severity: High]
Could this change introduce a use-after-free when setting up an XDP program?
Since the ISR stmmac_interrupt() remains registered after replacing
stmmac_free_irq(), the interrupt handler can still run if a shared IRQ or
unmasked MAC interrupt fires.
When stmmac_stop_all_dma() stops the TX DMA, it sets the TPS (Tx Process
Stopped) bit in the hardware DMA status register.
Immediately after this in stmmac_xdp_release(), free_dma_desc_resources()
is called, which frees the descriptor memory without assigning NULL.
If an interrupt fires at this point, stmmac_interrupt() reads the raw DMA
status and evaluates TPS regardless of whether DMA interrupts are masked:
drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c:dwmac4_dma_interrupt() {
...
if (unlikely(intr_status & DMA_CHAN_STATUS_TPS)) {
x->tx_process_stopped_irq++;
ret = tx_hard_error;
}
...
}
This unconditionally returns tx_hard_error, which then triggers
stmmac_tx_err() to execute.
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_tx_err() {
...
dma_free_tx_skbufs(priv, &priv->dma_conf, chan);
stmmac_clear_tx_descriptors(priv, &priv->dma_conf, chan);
...
}
Will stmmac_tx_err() then try to clear and re-free the already freed
descriptors, resulting in a use-after-free?
@@ -144,6 +144,7 @@ static const char *stmmac_dwxgmac_phyif[4] = {
[PHY_INTF_RGMII] = "RGMII",
};
+static void stmmac_synchronize_irq(struct net_device *dev);
static irqreturn_t stmmac_interrupt(int irq, void *dev_id);
/* For MSI interrupts handling */
static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id);
@@ -2520,6 +2521,23 @@ static void stmmac_enable_all_dma_irq(struct stmmac_priv *priv)
}
}
+static void stmmac_disable_all_dma_irq(struct stmmac_priv *priv)
+{
+ u8 rx_channels_count = priv->plat->rx_queues_to_use;
+ u8 tx_channels_count = priv->plat->tx_queues_to_use;
+ u8 dma_csr_ch = max(rx_channels_count, tx_channels_count);
+ u8 chan;
+
+ for (chan = 0; chan < dma_csr_ch; chan++) {
+ struct stmmac_channel *ch = &priv->channel[chan];
+ unsigned long flags;
+
+ spin_lock_irqsave(&ch->lock, flags);
+ stmmac_disable_dma_irq(priv, priv->ioaddr, chan, STMMAC_DMA_IRQ_ALL);
+ spin_unlock_irqrestore(&ch->lock, flags);
+ }
+}
+
/**
* stmmac_start_all_dma - start all RX and TX DMA channels
* @priv: driver private structure
@@ -3816,6 +3834,33 @@ static void stmmac_free_irq(struct net_device *dev,
}
}
+static void stmmac_synchronize_irq(struct net_device *dev)
+{
+ struct stmmac_priv *priv = netdev_priv(dev);
+ struct stmmac_msi *msi = priv->msi;
+ int j;
+
+ for (j = priv->plat->tx_queues_to_use - 1; msi && j >= 0; j--) {
+ if (msi->tx_irq[j] > 0)
+ synchronize_irq(msi->tx_irq[j]);
+ }
+
+ for (j = priv->plat->rx_queues_to_use - 1; msi && j >= 0; j--) {
+ if (msi->rx_irq[j] > 0)
+ synchronize_irq(msi->rx_irq[j]);
+ }
+
+ if (msi && msi->sfty_ue_irq > 0 && msi->sfty_ue_irq != dev->irq)
+ synchronize_irq(msi->sfty_ue_irq);
+ if (msi && msi->sfty_ce_irq > 0 && msi->sfty_ce_irq != dev->irq)
+ synchronize_irq(msi->sfty_ce_irq);
+ if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
+ synchronize_irq(priv->wol_irq);
+ if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq)
+ synchronize_irq(priv->sfty_irq);
+ synchronize_irq(dev->irq);
+}
+
static int stmmac_msi_init(struct stmmac_priv *priv,
struct stmmac_resources *res)
{
@@ -7141,8 +7186,9 @@ void stmmac_xdp_release(struct net_device *dev)
for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
- /* Free the IRQ lines */
- stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
+ /* Silence DMA interrupts */
+ stmmac_disable_all_dma_irq(priv);
+ stmmac_synchronize_irq(dev);
/* Stop TX/RX DMA channels */
stmmac_stop_all_dma(priv);
@@ -7165,7 +7211,6 @@ int stmmac_xdp_open(struct net_device *dev)
struct stmmac_priv *priv = netdev_priv(dev);
u8 rx_cnt = priv->plat->rx_queues_to_use;
u8 tx_cnt = priv->plat->tx_queues_to_use;
- u8 dma_csr_ch = max(rx_cnt, tx_cnt);
struct stmmac_rx_queue *rx_q;
struct stmmac_tx_queue *tx_q;
bool sph_en;
@@ -7188,12 +7233,6 @@ int stmmac_xdp_open(struct net_device *dev)
stmmac_reset_queues_param(priv);
- /* DMA CSR Channel configuration */
- for (chan = 0; chan < dma_csr_ch; chan++) {
- stmmac_init_chan(priv, priv->ioaddr, priv->plat->dma_cfg, chan);
- stmmac_disable_dma_irq(priv, priv->ioaddr, chan, STMMAC_DMA_IRQ_TXRX);
- }
-
/* Adjust Split header */
sph_en = (priv->hw->rx_csum > 0) && priv->sph_active;
@@ -7230,10 +7269,6 @@ int stmmac_xdp_open(struct net_device *dev)
/* Start Rx & Tx DMA Channels */
stmmac_start_all_dma(priv);
- ret = stmmac_request_irq(dev);
- if (ret)
- goto irq_error;
-
/* Enable NAPI process*/
stmmac_enable_all_queues(priv);
netif_carrier_on(dev);
@@ -7242,10 +7277,6 @@ int stmmac_xdp_open(struct net_device *dev)
return 0;
-irq_error:
- for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
- hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
-
init_error:
free_dma_desc_resources(priv, &priv->dma_conf);
dma_desc_error: