[PATCH net-next v6 RESEND 0/4] net: stmmac: fix DMA descriptor ring cleanup issues

[PATCH net-next v6 RESEND 0/4] net: stmmac: fix DMA descriptor ring cleanup issues

[Ding Hui]

From: Ding Hui <dinghui@lixiang.com>

Hello,

This is a resend of v6, originally posted on October 24, 2026,
when net-next was closed. The four patches are unchanged.

Previous posting (v6):
https://lore.kernel.org/netdev/20261024071319.875921-1-dinghui1111@163.com/

This series fixes the NULL pointer dereferences in the stmmac TX/RX
DMA resource cleanup paths and the associated error path issues,
found on a DWMAC platform whose descriptor ring allocation may fail
under memory pressure.

Patch 1 adds NULL guards in the free helpers and makes them clear
the pointers they free, so that a NULL pointer reliably means "not
allocated" for the long-lived priv->dma_conf.

Patch 2 makes the per-queue allocation functions undo their own
allocations on error, instead of leaving the cleanup to the outer
error paths that would then walk partially-initialized queues.

Patch 3 releases the descriptor resources through priv->dma_conf
when __stmmac_open() fails after copying the temporary dma_conf, so
the long-lived configuration stays consistent with the resource
cleanup.

Patch 4 holds RTNL around the descriptors_status debugfs reader and
skips queues whose rings are not allocated.

The original v6 series was based on net-next commit
734bef548a96 (Merge tag 'net-next-7.4' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next).
This resend retains the same base.

Changes in v6:
  - Split the single patch into a four-patch series as requested
    during review: NULL guards and pointer clearing, allocation
    error paths, descriptor resource release through priv->dma_conf
    on open failure, and the descriptors_status debugfs locking.
  - Merge the descriptor resource release with the ptp_error unwind
    from 8181678a92f0 and rebase on top of the current net-next.
  - Use the "net-next" subject prefix, which was requested back in
    v1 but mistakenly kept as "net" in all previous revisions.
  - Link to v5:
    https://lore.kernel.org/netdev/20261012142458.2845385-1-dinghui1111@163.com/

Changes in v5:
  - Release the descriptor resources through priv->dma_conf when
    __stmmac_open() fails, avoiding stale pointers in the
    long-lived configuration.
  - Hold RTNL while descriptors_status reads the descriptor rings.
  - Link to v4:
    https://lore.kernel.org/netdev/20261001035045.3305816-1-dinghui1111@163.com/

Changes in v4:
  - Free the RX resources in alloc_dma_desc_resources() when the TX
    allocation fails.
  - Skip queues whose descriptor rings are not allocated in
    stmmac_rings_status_show() and print a hint instead.
  - Link to v3:
    https://lore.kernel.org/netdev/20260919121436.1642724-1-dinghui1111@163.com/

Changes in v3:
  - Use addr directly in __alloc_dma_rx_desc_resources() error path
    instead of the if/else block on rx_q->dma_erx/dma_rx.
  - Clear the freed pointers in __free_dma_rx_desc_resources() and
    __free_dma_tx_desc_resources().
  - Link to v2:
    https://lore.kernel.org/netdev/20260905154654.1725313-1-dinghui1111@163.com/

Changes in v2:
  - Instead of only adding NULL checks in the free helpers, also fix
    __alloc_dma_rx_desc_resources() and
    __alloc_dma_tx_desc_resources() to clean up their own
    allocations on error, as suggested by Andrew.
  - Link to v1:
    https://lore.kernel.org/netdev/20260830040610.1156008-1-dinghui1111@163.com/

Ding Hui (4):
  net: stmmac: guard against NULL pointers when freeing TX/RX buffers
  net: stmmac: clean up partially allocated DMA rings on error
  net: stmmac: release descriptor resources through priv->dma_conf on
    open failure
  net: stmmac: hold RTNL when reading descriptor rings in
    descriptors_status

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 115 +++++++++++++++---
 1 file changed, 98 insertions(+), 17 deletions(-)

--
2.34.1

[PATCH net-next v6 RESEND 1/4] net: stmmac: guard against NULL pointers when freeing TX/RX buffers

[Ding Hui]

From: Ding Hui <dinghui@lixiang.com>

The DMA resource free helpers may be invoked on queues whose buffers
were never allocated:

  - dma_free_rx_skbufs() and dma_free_rx_xskbufs() dereference
    rx_q->buf_pool[i] via stmmac_free_rx_buffer(), but buf_pool may
    be NULL if its kzalloc_objs() failed.

  - dma_free_tx_skbufs() dereferences tx_q->tx_skbuff_dma[i] via
    stmmac_free_tx_buffer(), but tx_skbuff_dma may be NULL if its
    kzalloc_objs() failed.

  - stmmac_free_tx_buffer() dereferences tx_q->xdpf[i] and
    tx_q->tx_skbuff[i] (aliased through a union), but tx_skbuff may
    be NULL if its allocation failed while tx_skbuff_dma succeeded.

Add NULL checks in the free helpers as a defensive measure, since they may
be called on partially-initialized queues.
Additionally, make __free_dma_rx_desc_resources() and
__free_dma_tx_desc_resources() clear the pointers they free, so that
the NULL guards in the free helpers hold reliably when the long-lived
priv->dma_conf is reused across XDP open/release cycles.

Signed-off-by: Ding Hui <dinghui@lixiang.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 32 +++++++++++++++++--
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b20e5038eead..2f03600cc5bd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1766,7 +1766,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
 					 DMA_TO_DEVICE);
 	}
 
-	if (tx_q->xdpf[i] &&
+	if (tx_q->xdpf && tx_q->xdpf[i] &&
 	    (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_TX ||
 	     tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_NDO)) {
 		xdp_return_frame(tx_q->xdpf[i]);
@@ -1778,7 +1778,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
 		tx_q->tx_skbuff_dma[i].buf_type = STMMAC_TXBUF_T_SKB;
 	}
 
-	if (tx_q->tx_skbuff[i] &&
+	if (tx_q->tx_skbuff && tx_q->tx_skbuff[i] &&
 	    tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_SKB) {
 		dev_kfree_skb_any(tx_q->tx_skbuff[i]);
 		tx_q->tx_skbuff[i] = NULL;
@@ -1801,6 +1801,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
 	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
 	int i;
 
+	/* buf_pool may not be allocated if alloc failed early */
+	if (!rx_q->buf_pool)
+		return;
+
 	for (i = 0; i < dma_conf->dma_rx_size; i++)
 		stmmac_free_rx_buffer(priv, rx_q, i);
 }
@@ -1842,6 +1846,10 @@ static void dma_free_rx_xskbufs(struct stmmac_priv *priv,
 	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
 	int i;
 
+	/* buf_pool may not be allocated if alloc failed early */
+	if (!rx_q->buf_pool)
+		return;
+
 	for (i = 0; i < dma_conf->dma_rx_size; i++) {
 		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
 
@@ -2137,6 +2145,10 @@ static void dma_free_tx_skbufs(struct stmmac_priv *priv,
 	struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[queue];
 	int i;
 
+	/* tx_skbuff_dma may not be allocated if alloc failed early */
+	if (!tx_q->tx_skbuff_dma)
+		return;
+
 	tx_q->xsk_frames_done = 0;
 
 	for (i = 0; i < dma_conf->dma_tx_size; i++)
@@ -2194,13 +2206,20 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
 	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
 
 	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
+	rx_q->dma_erx = NULL;
+	rx_q->dma_rx = NULL;
+	rx_q->dma_rx_phy = 0;
 
 	if (xdp_rxq_info_is_reg(&rx_q->xdp_rxq))
 		xdp_rxq_info_unreg(&rx_q->xdp_rxq);
 
 	kfree(rx_q->buf_pool);
-	if (rx_q->page_pool)
+	rx_q->buf_pool = NULL;
+
+	if (rx_q->page_pool) {
 		page_pool_destroy(rx_q->page_pool);
+		rx_q->page_pool = NULL;
+	}
 }
 
 static void free_dma_rx_desc_resources(struct stmmac_priv *priv,
@@ -2242,9 +2261,16 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
 	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
 
 	dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);
+	tx_q->dma_etx = NULL;
+	tx_q->dma_entx = NULL;
+	tx_q->dma_tx = NULL;
+	tx_q->dma_tx_phy = 0;
 
 	kfree(tx_q->tx_skbuff_dma);
+	tx_q->tx_skbuff_dma = NULL;
+
 	kfree(tx_q->tx_skbuff);
+	tx_q->tx_skbuff = NULL;
 }
 
 static void free_dma_tx_desc_resources(struct stmmac_priv *priv,
-- 
2.34.1

Re: [PATCH net-next v6 RESEND 1/4] net: stmmac: guard against NULL pointers when freeing TX/RX buffers

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · Medium: 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).

- [Medium] The commit message understates what the patch fixes and has no
  Fixes: tag.

Pre-existing issues:
- [High] stmmac_enable_rx_queue() (stmmac_main.c:7204) and
  stmmac_enable_tx_queue() (stmmac_main.c:7254) return void.
- [High] stmmac_xdp_set_prog() (stmmac_xdp.c) calls stmmac_xdp_release()
  and then stmmac_xdp_open(dev), but ignores the return value.
- [High] stmmac_xdp_enable_pool() and stmmac_xdp_disable_pool()
  (stmmac_xdp.c) call stmmac_disable_tx_queue() ->…

--

net: stmmac: guard against NULL pointers when freeing TX/RX buffers

This adds NULL checks to the stmmac TX/RX buffer free helpers so they can
be called on queues whose buffer arrays were never allocated. It also
clears the freed pointers in __free_dma_rx_desc_resources() and
__free_dma_tx_desc_resources() so priv->dma_conf can be reused across XDP
open/release cycles.

> Add NULL checks in the free helpers as a defensive measure, since they may
> be called on partially-initialized queues.
> Additionally, make __free_dma_rx_desc_resources() and
> __free_dma_tx_desc_resources() clear the pointers they free, so that
> the NULL guards in the free helpers hold reliably when the long-lived
> priv->dma_conf is reused across XDP open/release cycles.

[Severity: Medium]
Is "a defensive measure" the right description here? It looks like this
fixes two crashes that can be hit on an ordinary allocation failure.

First, when __alloc_dma_rx_desc_resources() fails on queue k,
alloc_dma_rx_desc_resources() unwinds with:

err_dma:
	free_dma_rx_desc_resources(priv, dma_conf);

This walks all rx_queues_to_use queues, including the ones after k that
were never allocated. Their buf_pool is NULL, so the following path
dereferences NULL:

free_dma_rx_desc_resources()
  __free_dma_rx_desc_resources()
    dma_free_rx_skbufs()
      stmmac_free_rx_buffer()
        buf = &rx_q->buf_pool[i];
        if (buf->page)

The TX side does the same thing with tx_q->tx_skbuff_dma[i].buf in
stmmac_free_tx_buffer().

Second, stmmac_xdp_set_prog() calls stmmac_xdp_release() and then
stmmac_xdp_open() on the same priv->dma_conf. Before this patch the free
path left buf_pool, page_pool, the coherent rings and tx_skbuff(_dma)
dangling. A failure in stmmac_xdp_open()->alloc_dma_desc_resources() then
made the unwind kfree() and dma_free_coherent() those stale pointers a
second time, and call page_pool_destroy() on a pool that was already
destroyed.

Should the commit message describe these as the NULL dereference and
double free they are? Should it also carry a Fixes: tag so it can go
through net and be picked up by stable?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a2177777727ef..fcc80ece37679 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -2198,13 +2210,20 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
>  
>  	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
> +	rx_q->dma_erx = NULL;
> +	rx_q->dma_rx = NULL;
> +	rx_q->dma_rx_phy = 0;

[Severity: High]
This is a pre-existing issue, but what happens on the AF_XDP pool path
when the re-allocation after this free fails?

stmmac_xdp_enable_pool() does:

	stmmac_disable_rx_queue(priv, queue);
	stmmac_disable_tx_queue(priv, queue);
	...
	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);

stmmac_enable_rx_queue() and stmmac_enable_tx_queue() return void. On
failure they only log a message:

	ret = __alloc_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
	if (ret) {
		netdev_err(priv->dev, "Failed to alloc RX desc.\n");
		return;
	}

Suppose the RX allocation fails and the TX one succeeds. tx_q->xsk_pool
is then set, so stmmac_xsk_wakeup() schedules rxtx_napi.
stmmac_napi_poll_rxtx()->stmmac_rx_zc() then does the following with
rx_q->dma_rx now NULL:

	p = stmmac_get_rx_desc(priv, rx_q, entry);
	status = stmmac_rx_status(priv, &priv->xstats, p);

A TX failure has the same problem with tx_q->dma_tx and tx_skbuff_dma.

With this patch the result is a NULL dereference in NAPI context instead
of a use-after-free of the freed ring.

Could the enable helpers return an error? That would let
stmmac_xdp_enable_pool() and stmmac_xdp_disable_pool() avoid enabling
NAPI on a queue that has no rings.

[ ... ]

> @@ -2246,9 +2265,16 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
>  	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
>  
>  	dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);
> +	tx_q->dma_etx = NULL;
> +	tx_q->dma_entx = NULL;
> +	tx_q->dma_tx = NULL;
> +	tx_q->dma_tx_phy = 0;

[Severity: High]
This isn't a bug introduced by this patch, but stmmac_xdp_set_prog()
ignores the return value of stmmac_xdp_open():

	if (if_running && need_update)
		stmmac_xdp_open(dev);

	if (prog)
		xdp_features_set_redirect_target(dev, true);

	return 0;

stmmac_xdp_open() can fail in alloc_dma_desc_resources(),
init_dma_desc_rings() or stmmac_request_irq(). In each case it returns
with the rings freed (now NULL), the IRQs released and all NAPIs still
disabled. The device stays IFF_UP and STMMAC_DOWN is not set.

Can a later ndo_stop, XDP program change or suspend then hang?
stmmac_disable_all_queues() would call napi_disable() on NAPIs that are
already disabled. napi_disable_locked() would then wait forever, with
RTNL held, for NAPIF_STATE_SCHED and NAPIF_STATE_NPSVC to clear.
stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0) would also run a second time on
IRQs that were already freed.

There is also a problem on the redirect side, because stmmac_xdp_xmit()
only checks STMMAC_DOWN:

	if (unlikely(test_bit(STMMAC_DOWN, &priv->state)))
		return -ENETDOWN;

Frames redirected to this device from another interface therefore reach
stmmac_xdp_xmit_xdpf(), which writes tx_q->tx_skbuff_dma[entry] and
tx_q->xdpf[entry]. With this patch those writes go near NULL. Before it
they went into freed memory.

>  
>  	kfree(tx_q->tx_skbuff_dma);
> +	tx_q->tx_skbuff_dma = NULL;
> +
>  	kfree(tx_q->tx_skbuff);
> +	tx_q->tx_skbuff = NULL;
>  }

[Severity: High]
This is a pre-existing issue, but is there anything that keeps the
transmit paths away from these arrays while they are being freed and
cleared here?

stmmac_xdp_enable_pool() and stmmac_xdp_disable_pool() call
stmmac_disable_tx_queue()->__free_dma_tx_desc_resources() on a running
interface while holding only RTNL:

	napi_disable(&ch->rxtx_napi);
	stmmac_disable_rx_queue(priv, queue);
	stmmac_disable_tx_queue(priv, queue);
	synchronize_rcu();

The netdev TX queue is never stopped, and __netif_tx_lock is not taken.
So stmmac_xmit() and stmmac_tso_xmit() (under HARD_TX_LOCK), and
stmmac_xdp_xmit() (under __netif_tx_lock), can run while tx_q->dma_tx,
tx_skbuff/xdpf and tx_skbuff_dma are being freed. The synchronize_rcu() in
stmmac_xdp_disable_pool() runs only after the memory is already freed.

Before this patch a concurrent xmit wrote into freed memory. With it, the
xmit either hits the window between kfree() and the NULL store, or
dereferences NULL.

If stmmac_enable_tx_queue() then fails, the queue stays transmit-enabled
with these pointers permanently NULL.

Does the TX queue need to be stopped first, the way stmmac_xdp_release()
does with netif_tx_disable()?

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

[PATCH net-next v6 RESEND 2/4] net: stmmac: clean up partially allocated DMA rings on error

[Ding Hui]

From: Ding Hui <dinghui@lixiang.com>

__alloc_dma_rx_desc_resources() and __alloc_dma_tx_desc_resources()
allocate resources in multiple steps but return early on failure
without cleaning up what they have already allocated. The outer
error paths then call the free helpers on partially-initialized
queues, relying on the NULL guards added previously.

Fix this by making each allocation function responsible for undoing
its own allocations on error, following the standard kernel error
handling pattern of cleaning up in reverse order of allocation.

Also free the RX resources in alloc_dma_desc_resources() when the TX
allocation fails, as the callers only free dma_conf itself on that
path.

Signed-off-by: Ding Hui <dinghui@lixiang.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 40 +++++++++++++++----
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 2f03600cc5bd..a8401b04d79f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2333,15 +2333,19 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
 	}
 
 	rx_q->buf_pool = kzalloc_objs(*rx_q->buf_pool, dma_conf->dma_rx_size);
-	if (!rx_q->buf_pool)
-		return -ENOMEM;
+	if (!rx_q->buf_pool) {
+		ret = -ENOMEM;
+		goto err_destroy_pool;
+	}
 
 	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
 
 	addr = dma_alloc_coherent(priv->device, size, &rx_q->dma_rx_phy,
 				  GFP_KERNEL);
-	if (!addr)
-		return -ENOMEM;
+	if (!addr) {
+		ret = -ENOMEM;
+		goto err_free_buf_pool;
+	}
 
 	if (priv->extend_desc)
 		rx_q->dma_erx = addr;
@@ -2357,10 +2361,22 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
 	ret = xdp_rxq_info_reg(&rx_q->xdp_rxq, priv->dev, queue, napi_id);
 	if (ret) {
 		netdev_err(priv->dev, "Failed to register xdp rxq info\n");
-		return -EINVAL;
+		goto err_free_dma;
 	}
 
 	return 0;
+
+err_free_dma:
+	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
+	rx_q->dma_erx = NULL;
+	rx_q->dma_rx = NULL;
+err_free_buf_pool:
+	kfree(rx_q->buf_pool);
+	rx_q->buf_pool = NULL;
+err_destroy_pool:
+	page_pool_destroy(rx_q->page_pool);
+	rx_q->page_pool = NULL;
+	return ret;
 }
 
 static int alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
@@ -2413,14 +2429,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
 
 	tx_q->tx_skbuff = kzalloc_objs(struct sk_buff *, dma_conf->dma_tx_size);
 	if (!tx_q->tx_skbuff)
-		return -ENOMEM;
+		goto err_free_skbuff_dma;
 
 	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
 
 	addr = dma_alloc_coherent(priv->device, size,
 				  &tx_q->dma_tx_phy, GFP_KERNEL);
 	if (!addr)
-		return -ENOMEM;
+		goto err_free_skbuff;
 
 	if (priv->extend_desc)
 		tx_q->dma_etx = addr;
@@ -2430,6 +2446,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
 		tx_q->dma_tx = addr;
 
 	return 0;
+
+err_free_skbuff:
+	kfree(tx_q->tx_skbuff);
+	tx_q->tx_skbuff = NULL;
+err_free_skbuff_dma:
+	kfree(tx_q->tx_skbuff_dma);
+	tx_q->tx_skbuff_dma = NULL;
+	return -ENOMEM;
 }
 
 static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
@@ -2472,6 +2496,8 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv,
 		return ret;
 
 	ret = alloc_dma_tx_desc_resources(priv, dma_conf);
+	if (ret)
+		free_dma_rx_desc_resources(priv, dma_conf);
 
 	return ret;
 }
-- 
2.34.1

Re: [PATCH net-next v6 RESEND 2/4] net: stmmac: clean up partially allocated DMA rings on error

[netdev-bot+sashiko]

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

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

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

- [Medium] The commit message misdescribes the patch, and a real leak fix
  ships without a Fixes: tag.

Pre-existing issues:
- [High] stmmac_enable_rx_queue() and stmmac_enable_tx_queue() return
  void.
- [Low] The kernel-doc for __alloc_dma_rx_desc_resources() says it
  'allocates the resources for TX and RX paths' and 'pre-allocated the…

--

net: stmmac: clean up partially allocated DMA rings on error

This commit adds per-queue error unwinding to
__alloc_dma_rx_desc_resources() and __alloc_dma_tx_desc_resources(), so
each function frees what it allocated before it returns an error. It also
makes alloc_dma_desc_resources() free the RX resources when the TX
allocation fails.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index fcc80ece37679..b02371ed2d062 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -2361,10 +2365,22 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	ret = xdp_rxq_info_reg(&rx_q->xdp_rxq, priv->dev, queue, napi_id);
>  	if (ret) {
>  		netdev_err(priv->dev, "Failed to register xdp rxq info\n");
> -		return -EINVAL;
> +		goto err_free_dma;
>  	}
>  
>  	return 0;
> +
> +err_free_dma:
> +	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
> +	rx_q->dma_erx = NULL;
> +	rx_q->dma_rx = NULL;

[Severity: High]

This is a pre-existing issue, but this path now leaves rx_q->dma_rx and
rx_q->dma_erx NULL on failure. Can stmmac_xdp_enable_pool() then run the
zero-copy RX path on a queue that has no descriptor ring?

stmmac_enable_rx_queue() returns void and only logs the failure:

	ret = __alloc_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
	if (ret) {
		netdev_err(priv->dev, "Failed to alloc RX desc.\n");
		return;
	}

stmmac_xdp_enable_pool() has just torn the queue down with
stmmac_disable_rx_queue(). It then continues without checking anything:

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

Suppose the RX re-allocation fails under memory pressure (in
page_pool_create(), kzalloc_objs() or dma_alloc_coherent()) but the TX side
succeeds. Then tx_q->xsk_pool is set, so stmmac_xsk_wakeup() schedules
rxtx_napi, and we get:

stmmac_napi_poll_rxtx()
  stmmac_rx_zc()
    p = stmmac_get_rx_desc(priv, rx_q, entry);   /* &NULL[entry] */
    status = stmmac_rx_status(priv, &priv->xstats, p);

Would that be a NULL pointer dereference in softirq context?

stmmac_xdp_disable_pool() has the same pattern: it calls the enable helpers
without checking the result, then calls napi_enable() on rx_napi and
tx_napi.

The allocation failures, which are the realistic triggers, already left
NULL rings before this patch. The only change here is the
xdp_rxq_info_reg() failure case, which used to leave a zeroed but allocated
ring and now leaves NULL pointers.

> +err_free_buf_pool:
> +	kfree(rx_q->buf_pool);
> +	rx_q->buf_pool = NULL;
> +err_destroy_pool:
> +	page_pool_destroy(rx_q->page_pool);
> +	rx_q->page_pool = NULL;
> +	return ret;
>  }

[Severity: Low]

This isn't a bug introduced by this patch, but the kernel-doc above
__alloc_dma_rx_desc_resources() still says:

 * this function allocates the resources for TX and RX paths. In case of
 * reception, for example, it pre-allocated the RX socket buffer in order to
 * allow zero-copy mechanism.

This per-queue helper only handles RX. It also does not allocate RX
buffers; that happens later, in __init_dma_rx_desc_rings().
alloc_dma_desc_resources() has the same comment.

This patch reworks the error handling in both functions. Could these
comments be updated at the same time?

[ ... ]

> @@ -2476,6 +2500,8 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv,
>  		return ret;
>  
>  	ret = alloc_dma_tx_desc_resources(priv, dma_conf);
> +	if (ret)
> +		free_dma_rx_desc_resources(priv, dma_conf);

[Severity: Medium]

This hunk looks like the only part of the patch that fixes a real leak.
Before this patch, if alloc_dma_tx_desc_resources() failed here, every RX
queue kept its page_pool, buf_pool, coherent descriptor ring and xdp_rxq
registration. stmmac_setup_dma_desc() then only did:

alloc_error:
	kfree(dma_conf);
	return ERR_PTR(ret);

This structure seems to date back to commit 71fedb0198cbbd ("stmmac: break
some functions into RX and TX scopes"). Should this hunk have a Fixes: tag?
It could also go in its own patch so stable can pick it up.

The commit message says the outer error paths are "relying on the NULL
guards added previously". That reads as if the per-queue unwinding removes
the dependency. Does it?

alloc_dma_rx_desc_resources() still does:

err_dma:
	free_dma_rx_desc_resources(priv, dma_conf);

That loop walks all rx_queues_to_use queues. It includes the failed queue,
which is now already cleaned up, and any later queues that were never
allocated. alloc_dma_tx_desc_resources() works the same way. So the free
helpers still depend on checks such as !rx_q->buf_pool in
dma_free_rx_skbufs().

Also, before this patch those err_dma labels already freed whatever a
failed queue had partly allocated. The per-queue unwinding therefore
doesn't appear to fix a leak on the open, MTU change or ringparam paths.

The only callers whose behaviour changes are stmmac_enable_rx_queue() and
stmmac_enable_tx_queue(). On those paths the leftovers stayed in
priv->dma_conf until the next disable or release, so nothing leaked.

The commit message also says "the callers only free dma_conf itself on
that path". stmmac_xdp_open() frees nothing on that path:

dma_desc_error:
	return ret;

Could the commit message be reworded to describe what the patch actually
fixes?

>  
>  	return ret;
>  }

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

[PATCH net-next v6 RESEND 3/4] net: stmmac: release descriptor resources through priv->dma_conf on open failure

[Ding Hui]

From: Ding Hui <dinghui@lixiang.com>

__stmmac_open() copies the temporary dma_conf passed by its callers
into the long-lived priv->dma_conf before setting up the hardware.
When the open fails after this copy, the descriptor resources now
belong to priv->dma_conf, but the error path returns without
releasing them and leaves stale pointers behind. The callers then
free the already-copied resources through the temporary dma_conf,
which aliases priv->dma_conf after the memcpy.

Fix this by making __stmmac_open() release the descriptor resources
through priv->dma_conf on failure. Callers of __stmmac_open() only
need to free the temporary dma_conf container in that case, while
failures before the call (PHY setup, serdes power up) still release
the resources through the temporary dma_conf via the existing
err_dma_resources path in stmmac_open().

stmmac_open() no longer jumps to the serdes label chain after
__stmmac_open() fails, as that chain would double-release the
descriptor resources and double-put the runtime PM reference, so the
cleanup is done inline instead.

The resource release is merged with the error unwinding that stops
the DMA engines and the MAC: failures after the hardware setup
first stop the DMA engines and the MAC, and then fall through to
the common init_error label that releases the descriptor resources,
while a stmmac_hw_setup() failure jumps to init_error directly as
the engines were never started.

Signed-off-by: Ding Hui <dinghui@lixiang.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c    | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a8401b04d79f..b093119e752f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4246,7 +4246,7 @@ static int __stmmac_open(struct net_device *dev,
 	ret = stmmac_hw_setup(dev);
 	if (ret < 0) {
 		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
-		return ret;
+		goto init_error;
 	}
 
 	ret = stmmac_setup_ptp(priv);
@@ -4282,6 +4282,8 @@ static int __stmmac_open(struct net_device *dev,
 	stmmac_stop_all_dma(priv);
 	stmmac_mac_set(priv, priv->ioaddr, false);
 
+init_error:
+	free_dma_desc_resources(priv, &priv->dma_conf);
 	return ret;
 }
 
@@ -4314,8 +4316,13 @@ static int stmmac_open(struct net_device *dev)
 	}
 
 	ret = __stmmac_open(dev, dma_conf);
-	if (ret)
-		goto err_serdes;
+	if (ret) {
+		stmmac_legacy_serdes_power_down(priv);
+		phylink_disconnect_phy(priv->phylink);
+		pm_runtime_put(priv->device);
+		kfree(dma_conf);
+		return ret;
+	}
 
 	kfree(dma_conf);
 
@@ -4324,8 +4331,6 @@ static int stmmac_open(struct net_device *dev)
 
 	return ret;
 
-err_serdes:
-	stmmac_legacy_serdes_power_down(priv);
 err_disconnect_phy:
 	phylink_disconnect_phy(priv->phylink);
 err_runtime_pm:
@@ -6343,7 +6348,6 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
 
 		ret = __stmmac_open(dev, dma_conf);
 		if (ret) {
-			free_dma_desc_resources(priv, dma_conf);
 			kfree(dma_conf);
 			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
 			return ret;
-- 
2.34.1

Re: [PATCH net-next v6 RESEND 3/4] net: stmmac: release descriptor resources through priv->dma_conf on open failure

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This commit fixes a bug but has no Fixes: tag and no Cc: stable.
- [Low] The commit message says stmmac_open() stops jumping into the
  err_serdes chain after __stmmac_open() fails because that chain 'would…
- [Low] The patch changes the ownership contract of __stmmac_open().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a failed
  internal reopen leaves the netdev IFF_UP/netif_running() while the…

--

net: stmmac: release descriptor resources through priv->dma_conf on open failure

After __stmmac_open() copies the temporary dma_conf, it now releases the
descriptor resources through priv->dma_conf when it fails, so callers
only kfree() the container. When __stmmac_open() fails, stmmac_open()
now cleans up inline instead of jumping into the err_serdes label chain.
stmmac_change_mtu() no longer calls free_dma_desc_resources() itself.

> stmmac_open() no longer jumps to the serdes label chain after
> __stmmac_open() fails, as that chain would double-release the
> descriptor resources and double-put the runtime PM reference, so the
> cleanup is done inline instead.

[Severity: Low]
Is the runtime PM part of this description accurate?

After this patch __stmmac_open() makes no pm_runtime_* calls. Its error
labels only stop the DMA engines and the MAC, release PTP and free the
descriptors.

The old chain err_serdes -> err_disconnect_phy -> err_runtime_pm ->
err_dma_resources calls pm_runtime_put(priv->device) once. The new
inline block also calls it once.

The only real problem with reusing the chain seems to be the second
free_dma_desc_resources(priv, dma_conf) on the stale temporary copy.
Could the labels be rearranged, for example with one just before the
kfree(dma_conf), so the cleanup sequence isn't duplicated inline?

> Signed-off-by: Ding Hui <dinghui@lixiang.com>

[Severity: Low]
Should this have a Fixes: tag and Cc: stable?

The caller-side free through the temporary dma_conf appears to come from
commit 30134b7c47bd ("net: ethernet: stmicro: stmmac: fix possible
memory leak in __stmmac_open"). That commit removed
free_dma_desc_resources(priv, &priv->dma_conf) from __stmmac_open() and
moved the free into stmmac_open() and stmmac_change_mtu().

That commit carries Cc: stable@vger.kernel.org, so stable trees have the
same caller-side free.

Backporters would also need "net: stmmac: guard against NULL pointers
when freeing TX/RX buffers" from this series. The later
free_dma_desc_resources(priv, &priv->dma_conf) in __stmmac_release() is
only safe when the free helpers NULL their pointers.

Could the commit message also describe what the user sees? It only
mentions "stale pointers". It doesn't say that freed coherent ring
memory can still be read without locking, for example by the debugfs
reader stmmac_rings_status_show(), while IFF_UP stays set after a failed
MTU change reopen.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b02371ed2d062..cd4631efe47ab 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4290,6 +4290,8 @@ static int __stmmac_open(struct net_device *dev,
>  	stmmac_stop_all_dma(priv);
>  	stmmac_mac_set(priv, priv->ioaddr, false);
>  
> +init_error:
> +	free_dma_desc_resources(priv, &priv->dma_conf);
>  	return ret;
>  }

[Severity: Low]
With this change, __stmmac_open() takes ownership of the descriptor
resources passed in through @dma_conf and releases them on any failure.
Both callers now only kfree() the container.

The kernel-doc above __stmmac_open() still only says:

 *  @dma_conf :  structure to take the dma data

The same phrase is used on many other helpers in this file, so it says
nothing about ownership.

Could the kernel-doc state that the callee releases the descriptor
resources on failure, so a future caller does not free them a second
time?

[ ... ]

> @@ -6384,7 +6389,6 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  
>  		ret = __stmmac_open(dev, dma_conf);
>  		if (ret) {
> -			free_dma_desc_resources(priv, dma_conf);
>  			kfree(dma_conf);
>  			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
>  			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but what state is the device
left in when this reopen fails?

Before the reopen, __stmmac_release() has already run
stmmac_disable_all_queues(), which calls napi_disable() on every
rx/tx/rxtx NAPI. It has also freed the IRQs and the rings.

__stmmac_open() can then fail, for example on a DMA reset timeout in
stmmac_init_dma_engine(), on stmmac_rxp_config(), on PTP or EST setup,
or on stmmac_request_irq(). Every one of its error labels returns before
stmmac_enable_all_queues().

stmmac_change_mtu() then returns without clearing IFF_UP, so
netif_running() stays true.

On the next ifdown or unregister_netdev():

stmmac_release()
  __stmmac_release()
    stmmac_disable_all_queues()
      __stmmac_disable_all_queues()
        napi_disable(&ch->rx_napi)

This calls napi_disable() a second time on a NAPI that is already
disabled. napi_disable_locked() in net/core/dev.c loops:

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

Those bits are still set from the first disable. Can this spin forever
with RTNL held?

stmmac_suspend() also trusts netif_running() and reaches the same
napi_disable() while holding priv->lock, so system suspend looks like it
would hang too.

stmmac_reinit_ringparam() and stmmac_reinit_queues() release the device
and then call stmmac_open() the same way. If that stmmac_open() fails,
its error path has already dropped the runtime PM reference and
disconnected the PHY. A later stmmac_release() would do both again.

The hang hides some other problems. The memcpy() in __stmmac_open()
overwrites priv->dma_conf.tx_queue[].txtimer with the kzalloc'ed zeros.
On a stmmac_hw_setup(), PTP or EST failure, stmmac_init_coalesce()
never sets the timer up again, so the hrtimer_cancel() in
__stmmac_release() would act on an uninitialized hrtimer.

stmmac_free_irq(REQ_IRQ_ERR_ALL) would also run on IRQs that were never
requested.

This patch makes the repeated free_dma_desc_resources(priv,
&priv->dma_conf) safe because the free helpers NULL the pointers. The
last patch in the series only guards the debugfs reader. Nothing in the
series seems to handle a device that still reports running after it has
been torn down.

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

[PATCH net-next v6 RESEND 4/4] net: stmmac: hold RTNL when reading descriptor rings in descriptors_status

[Ding Hui]

From: Ding Hui <dinghui@lixiang.com>

The descriptors_status debugfs entry inspects the descriptor rings
without any synchronization against the control-plane paths that
tear them down (ndo_stop, MTU change, XDP program release, XSK pool
reconfiguration). All of those paths hold RTNL, so take the same
lock around the reader.

Holding RTNL also makes it safe to report queues whose descriptor
rings are not allocated: the interface may be IFF_UP with unallocated
rings when an internal reopen fails (e.g. an MTU change whose
__stmmac_open() call fails), as those paths do not clear IFF_UP.
Skip such queues with a hint instead of dereferencing a NULL pointer
in stmmac_rings_status_show().

Signed-off-by: Ding Hui <dinghui@lixiang.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 27 ++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b093119e752f..0f4af7ec3fc8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6707,8 +6707,10 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
 	u8 tx_count = priv->plat->tx_queues_to_use;
 	u8 queue;
 
+	rtnl_lock();
+
 	if ((dev->flags & IFF_UP) == 0)
-		return 0;
+		goto out;
 
 	for (queue = 0; queue < rx_count; queue++) {
 		struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
@@ -6716,10 +6718,20 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
 		seq_printf(seq, "RX Queue %d:\n", queue);
 
 		if (priv->extend_desc) {
+			if (!rx_q->dma_erx) {
+				seq_puts(seq, "Extended descriptor ring not allocated\n");
+				continue;
+			}
+
 			seq_printf(seq, "Extended descriptor ring:\n");
 			sysfs_display_ring((void *)rx_q->dma_erx,
 					   priv->dma_conf.dma_rx_size, 1, seq, rx_q->dma_rx_phy);
 		} else {
+			if (!rx_q->dma_rx) {
+				seq_puts(seq, "Descriptor ring not allocated\n");
+				continue;
+			}
+
 			seq_printf(seq, "Descriptor ring:\n");
 			sysfs_display_ring((void *)rx_q->dma_rx,
 					   priv->dma_conf.dma_rx_size, 0, seq, rx_q->dma_rx_phy);
@@ -6732,16 +6744,29 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
 		seq_printf(seq, "TX Queue %d:\n", queue);
 
 		if (priv->extend_desc) {
+			if (!tx_q->dma_etx) {
+				seq_puts(seq, "Extended descriptor ring not allocated\n");
+				continue;
+			}
+
 			seq_printf(seq, "Extended descriptor ring:\n");
 			sysfs_display_ring((void *)tx_q->dma_etx,
 					   priv->dma_conf.dma_tx_size, 1, seq, tx_q->dma_tx_phy);
 		} else if (!(tx_q->tbs & STMMAC_TBS_AVAIL)) {
+			if (!tx_q->dma_tx) {
+				seq_puts(seq, "Descriptor ring not allocated\n");
+				continue;
+			}
+
 			seq_printf(seq, "Descriptor ring:\n");
 			sysfs_display_ring((void *)tx_q->dma_tx,
 					   priv->dma_conf.dma_tx_size, 0, seq, tx_q->dma_tx_phy);
 		}
 	}
 
+out:
+	rtnl_unlock();
+
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(stmmac_rings_status);
-- 
2.34.1

Re: [PATCH net-next v6 RESEND 4/4] net: stmmac: hold RTNL when reading descriptor rings in descriptors_status

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a NULL pointer dereference that a user can
  trigger, but its message has no Fixes: tag (and no Cc: stable).
- [Low] stmmac_rings_status_show() reads its loop bounds before taking
  RTNL.

--

net: stmmac: hold RTNL when reading descriptor rings in descriptors_status

This commit takes RTNL around the descriptors_status debugfs reader so it
is serialized against the control-plane paths that free the descriptor
rings. It also skips queues whose rings are NULL, which happens when the
interface stays IFF_UP after a failed internal reopen.

> Skip such queues with a hint instead of dereferencing a NULL pointer
> in stmmac_rings_status_show().
>
> Signed-off-by: Ding Hui <dinghui@lixiang.com>

[Severity: Low]
This fixes a NULL pointer dereference that can be triggered from debugfs,
but the commit has no Fixes: tag and no Cc: stable. Should they be added so
the affected stable trees pick this up?

The state described above comes from stmmac_change_mtu() when the reopen
fails:

stmmac_change_mtu()
    __stmmac_release(dev);
    ret = __stmmac_open(dev, dma_conf);
        init_error:
            free_dma_desc_resources(priv, &priv->dma_conf);

free_dma_desc_resources() frees the rings and sets dma_rx/dma_erx and
dma_tx/dma_etx to NULL. IFF_UP stays set.

Before this patch, stmmac_rings_status_show() checked only IFF_UP. It then
passed the NULL ring to sysfs_display_ring(), which dereferences p->des0. A
reader that raced with ndo_stop, an MTU change, an XDP change or an XSK
change could also read DMA memory while it was being freed.

There are two candidate targets. One is the commit that added the
unsynchronized descriptors_status reader. The other is the commit that
added the MTU-change reopen path:

Fixes: 347007968744 ("net: ethernet: stmicro: stmmac: permit MTU change with interface up")

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index cd4631efe47ab..10d33a43db947 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6748,8 +6748,10 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
>  	u8 tx_count = priv->plat->tx_queues_to_use;
>  	u8 queue;
>  
> +	rtnl_lock();
> +

[Severity: Low]
Are the loop bounds still read outside the lock? rx_count and tx_count are
set in the declarations, before rtnl_lock() is taken:

    u8 rx_count = priv->plat->rx_queues_to_use;
    u8 tx_count = priv->plat->tx_queues_to_use;

stmmac_reinit_queues() (ethtool -L) runs under RTNL. It releases the rings
and then updates both fields:

stmmac_reinit_queues()
    stmmac_release(dev);
    ...
    priv->plat->rx_queues_to_use = rx_cnt;
    priv->plat->tx_queues_to_use = tx_cnt;
    ...
    ret = stmmac_open(dev);

Suppose the reader loads the counts and then blocks in rtnl_lock() while
set_channels runs. The loops then use stale bounds.

If the count shrank, the reader prints "Descriptor ring not allocated" for
queues that no longer exist. If it grew, live queues are skipped without
any message. This is also a data race that KCSAN could report, since the
fields are written under RTNL.

The rx_queue[] and tx_queue[] arrays are sized by MTL_MAX_*_QUEUES, so the
accesses stay in bounds. Only the debugfs output is affected.

The commit message says RTNL now synchronizes this reader with the control
plane. Would it make sense to read rx_queues_to_use and tx_queues_to_use
after rtnl_lock() as well?

[ ... ]

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

Re: [PATCH net-next v6 RESEND 0/4] net: stmmac: fix DMA descriptor ring cleanup issues

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.