Re: [PATCH net v3 10/10] net: stmmac: retain DMA resources across MTU changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: stmmac: Use-After-Free of TX hrtimers during MTU reconfiguration
- [High] net: stmmac: Shared interrupt line disruption via disable_irq()
--

commit 3f4df67ed0eaa758c59a612b2b520f3af55b5627
Author: James Hilliard <james.hilliard1@gmail.com>

net: stmmac: retain DMA resources across MTU changes

Releasing the current datapath before reopening with a new MTU loses the
working configuration when hardware setup fails. Keep the old rings and
IRQ registrations until replacement hardware setup succeeds, so an
ordinary setup error can restore the old MTU without allocating new rings
or requesting interrupts again.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d17c68f55caf6..1a552455a633d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4080,14 +4156,40 @@ static int stmmac_request_irq(struct net_device *dev)
>  	return ret;
>  }
>  
> +/* Balance disable_irq()/enable_irq() for every registered IRQ, including
> + * shared lines. Unlike freeing and requesting IRQs, this cannot fail.
> + */
> +static void stmmac_set_irq_state(struct stmmac_priv *priv, bool enable)
> +{
> +	void (*set_state)(unsigned int) = enable ? enable_irq : disable_irq;
> +	struct stmmac_msi *msi = priv->msi;
> +	int irq = priv->dev->irq;
> +	u32 i;
> +
> +	set_state(irq);

[Severity: High]
Since dev->irq, priv->wol_irq, and priv->sfty_irq can be registered as
shared interrupt lines (IRQF_SHARED) in single IRQ mode, does using
disable_irq() here risk completely disrupting other unrelated devices
sharing the same interrupt line?

Could this suppress the interrupt at the controller level and lead to
system-wide instability or timeouts for those other devices?

[ ... ]

> @@ -6234,6 +6338,113 @@ static void stmmac_set_rx_mode(struct net_device *dev)
>  	stmmac_set_filter(priv, priv->hw, dev);
>  }
>  
> +static int stmmac_reconfigure_mtu(struct net_device *dev, int mtu)
> +{

[ ... ]

> +	/* An IRQ can recover a TX error and restart a queue. Drain handlers
> +	 * before the final TX stop, and keep the registrations for rollback.
> +	 */
> +	stmmac_set_irq_state(priv, false);
> +	netif_tx_disable(dev);
> +	synchronize_net();
> +
> +	ret = stmmac_prepare_rx_buffers(priv);

[Severity: High]
If a TX error IRQ fires between the initial stmmac_quiesce() and the
stmmac_set_irq_state(priv, false) call here in stmmac_reconfigure_mtu(),
the queue can be restarted and stmmac_tx_timer_arm() will re-arm the TX
hrtimer.

However, this block appears to omit a final stmmac_stop_tx_queues(priv)
or hrtimer_cancel() call to disarm the TX hrtimers before freeing the
old configuration.

Does this mean the timer could remain queued when old_conf is freed,
potentially causing a use-after-free and a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282@gmail.com?part=10