Re: [PATCH net v3 06/10] net: stmmac: keep DMA configurations at stable addresses

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 3 (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] Baseline decision: not confirmed as a remaining issue.
- [Medium] In drivers/net/ethernet/stmicro/stmmac/stmmac_main.c, the EEE
  software LPI timer can access a freed DMA configuration.
- [Medium] In drivers/net/ethernet/stmicro/stmmac/stmmac_main.c,
  stmmac_features_check() (ndo_features_check) calls…
- [Medium] Baseline decision: not confirmed as a remaining issue.

Pre-existing issues:
- [Medium] Baseline decision: not confirmed as a remaining issue.

--

net: stmmac: keep DMA configurations at stable addresses

This commit turns priv->dma_conf from an embedded struct into a pointer
to the allocated DMA configuration. The goal is to keep RXQ metadata
registered with XDP and AF_XDP at a stable address. Per-queue MSI
handlers now use priv->channel[] as their cookie. stmmac_quiesce()
drains transmitters and NAPI with synchronize_net() before cancelling
the TX hrtimers. On success the open and MTU paths free the old
configuration, and on failure they restore it.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3b918cf898067..ce598b5757637 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -463,7 +463,7 @@ static bool stmmac_eee_tx_busy(struct stmmac_priv *priv)
>  
>  	/* check if all TX queues have the work finished */
>  	for (queue = 0; queue < tx_cnt; queue++) {
> -		struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
> +		struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
>  
>  		if (tx_q->dirty_tx != tx_q->cur_tx)
>  			return true; /* still unfinished work */

[Severity: Medium]
Now that dma_conf is a separate allocation that gets freed on reopen,
can the EEE software LPI timer read a freed configuration here? The
timer path is:

stmmac_eee_ctrl_timer()
  stmmac_try_to_start_sw_lpi()
    stmmac_eee_tx_busy()
      tx_q = &priv->dma_conf->tx_queue[queue];

No lock or RCU protects the priv->dma_conf load.

__stmmac_release() calls phylink_stop() first, and that reaches
stmmac_mac_disable_tx_lpi():

    priv->eee_sw_timer_en = false;
    timer_delete_sync(&priv->eee_ctrl_timer);

NAPI is still running at that point. stmmac_tx_clean() checks the flag
without a lock before it re-arms the timer:

    if (priv->eee_sw_timer_en && !priv->tx_path_in_lpi_mode)
        stmmac_restart_sw_lpi_timer(priv);

Suppose that check passes just before the flag is cleared. Then
mod_timer() can run after timer_delete_sync() has returned. Nothing in
stmmac_quiesce() cancels eee_ctrl_timer again.

Next, suppose the callback loads the old priv->dma_conf before
__stmmac_open() stores the new one, and is then delayed (for example,
preempted in the timer softirq thread on PREEMPT_RT). It would then
dereference tx_queue[] after kfree(old_conf) in stmmac_change_mtu() or
stmmac_open(). Before this patch the configuration was embedded in
priv, so the worst case was a stale read.

Later in the series, stmmac_reconfigure_mtu() adds
timer_delete_sync(&priv->eee_ctrl_timer) after stmmac_quiesce(). The
ethtool ring and channel paths still seem to lack it:

stmmac_reinit_ringparam() / stmmac_reinit_queues()
  __stmmac_release()
  stmmac_reopen()
    __stmmac_open()
    kfree(old_conf)

The close/open path also seems to lack it. Should the EEE timer also be
cancelled on those paths after NAPI is quiesced?

[ ... ]

> @@ -3650,7 +3650,7 @@ static bool stmmac_tso_channel_permitted(struct stmmac_priv *priv,
>  					 unsigned int chan)
>  {
>  	/* TSO and TBS cannot co-exist */
> -	return !(priv->dma_conf.tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
> +	return !(priv->dma_conf->tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
>  }

[Severity: Medium]
stmmac_features_check(), the ndo_features_check callback, calls this.
Can it read priv->dma_conf after the old configuration has been freed?

netif_skb_features() is called from paths that neither
netif_device_detach() nor stopped TX queues exclude:

- tbf_segment() in net/sched/sch_tbf.c, and similarly taprio, cake,
  netem and dualpi2
- ip_finish_output_gso() and its IPv6 counterpart
- validate_xmit_skb() in sch_direct_xmit(), before the stopped-queue
  check

In stmmac_change_mtu(), the only synchronize_net() is the one inside
__stmmac_release(), and it runs before the pointer swap:

stmmac_change_mtu()
  netif_device_detach(dev)
  __stmmac_release(dev)
    stmmac_quiesce()
      synchronize_net()
  __stmmac_open(dev, dma_conf)
    priv->dma_conf = dma_conf;
  kfree(old_conf)

A GSO feature check could load the old pointer after that drain and be
preempted until after kfree(old_conf). It would then read tbs from
freed memory.

The same ordering seems to remain at the end of the series. In
stmmac_reconfigure_mtu(), synchronize_net() runs before
priv->dma_conf = new_conf, followed by kfree(old_conf). stmmac_reopen()
does the same for ring and channel changes.

Is a grace period needed between publishing the new configuration and
freeing the old one?

[ ... ]

> @@ -6267,10 +6274,12 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  			return PTR_ERR(dma_conf);
>  		}
>  
> +		netif_device_detach(dev);
>  		__stmmac_release(dev);
>  
>  		ret = __stmmac_open(dev, dma_conf);
>  		if (ret) {
> +			priv->dma_conf = old_conf;
>  			free_dma_desc_resources(priv, dma_conf);
>  			kfree(dma_conf);
>  			/*
> @@ -6282,9 +6291,10 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  			return ret;
>  		}
>  
> -		kfree(dma_conf);
> +		kfree(old_conf);

[Severity: High]
The commit message says:

  Drain transmitters and NAPI poll tails before cancelling TX timers so
  a late rearm cannot outlive the configuration containing the timer.

Can a TX hard-error interrupt restart transmitters after that drain?
__stmmac_release() runs stmmac_quiesce() and frees the IRQs only
afterwards:

    stmmac_quiesce(priv);
    ...
    stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
    stmmac_stop_all_dma(priv);
    free_dma_desc_resources(priv, priv->dma_conf);

In that window, stmmac_msi_intr_tx() calls stmmac_tx_err(), which ends
with:

    netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, chan));

Once a queue is woken, stack transmitters can enter stmmac_xmit() or
stmmac_tso_xmit() again. They would be using the old configuration
while free_dma_desc_resources() frees its rings, and while this
kfree(old_conf) frees the struct itself.

Two later commits in the series deal with this:

- "net: stmmac: account for failed live XDP reopening" adds a second
  stmmac_stop_tx_queues() after stmmac_free_irq() in
  __stmmac_release().
- "net: stmmac: retain DMA resources across MTU changes" disables IRQ
  handling in stmmac_reconfigure_mtu() before replacing the
  configuration.

At this patch, though, can the old configuration still be in use after
kfree(old_conf)?

[ ... ]

> @@ -7251,7 +7253,7 @@ void stmmac_xdp_release(struct net_device *dev)
>  	stmmac_disable_all_queues(priv);
>  
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> -		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> +		hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
>  
>  	/* Free the IRQ lines */
>  	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);

[Severity: Medium]
This isn't a bug introduced by this patch, but stmmac_xdp_release()
still cancels the TX hrtimers right after stmmac_disable_all_queues().
It does not run the synchronize_net() that stmmac_quiesce() now uses.
Can a NAPI poll tail re-arm a timer after this cancel?

After napi_complete_done() clears SCHED, stmmac_tx_timer_arm() can see
napi_is_scheduled() == false and call hrtimer_start(). stmmac_xdp_open()
then calls hrtimer_setup() on the same txtimer, which could
re-initialize an armed hrtimer.

Later in the series, "net: stmmac: account for failed live XDP
reopening" addresses this by making stmmac_xdp_release() call
stmmac_quiesce().

[ ... ]

> @@ -7382,11 +7382,9 @@ int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags)
>  	    queue >= priv->plat->tx_queues_to_use)
>  		return -EINVAL;
>  
> -	rx_q = &priv->dma_conf.rx_queue[queue];
> -	tx_q = &priv->dma_conf.tx_queue[queue];
>  	ch = &priv->channel[queue];
>  
> -	if (!rx_q->xsk_pool && !tx_q->xsk_pool)
> +	if (!test_bit(queue, priv->af_xdp_zc_qps))
>  		return -EINVAL;

[Severity: Medium]
Is the af_xdp_zc_qps bit a strong enough guard here? In
stmmac_xdp_enable_pool(), the sequence is:

    set_bit(queue, priv->af_xdp_zc_qps);
    ...
    stmmac_enable_rx_queue(priv, queue);
    stmmac_enable_tx_queue(priv, queue);
    napi_enable(&ch->rxtx_napi);
    err = stmmac_xsk_wakeup(priv->dev, queue, XDP_WAKEUP_RX);

Both enable helpers return void. If they fail, dma_rx, buf_pool and
xsk_pool are left NULL.

If both queue re-enables fail, the old check returned -EINVAL. The new
check lets NAPI run stmmac_rx_zc() on a NULL ring and pool.

This seems to go away later in the series with "net: stmmac: use the
tracked datapath restart for XSK pool changes". That commit switches to
stmmac_xdp_release()/stmmac_xdp_open() and clears the bit when the
reopen fails.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com