Re: [PATCH v2] net: stmmac: clear dma_conf in __stmmac_open error path

Re: [PATCH v2] net: stmmac: clear dma_conf in __stmmac_open error path

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 2 · Low: 0

- [High] The new `memset(&priv->dma_conf, 0, sizeof(priv->dma_conf))` at
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:4193 also zeroes the…
- [High] On the `irq_error` path the MAC and all DMA channels are already
  running, but the unwind never stops them, so the descriptor rings the…
- [Medium] The new memset() at stmmac_main.c:4193 also wipes the per-TX-
  queue TBS state in `priv->dma_conf.tx_queue[].tbs` (STMMAC_TBS_EN /…
- [Medium] The whole-struct clear at stmmac_main.c:4193 also erases
  persistent user configuration held in `priv->dma_conf.dma_rx_size` /…

Pre-existing issues:
- [High] After a failed reopen the driver leaves every NAPI instance
  disabled while the netdevice stays logically running, and the next…

--

net: stmmac: clear dma_conf in __stmmac_open error path

This adds a memset of priv->dma_conf on the error paths of
__stmmac_open(), so that the struct no longer points at the descriptor
resources the callers free after a failed open.  A few questions about
what else that whole-struct clear erases, and about the state the failed
open leaves behind.

Link to v1: https://lore.kernel.org/r/20260903-fix-stmmac-mtu-change-use-after-free-v1-1-c81dc7d6d18a@uniontech.com

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24656b35350b1..4369e64faf9f8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4190,6 +4190,7 @@ static int __stmmac_open(struct net_device *dev,
>  
>  	stmmac_release_ptp(priv);
>  init_error:
> +	memset(&priv->dma_conf, 0, sizeof(priv->dma_conf));
       ^^^^^^

[Severity: High]
Can this memset turn the per-queue txtimer hrtimers into invalid ones?

The struct hrtimer txtimer lives inside priv->dma_conf.tx_queue[], and on
the irq_error path stmmac_init_coalesce() has already initialized them:

  struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
  ...
  hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC,
                HRTIMER_MODE_REL);

After the memset those timers are all-zero, so timer->base is NULL.  Every
teardown path walks them unconditionally, in __stmmac_release(),
stmmac_suspend() and stmmac_xdp_release():

  for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
          hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);

and hrtimer_cancel() -> hrtimer_try_to_cancel() -> hrtimer_active() has no
NULL check:

  base = READ_ONCE(timer->base);
  seq = raw_read_seqcount_begin(&base->seq);

A failed reopen in stmmac_change_mtu() just returns the error, so the
interface stays IFF_UP and the next ip link set down, suspend or XDP
attach reaches those zeroed timers.

The commit message says the memset leaves "the state consistent regardless
of the caller" - but priv->dma_conf is the live object all of those paths
dereference.  Would it be better to invalidate only the freed resource
pointers, or to leave the timers alone, rather than clearing the whole
struct?

[Severity: Medium]
Does this also drop the per-queue TBS state?

tc_setup_etf() in stmmac_tc.c keeps the ETF/launch-time offload state
only in priv->dma_conf:

  if (!(priv->dma_conf.tx_queue[qopt->queue].tbs & STMMAC_TBS_AVAIL))
          return -EINVAL;

  if (qopt->enable)
          priv->dma_conf.tx_queue[qopt->queue].tbs |= STMMAC_TBS_EN;

and the loop at the top of __stmmac_open() exists to carry that bit across
a reopen:

  for (int i = 0; i < priv->plat->tx_queues_to_use; i++)
          if (priv->dma_conf.tx_queue[i].tbs & STMMAC_TBS_EN)
                  dma_conf->tx_queue[i].tbs = priv->dma_conf.tx_queue[i].tbs;

After the memset both STMMAC_TBS_EN and STMMAC_TBS_AVAIL are gone while
the etf qdisc is still installed, so a following tc ... etf call returns
-EINVAL.

On the next successful open stmmac_setup_dma_desc() re-derives only AVAIL
from plat->tx_queues_cfg[].tbs_en and stmmac_hw_setup() re-arms the
hardware:

  int enable = tx_q->tbs & STMMAC_TBS_AVAIL;

  stmmac_enable_tbs(priv, priv->ioaddr, enable, chan);

but stmmac_xmit() only programs the launch time when tx_q->tbs &
STMMAC_TBS_EN.  Does that mean launch times are silently ignored from
then on, with no error reported anywhere?

[Severity: Medium]
The same clear erases the ring geometry the user configured through
ethtool -G.  stmmac_reinit_ringparam() stores it directly in the struct
being zeroed:

  priv->dma_conf.dma_rx_size = rx_size;
  priv->dma_conf.dma_tx_size = tx_size;

  if (netif_running(dev))
          ret = stmmac_open(dev);

and stmmac_setup_dma_desc() reads it back, treating 0 as "never
configured":

  dma_conf->dma_tx_size = priv->dma_conf.dma_tx_size;
  dma_conf->dma_rx_size = priv->dma_conf.dma_rx_size;

  if (!dma_conf->dma_tx_size)
          dma_conf->dma_tx_size = DMA_DEFAULT_TX_SIZE;
  if (!dma_conf->dma_rx_size)
          dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;

So after ethtool -G followed by a failed ip link set up,
stmmac_get_ringparam() reports rx_pending 0 / tx_pending 0 - a value
stmmac_set_ringparam() itself rejects as below DMA_MIN_RX_SIZE /
DMA_MIN_TX_SIZE - and the next successful open quietly allocates default
sized rings.  Is losing the configured sizes intended here?

There is a second consumer: stmmac_xdp_open() uses priv->dma_conf in
place without recomputing the sizes or dma_buf_sz:

  ret = alloc_dma_desc_resources(priv, &priv->dma_conf);
  ...
  ret = init_dma_desc_rings(dev, &priv->dma_conf, GFP_KERNEL);

Would a zeroed conf describe zero-length rings to the hardware there?

[Severity: High]
This isn't a bug introduced by this patch, but it affects the same
failure path the commit message says it makes consistent, so it seems
worth raising here: after a failed reopen, NAPI is left disabled while
netif_running() is still true, and the next teardown calls napi_disable()
on it again.

stmmac_change_mtu() calls __stmmac_release() (which runs
stmmac_disable_all_queues() -> __stmmac_disable_all_queues() ->
napi_disable() per queue) and then __stmmac_open().  NAPI is re-enabled
only on the success path:

  stmmac_enable_all_queues(priv);
  netif_tx_start_all_queues(priv->dev);

so both error labels return with NAPI disabled.  napi_disable_locked()
in net/core/dev.c leaves the state bits set and a second call waits on
them:

  while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
          usleep_range(20, 200);
          val = READ_ONCE(n->state);
  }

Nothing clears those bits because napi_enable() is never called, so does
the following ip link set down, suspend, stmmac_xdp_release(),
stmmac_reinit_ringparam() or stmmac_reset_subtask() hang there while
holding rtnl_lock?

If that is the case, the release-side unwind never reaches the
free_dma_desc_resources() the commit message describes, so does the
memset on its own actually make the post-failure state safe?

>  	return ret;
>  }
>  

[Severity: High]
One more question about the same error path: are the DMA engines still
running when the callers free the rings?

stmmac_hw_setup() programs every channel with the new dma_conf ring
addresses, enables MAC Rx/Tx, and ends with:

  /* Start the ball rolling... */
  stmmac_start_all_dma(priv);

If stmmac_request_irq() then fails, the irq_error path only does
phylink_stop(), the per-queue hrtimer_cancel() loop and
stmmac_release_ptp() - there is no stmmac_stop_all_dma().  Compare
__stmmac_release(), which stops the engines first:

  /* Stop TX/RX DMA and clear the descriptors */
  stmmac_stop_all_dma(priv);

  /* Release and free the Rx/Tx resources */
  free_dma_desc_resources(priv, &priv->dma_conf);

Both callers of __stmmac_open() free immediately after the failure, for
example in stmmac_change_mtu():

  free_dma_desc_resources(priv, dma_conf);
  kfree(dma_conf);

Can the Rx DMA engine keep writing incoming frames into those unmapped
and freed descriptors and buffers?  After a failed MTU change the
interface stays IFF_UP, so nothing else stops the engines.  Should the
error path stop the DMA and disable the MAC instead of (or in addition
to) clearing priv->dma_conf?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-fix-stmmac-mtu-change-use-after-free-v2-1-91e680476921%40uniontech.com

[PATCH v2] net: stmmac: clear dma_conf in __stmmac_open error path

[ZhaoJinming]

__stmmac_open() copies the freshly allocated dma_conf into priv->dma_conf
via memcpy() before it can fail, e.g. in stmmac_request_irq().  When it
does fail, the callers free the dma_conf descriptor resources and the
struct itself, but priv->dma_conf still holds pointers to those freed
resources, a use-after-free that triggers when a subsequent
stmmac_release() or another MTU change releases the resources again.

Undo the memcpy in the error path by zeroing priv->dma_conf, leaving the
state consistent regardless of the caller.

Fixes: 30134b7c47bd2 ("net: ethernet: stmicro: stmmac: fix possible memory leak in __stmmac_open")
Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
Changes in v2:
- Move the clearing of priv->dma_conf from stmmac_change_mtu() into
  __stmmac_open() error path, undoing the memcpy() at the point where it
  was made and covering both callers.
- Link to v1: https://lore.kernel.org/r/20260903-fix-stmmac-mtu-change-use-after-free-v1-1-c81dc7d6d18a@uniontech.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b14454fb10deced6516eb89e2c0c9..4369e64faf9f878aa20ac5075705044f55c5f8bf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4190,6 +4190,7 @@ static int __stmmac_open(struct net_device *dev,
 
 	stmmac_release_ptp(priv);
 init_error:
+	memset(&priv->dma_conf, 0, sizeof(priv->dma_conf));
 	return ret;
 }
 

---
base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3
change-id: 20260903-fix-stmmac-mtu-change-use-after-free-693da6eb4a30

Best regards,
-- 
ZhaoJinming <zhaojinming@uniontech.com>

Re: [PATCH v2] net: stmmac: clear dma_conf in __stmmac_open error path

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

> __stmmac_open() copies the freshly allocated dma_conf into priv->dma_conf
> via memcpy() before it can fail, e.g. in stmmac_request_irq().  When it
> does fail, the callers free the dma_conf descriptor resources and the
> struct itself, but priv->dma_conf still holds pointers to those freed
> resources, a use-after-free that triggers when a subsequent
> stmmac_release() or another MTU change releases the resources again.
> 
> Undo the memcpy in the error path by zeroing priv->dma_conf, leaving the
> state consistent regardless of the caller.
> 
> Fixes: 30134b7c47bd2 ("net: ethernet: stmicro: stmmac: fix possible memory leak in __stmmac_open")
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>

Hi ZhaoJinming,

I agree the issue is real, but assuming free_dma_desc_resources() always
tolerates a fully zeroed dma_conf struct seems a bit fragile to me.
Is it better to use a pointer for priv->dma_conf that we can set to NULL in
case of error? What do you think?

Regards,
Lorenzo

> ---
> Changes in v2:
> - Move the clearing of priv->dma_conf from stmmac_change_mtu() into
>   __stmmac_open() error path, undoing the memcpy() at the point where it
>   was made and covering both callers.
> - Link to v1: https://lore.kernel.org/r/20260903-fix-stmmac-mtu-change-use-after-free-v1-1-c81dc7d6d18a@uniontech.com
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24656b35350b14454fb10deced6516eb89e2c0c9..4369e64faf9f878aa20ac5075705044f55c5f8bf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4190,6 +4190,7 @@ static int __stmmac_open(struct net_device *dev,
>  
>  	stmmac_release_ptp(priv);
>  init_error:
> +	memset(&priv->dma_conf, 0, sizeof(priv->dma_conf));
>  	return ret;
>  }
>  
> 
> ---
> base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3
> change-id: 20260903-fix-stmmac-mtu-change-use-after-free-693da6eb4a30
> 
> Best regards,
> -- 
> ZhaoJinming <zhaojinming@uniontech.com>
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] net: stmmac: clear dma_conf in __stmmac_open error path

[赵金明]

Thanks for the review.

I agree that using NULL as an explicit "no configuration" sentinel is cleaner than relying on an all-zero struct. However, the zeroed struct is currently safe to tear down, and that safety comes from established kernel conventions rather than from anything stmmac-specific:
 - kfree(NULL) is a no-op by definition.
 - dma_free_coherent() bails out early on a NULL cpu_addr (see kernel/dma/mapping.c, dma_free_attrs() -> "if (!cpu_addr) return;").
 - page_pool_destroy() is guarded by "if (rx_q->page_pool)".
 - xdp_rxq_info_unreg() is guarded by xdp_rxq_info_is_reg().
 - The skb/descriptor loops are bounded by dma_tx_size/dma_rx_size, which are zero after the memset, so they never dereference the (now NULL) buffer/descriptor pointers.
So while memset() does lean on those guards, they are not accidental and hold for the current code.

Converting priv->dma_conf to a pointer is a much larger change than it first appears: it is referenced ~120 times across six files (stmmac_main.c, stmmac_selftests.c, stmmac_tc.c, stmmac_ethtool.c, chain_mode.c, ring_mode.c), and would require changing the allocation lifetime in probe/remove plus NULL guards at every teardown site. That feels like net-next cleanup material rather than something to fold into a use-after-free fix that should be backported to stable.

Would you be OK with keeping the memset() here as the minimal fix, and I follow up with a separate net-next series to convert priv->dma_conf to a pointer? If you'd still prefer the pointer approach in this patch, I can do it, but I wanted to flag the scope first.

Regards, 
ZhaoJinming



>> __stmmac_open() copies the freshly allocated dma_conf into priv->dma_conf



>> via memcpy() before it can fail, e.g. in stmmac_request_irq().? When it



>> does fail, the callers free the dma_conf descriptor resources and the



>> struct itself, but priv->dma_conf still holds pointers to those freed



>> resources, a use-after-free that triggers when a subsequent



>> stmmac_release() or another MTU change releases the resources again.



>> 



>> Undo the memcpy in the error path by zeroing priv->dma_conf, leaving the



>> state consistent regardless of the caller.



>> 



>> Fixes: 30134b7c47bd2 ("net: ethernet: stmicro: stmmac: fix possible memory leak in __stmmac_open")



>> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>



>



>Hi ZhaoJinming,



>



>I agree the issue is real, but assuming free_dma_desc_resources() always



>tolerates a fully zeroed dma_conf struct seems a bit fragile to me.



>Is it better to use a pointer for priv->dma_conf that we can set to NULL in



>case of error? What do you think?



>



>Regards,



>Lorenzo



>



>> ---



>> Changes in v2:



>> - Move the clearing of priv->dma_conf from stmmac_change_mtu() into



>>?? __stmmac_open() error path, undoing the memcpy() at the point where it



>>?? was made and covering both callers.



>> - Link to v1: https://lore.kernel.org/r/20260903-fix-stmmac-mtu-change-use-after-free-v1-1-c81dc7d6d18a@uniontech.com



>> ---



>>? drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +



>>? 1 file changed, 1 insertion(+)



>> 



>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c



>> index 24656b35350b14454fb10deced6516eb89e2c0c9..4369e64faf9f878aa20ac5075705044f55c5f8bf 100644



>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c



>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c



>> @@ -4190,6 +4190,7 @@ static int __stmmac_open(struct net_device *dev,



>>? 



>>? 	stmmac_release_ptp(priv);



>>? init_error:



>> +	memset(&priv->dma_conf, 0, sizeof(priv->dma_conf));



>>? 	return ret;



>>? }



>>? 



>> 



>> ---



>> base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3



>> change-id: 20260903-fix-stmmac-mtu-change-use-after-free-693da6eb4a30



>> 



>> Best regards,



>> -- 



>> ZhaoJinming <zhaojinming@uniontech.com>



>> 



>>