* [PATCH net v4] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
@ 2026-10-01 3:50 Ding Hui
2026-10-01 3:58 ` netdev-bot+sinfo
2026-10-05 3:51 ` netdev-bot+sashiko
0 siblings, 2 replies; 4+ messages in thread
From: Ding Hui @ 2026-10-01 3:50 UTC (permalink / raw)
To: netdev-bot+sashiko, Maxime Chevallier, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Maxime Coquelin, Alexandre Torgue,
open list:STMMAC ETHERNET DRIVER,
moderated list:ARM/STM32 ARCHITECTURE,
moderated list:ARM/STM32 ARCHITECTURE, open list
Cc: dinghui, xiasanbo, yangchen11, liuxuanjun
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, which dereference pointers that 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.
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. Also 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.
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. And skip queues whose descriptor rings are not allocated
in stmmac_rings_status_show() so reading the rings sysfs entry will
never dereference a NULL pointer.
Signed-off-by: Ding Hui <dinghui@lixiang.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.
- Update commit message.
- Link to v1:
https://lore.kernel.org/netdev/20260830040610.1156008-1-dinghui1111@163.com/
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 92 +++++++++++++++++--
1 file changed, 82 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f2fc89176654..e79dd726b1ca 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1728,7 +1728,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]);
@@ -1738,7 +1738,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
if (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XSK_TX)
tx_q->xsk_frames_done++;
- 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;
@@ -1761,6 +1761,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);
}
@@ -1802,6 +1806,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];
@@ -2097,6 +2105,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++)
@@ -2154,13 +2166,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,
@@ -2202,9 +2221,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,
@@ -2272,15 +2298,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;
@@ -2296,10 +2326,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,
@@ -2352,14 +2394,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;
@@ -2369,6 +2411,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,
@@ -2411,6 +2461,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;
}
@@ -6510,10 +6562,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);
@@ -6526,10 +6588,20 @@ 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);
--
2.34.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net v4] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
2026-10-01 3:50 [PATCH net v4] net: stmmac: fix error path cleanup in DMA descriptor ring allocation Ding Hui
@ 2026-10-01 3:58 ` netdev-bot+sinfo
2026-10-01 14:06 ` Ding Hui
2026-10-05 3:51 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: netdev-bot+sinfo @ 2026-10-01 3:58 UTC (permalink / raw)
To: Ding Hui
Cc: netdev-bot+sashiko, Maxime Chevallier, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Maxime Coquelin, Alexandre Torgue, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, dinghui, xiasanbo, yangchen11,
liuxuanjun
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re:Re: [PATCH net v4] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
2026-10-01 3:58 ` netdev-bot+sinfo
@ 2026-10-01 14:06 ` Ding Hui
0 siblings, 0 replies; 4+ messages in thread
From: Ding Hui @ 2026-10-01 14:06 UTC (permalink / raw)
To: netdev-bot+sinfo
Cc: alexandre.torgue, andrew+netdev, davem, dinghui1111, dinghui,
edumazet, kuba, linux-arm-kernel, linux-kernel, linux-stm32,
liuxuanjun, maxime.chevallier, mcoquelin.stm32,
netdev-bot+sashiko, netdev, pabeni, xiasanbo, yangchen11
At 2026-10-01 11:58:59, netdev-bot+sinfo@kernel.org wrote:
>Hi!
>
>This is an automated message. This series looks like a fix, but its
>commit messages seem to be missing some information:
>
> - How the issue was discovered, e.g. hit in production, hit during
> development, syzbot report, manual code inspection, LLM or static
> analysis tool scan.
This issue was discovered during a stress testing scenario in the development process.
>
> - 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.
>
The specific scenario triggering this issue involves performing `ifconfig down/up` operations
on a network device during an OOM (Out of Memory) condition. The call stack at the time of the
failure may like this:
[540776.647689] Call trace:
[540776.647691] dump_backtrace+0x98/0xf8
[540776.647701] show_stack+0x20/0x38
[540776.647704] dump_stack_lvl+0xbc/0xd0
[540776.647719] dump_stack+0x18/0x28
[540776.647723] warn_alloc+0x138/0x1d0
[540776.647731] __alloc_pages_noprof+0x4e8/0xfd0
[540776.647735] ___kmalloc_large_node+0xb8/0x1a8
[540776.647740] __kmalloc_large_node_noprof+0x34/0x118
[540776.647743] __kmalloc_noprof+0x2d4/0x378
[540776.647747] __alloc_dma_tx_desc_resources+0x4c/0x118
[540776.647753] alloc_dma_desc_resources+0xd8/0x150
[540776.647756] stmmac_setup_dma_desc+0x118/0x270
[540776.647759] stmmac_open+0x30/0xe8
[540776.647762] __dev_open+0x108/0x1f8
[540776.647767] __dev_change_flags+0x1d4/0x268
[540776.647770] dev_change_flags+0x2c/0x80
[540776.647773] devinet_ioctl+0x2dc/0x618
[540776.647778] inet_ioctl+0x1d4/0x1f0
[540776.647781] sock_do_ioctl+0x68/0x130
[540776.647786] sock_ioctl+0x288/0x398
[540776.647788] __arm64_sys_ioctl+0xb0/0x100
[540776.647795] invoke_syscall+0x84/0x108
[540776.647801] el0_svc_common.constprop.0+0xc8/0xf0
[540776.647805] do_el0_svc+0x24/0x38
[540776.647808] el0_svc+0x38/0x120
[540776.647813] el0t_64_sync_handler+0x120/0x130
[540776.647816] el0t_64_sync+0x190/0x198
[540776.648028] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[540776.648031] Mem abort info:
[540776.648033] ESR = 0x0000000096000006
[540776.648035] EC = 0x25: DABT (current EL), IL = 32 bits
[540776.648038] SET = 0, FnV = 0
[540776.648039] EA = 0, S1PTW = 0
[540776.648041] FSC = 0x06: level 2 translation fault
[540776.648043] Data abort info:
[540776.648045] ISV = 0, ISS = 0x00000006, ISS2 = 0x00000000
[540776.648047] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[540776.648049] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[540776.648051] user pgtable: 4k pages, 39-bit VAs, pgdp=00000013c6eed000
[540776.648053] [0000000000000000] pgd=08000013b0800003, p4d=08000013b0800003, pud=08000013b0800003, pmd=0000000000000000
[540776.648063] Internal error: Oops: 0000000096000006 [#1] PREEMPT_RT SMP
[540776.648145] pstate: 20401005 (nzCv daif +PAN -UAO -TCO -DIT +SSBS BTYPE=--)
[540776.648147] pc : dma_free_tx_skbufs+0x108/0x1b8
[540776.648151] lr : __free_dma_tx_desc_resources+0x2c/0xb8
[540776.648154] sp : ffffffc0bc5f3610
[540776.648156] x29: ffffffc0bc5f3610 x28: ffffff83e4a2c600 x27: ffffff87de595a00
[540776.648162] x26: ffffff87de8a8000 x25: 0000000000001003 x24: ffffff83dcc26000
[540776.648168] x23: 0000000000000000 x22: ffffff87de8a8a00 x21: 0000000000000000
[540776.648174] x20: 0000000000000000 x19: ffffff83dcc26100 x18: ffffffc0bc5f2f20
[540776.648179] x17: 0000000000000000 x16: 0000000000000000 x15: ffffffe62e994454
[540776.648185] x14: ffffffe62e994440 x13: 0a64656e6f73696f x12: 7077682073656761
[540776.648190] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffffe62d20dc4c
[540776.648196] x8 : ffffffc0bc5f34c8 x7 : 0000000000000000 x6 : 0000000000000001
[540776.648201] x5 : ffffffe62e41c000 x4 : ffffffe62e41c600 x3 : 0000000000000000
[540776.648207] x2 : 0000000000000000 x1 : ffffff83dcc26000 x0 : 0000000000001000
[540776.648213] Call trace:
[540776.648215] dma_free_tx_skbufs+0x108/0x1b8
[540776.648217] __free_dma_tx_desc_resources+0x2c/0xb8
[540776.648219] alloc_dma_desc_resources+0x104/0x150
[540776.648222] stmmac_setup_dma_desc+0x118/0x270
[540776.648225] stmmac_open+0x30/0xe8
[540776.648228] __dev_open+0x108/0x1f8
[540776.648231] __dev_change_flags+0x1d4/0x268
[540776.648234] dev_change_flags+0x2c/0x80
[540776.648236] devinet_ioctl+0x2dc/0x618
[540776.648240] inet_ioctl+0x1d4/0x1f0
[540776.648243] sock_do_ioctl+0x68/0x130
[540776.648246] sock_ioctl+0x288/0x398
[540776.648248] __arm64_sys_ioctl+0xb0/0x100
[540776.648252] invoke_syscall+0x84/0x108
[540776.648256] el0_svc_common.constprop.0+0xc8/0xf0
[540776.648260] do_el0_svc+0x24/0x38
[540776.648263] el0_svc+0x38/0x120
[540776.648267] el0t_64_sync_handler+0x120/0x130
[540776.648270] el0t_64_sync+0x190/0x198
[540776.648274] Code: 54000389 f9449262 93797eb4 937d7eb7 (f8746843)
[540776.648277] ---[ end trace 0000000000000000 ]---
[540776.681767] Kernel panic - not syncing: Oops: Fatal exception
Alternatively, a simpler method to reproduce the issue is by injecting a fault
through stubbing `alloc_dma_tx_desc_resources`.
> - 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.
>
This modification was tested and verified on an in-house developed SoC
platform (featuring Synopsys XGMAC).
>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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net v4] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
2026-10-01 3:50 [PATCH net v4] net: stmmac: fix error path cleanup in DMA descriptor ring allocation Ding Hui
2026-10-01 3:58 ` netdev-bot+sinfo
@ 2026-10-05 3:51 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-10-05 3:51 UTC (permalink / raw)
To: dinghui1111
Cc: netdev-bot+sashiko, maxime.chevallier, andrew+netdev, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, alexandre.torgue,
netdev, linux-stm32, linux-arm-kernel, linux-kernel, dinghui,
xiasanbo, yangchen11, liuxuanjun
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 0 (4) · Medium: 2 · Low: 2
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 patch's new 'freed pointers are NULL' invariant is only
written into the dma_conf object passed to…
- [Medium] stmmac_rings_status_show() takes no lock (no rtnl, no
priv->lock).
- [Low] The commit describes and fixes real, user-visible defects but has
no Fixes: trailer.
- [Low] The commit message says the stmmac_rings_status_show() change
keeps 'the rings sysfs entry' from dereferencing a NULL pointer.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
stmmac_enable_rx_queue() and stmmac_enable_tx_queue()…
- [High] This problem wasn't introduced by this patch, but
stmmac_xdp_set_prog() (stmmac_xdp.c:134-140) calls…
- [High] This problem wasn't introduced by this patch, but after a failed
stmmac_xdp_open() inside stmmac_xdp_set_prog() (return value ignored),…
- [High] This problem wasn't introduced by this patch, but in
stmmac_xdp_open() the irq_error path runs after the MAC RX/TX has been…
--
net: stmmac: fix error path cleanup in DMA descriptor ring allocation
This patch makes __alloc_dma_rx_desc_resources() and
__alloc_dma_tx_desc_resources() undo their own allocations on failure. It
also adds NULL checks in the RX/TX buffer free helpers and clears the freed
pointers in __free_dma_{rx,tx}_desc_resources(). Finally, it frees the RX
resources when the TX allocation fails in alloc_dma_desc_resources(), and
skips unallocated rings in stmmac_rings_status_show().
> And skip queues whose descriptor rings are not allocated
> in stmmac_rings_status_show() so reading the rings sysfs entry will
> never dereference a NULL pointer.
[Severity: Low]
This isn't a bug, but stmmac_rings_status_show() backs a debugfs file, not
a sysfs entry:
stmmac_init_fs() {
...
debugfs_create_file("descriptors_status", 0444, priv->dbgfs_dir, dev,
&stmmac_rings_status_fops);
...
}
Could the commit message also explain how the rings end up unallocated
while IFF_UP is set? As far as I can tell, this only happens after one of
two ignored failures:
- stmmac_xdp_open() failing inside stmmac_xdp_set_prog()
- a per-queue re-allocation failing in stmmac_enable_rx_queue() or
stmmac_enable_tx_queue()
In both cases the guard hides the symptom of a half-torn-down interface.
> Signed-off-by: Ding Hui <dinghui@lixiang.com>
[Severity: Low]
Should this have Fixes: tags? The patch fixes two separate problems:
- a NULL dereference of rx_q->buf_pool[i] or tx_q->tx_skbuff_dma[i] in
dma_free_rx_skbufs() and dma_free_tx_skbufs() after a failed
kzalloc_objs()
- a leak of the RX page_pools, buf_pools and coherent rings when
alloc_dma_tx_desc_resources() fails
Before this patch, stmmac_setup_dma_desc() only did kfree(dma_conf) under
alloc_error, and the dma_desc_error label in stmmac_xdp_open() just
returns.
These problems seem to come from different commits, so more than one
Fixes: tag may be needed for net and stable backports.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec62fa7418f41..07176750a0c48 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -2161,13 +2173,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: Medium]
Do these NULL stores ever reach priv->dma_conf on the __stmmac_open()
failure paths?
__stmmac_open() copies the heap dma_conf into priv->dma_conf before the
steps that can fail:
__stmmac_open() {
...
memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
...
}
If stmmac_hw_setup() or stmmac_request_irq() fails after that, the callers
free the temporary copy and not priv->dma_conf:
stmmac_change_mtu() {
...
ret = __stmmac_open(dev, dma_conf);
if (ret) {
free_dma_desc_resources(priv, dma_conf);
kfree(dma_conf);
...
}
stmmac_open() follows the same pattern under err_dma_resources.
So the new NULL stores to dma_rx, buf_pool, page_pool, tx_skbuff and
tx_skbuff_dma all land in memory that is kfree()d right away. Meanwhile
priv->dma_conf keeps non-NULL pointers to the freed coherent rings and
arrays.
After a failed MTU change the netdev stays IFF_UP. The new
"if (!rx_q->dma_rx)" guards in stmmac_rings_status_show() then pass, and
sysfs_display_ring() reads memory that was already released with
dma_free_coherent().
The commit message says clearing the pointers makes "the NULL guards in
the free helpers hold reliably when the long-lived priv->dma_conf is
reused". Does that invariant actually hold for priv->dma_conf on these
paths?
The aliasing itself predates this patch. Also, after this failure, a later
"ip link set down" makes __stmmac_release() call napi_disable() on NAPI
instances that are already disabled. That spins under the rtnl lock, so a
second free_dma_desc_resources() on priv->dma_conf is not reached on that
path.
[ ... ]
> @@ -2209,9 +2228,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 is a pre-existing issue, but is the hardware still pointed at these
rings when they are freed on the stmmac_xdp_open() error path?
stmmac_xdp_open() {
...
stmmac_mac_set(priv, priv->ioaddr, true);
/* Start Rx & Tx DMA Channels */
stmmac_start_all_dma(priv);
ret = stmmac_request_irq(dev);
if (ret)
goto irq_error;
...
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);
...
}
If stmmac_request_irq() fails, the rings are freed with dma_free_coherent()
and the RX pages go back to the page pool. Unlike stmmac_xdp_release(),
this path never calls stmmac_stop_all_dma() or
stmmac_mac_set(..., false).
Can the DMA engine keep fetching descriptors from freed memory and writing
received frames into freed pages?
__stmmac_open() looks similar. stmmac_hw_setup() enables the MAC and starts
DMA, and the irq_error/init_error path leaves both running before the
caller frees the rings.
Setting dma_rx_phy and dma_tx_phy to 0 makes the software state look
clean, but the channel registers still hold the old bus addresses.
[ ... ]
> @@ -2298,10 +2328,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;
> }
[Severity: High]
This isn't a bug introduced by this patch, but what happens on the XSK
pool enable path when this unwind runs?
stmmac_enable_rx_queue() and stmmac_enable_tx_queue() return void and only
log the failure:
stmmac_enable_rx_queue() {
...
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() then carries on regardless:
if (need_update) {
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);
With this patch, the earlier stmmac_disable_rx_queue() and the new unwind
leave rx_q->dma_rx, dma_erx and buf_pool NULL, and tx_q->dma_tx and
tx_skbuff_dma NULL as well.
The rxtx NAPI can still be scheduled, either by stmmac_xsk_wakeup() (which
is also ndo_xsk_wakeup) or by a TX DMA IRQ. Could that reach
stmmac_napi_poll_rxtx()->stmmac_rx_zc(), which uses &rx_q->buf_pool[entry]
and stmmac_get_rx_desc() on the NULL ring?
stmmac_xdp_disable_pool() also re-enables rx_napi and tx_napi after an
enable that may have failed.
Before this patch the same path was a use-after-free of the freed ring,
and the hardware channel still points at that freed ring.
[ ... ]
> @@ -2413,6 +2463,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;
> }
[Severity: High]
This is a pre-existing issue, but stmmac_xdp_open() is one of the callers
of alloc_dma_desc_resources(), and stmmac_xdp_set_prog() ignores its
failure:
stmmac_xdp_set_prog() {
...
if (if_running && need_update)
stmmac_xdp_open(dev);
if (prog)
xdp_features_set_redirect_target(dev, false);
return 0;
}
By this point stmmac_xdp_release() has disabled all NAPI instances, freed
the IRQs, stopped DMA and freed the rings.
stmmac_xdp_open() can fail in alloc_dma_desc_resources(),
init_dma_desc_rings() or stmmac_request_irq(). If it does, userspace still
gets success. The netdev stays IFF_UP with NAPI disabled, no IRQs, and
(with this patch) NULL ring pointers.
Would the next ndo_stop or XDP toggle then call napi_disable() on an
already-disabled NAPI through stmmac_disable_all_queues()?
napi_disable_locked() waits for NAPIF_STATE_SCHED to clear, which only
happens on napi_enable(). That looks like a hang while holding the
rtnl/netdev lock. stmmac_free_irq() would also run a second time.
[Severity: High]
This is a pre-existing issue, but after the failed stmmac_xdp_open()
described above, the device is still advertised as an XDP redirect
target. xdp_features_set_redirect_target() runs whenever prog is set,
even if the reopen failed.
stmmac_xdp_xmit() is only gated on STMMAC_DOWN, and neither
stmmac_xdp_release() nor the failed open sets that bit:
stmmac_xdp_xmit() {
...
if (unlikely(test_bit(STMMAC_DOWN, &priv->state)))
return -ENETDOWN;
...
}
Can a devmap or bpf_redirect from another interface then reach
stmmac_xdp_xmit_xdpf()? That function:
- computes tx_desc with stmmac_get_tx_desc()
- writes tx_q->tx_skbuff_dma[entry] through stmmac_set_tx_dma_entry()
- stores tx_q->xdpf[entry] = xdpf
With this patch these become NULL-pointer writes. Before it, they were
writes into freed memory.
[ ... ]
> @@ -6570,10 +6622,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);
[Severity: Medium]
Is this check enough without any locking against the teardown paths?
stmmac_rings_status_show() takes neither rtnl nor priv->lock. It loads the
pointer for the NULL test, calls seq_printf(), and then loads the field
again for sysfs_display_ring().
Several teardown paths free the rings under rtnl only:
- ndo_stop (IFF_UP is cleared only after ndo_stop returns)
- stmmac_xdp_release() (IFF_UP stays set)
- MTU and ring size changes
- XSK queue disable
With this patch, __free_dma_rx_desc_resources() now does:
dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
rx_q->dma_erx = NULL;
rx_q->dma_rx = NULL;
A reader that already passed the check can then pass NULL to
sysfs_display_ring() with dma_rx_size > 0 and dereference p->des0. If the
second load happens before the NULL store, it reads freed coherent memory
instead.
The use-after-free read predates this patch. The NULL dereference does
not, and it contradicts the commit message claim that reading the entry
"will never dereference a NULL pointer". The same pattern applies to the
dma_rx branch and to both TX branches.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001035045.3305816-1-dinghui1111%40163.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-10-05 3:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-01 3:50 [PATCH net v4] net: stmmac: fix error path cleanup in DMA descriptor ring allocation Ding Hui
2026-10-01 3:58 ` netdev-bot+sinfo
2026-10-01 14:06 ` Ding Hui
2026-10-05 3:51 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®