* Re: [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources
2026-06-30 10:09 ` [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources Jakub Raczynski
@ 2026-07-02 9:24 ` Maxime Chevallier
2026-07-02 10:17 ` Jakub Raczynski
2026-07-03 7:26 ` Paolo Abeni
1 sibling, 1 reply; 5+ messages in thread
From: Maxime Chevallier @ 2026-07-02 9:24 UTC (permalink / raw)
To: Jakub Raczynski, netdev
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, mcoquelin.stm32,
linux-kernel, k.tegowski, k.domagalski, Yashwant Varur
Hi,
On 6/30/26 12:09, Jakub Raczynski wrote:
> When freeing RX descriptor resources, there is standard clearing of
> descriptor page_pool via page_pool_destroy() which does destroy
> page but does not set its pointer to NULL, which must be done by driver
> calling this function.
> It is not done in __free_dma_rx_desc_resources() when stopping interface,
> which is generally not an issue, because __alloc_dma_rx_desc_resources() does
> setup this regardless of previous state.
> But above is true assuming reinitialization is successful.
>
> In case of failure of page_pool_create() in __alloc_dma_rx_desc_resources(),
> all non-NULL pages will be freed, including those already cleared.
> So there is possible kernel panic due to wrong paging request at address.
>
> Fix this by assigning NULL to page_pool pointer on free
>
> Fixes: da5ec7f22a0f1 ("net: stmmac: refactor stmmac_init_rx_buffers for stmmac_reinit_rx_buffers")
> Signed-off-by: Yashwant Varur <yashwant.v@samsung.com>
> Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3591755ea30b..4b8f2814d3b5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2172,8 +2172,10 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
> xdp_rxq_info_unreg(&rx_q->xdp_rxq);
>
> kfree(rx_q->buf_pool);
> - if (rx_q->page_pool)
> + if (rx_q->page_pool) {
> page_pool_destroy(rx_q->page_pool);
> + rx_q->page_pool = NULL;
> + }
page_pool_destroy can be passed NULL, as you're fixing this I think you might as well
drop the if(rx_q->page_pool) at the same time :) I agree with the change otherwise.
with that changed,
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Maxime
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources
2026-06-30 10:09 ` [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources Jakub Raczynski
2026-07-02 9:24 ` Maxime Chevallier
@ 2026-07-03 7:26 ` Paolo Abeni
2026-07-03 8:43 ` Jakub Raczynski
1 sibling, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2026-07-03 7:26 UTC (permalink / raw)
To: j.raczynski
Cc: netdev, andrew+netdev, davem, edumazet, kuba, pabeni,
mcoquelin.stm32, linux-kernel, k.tegowski, k.domagalski,
yashwant.v
From: AI Reviewer <ai@example.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources
This patch makes __free_dma_rx_desc_resources() clear rx_q->page_pool
after destroying it, to avoid a kernel panic if a subsequent
reinitialization triggers another teardown while page_pool_create()
fails in __alloc_dma_rx_desc_resources().
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2a0d7eff88d3..6cc5a3496f39 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2172,8 +2172,10 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
> xdp_rxq_info_unreg(&rx_q->xdp_rxq);
>
> kfree(rx_q->buf_pool);
> - if (rx_q->page_pool)
> + if (rx_q->page_pool) {
> page_pool_destroy(rx_q->page_pool);
> + rx_q->page_pool = NULL;
> + }
> }
[High]
Does this fix leave the other resources in __free_dma_rx_desc_resources()
exposed to the same crash class the commit message describes?
The same teardown function also frees rx_q->buf_pool, rx_q->dma_rx (or
rx_q->dma_erx), and uses rx_q->dma_rx_phy without ever clearing any of
those pointers:
static void __free_dma_rx_desc_resources(...)
{
...
if (rx_q->xsk_pool)
dma_free_rx_xskbufs(priv, dma_conf, queue);
else
dma_free_rx_skbufs(priv, dma_conf, queue);
...
if (priv->extend_desc)
addr = rx_q->dma_erx;
else
addr = rx_q->dma_rx;
size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
...
kfree(rx_q->buf_pool);
if (rx_q->page_pool) {
page_pool_destroy(rx_q->page_pool);
rx_q->page_pool = NULL;
}
}
Consider the same failure scenario the commit message names (a
page_pool_create() failure during reinitialization), but applied to a
later queue.
After ifdown, __free_dma_rx_desc_resources() has run on every queue and
buf_pool, dma_rx/dma_erx, and dma_rx_phy are all stale dangling values
on every queue.
On ifup, __alloc_dma_rx_desc_resources() begins assigning fresh pointers
queue by queue:
rx_q->page_pool = page_pool_create(&pp_params);
if (IS_ERR(rx_q->page_pool)) {
ret = PTR_ERR(rx_q->page_pool);
rx_q->page_pool = NULL;
return ret;
}
rx_q->buf_pool = kzalloc_objs(*rx_q->buf_pool, ...);
...
addr = dma_alloc_coherent(priv->device, size, &rx_q->dma_rx_phy, ...);
...
If page_pool_create() fails on queue Q, the rollback in
alloc_dma_rx_desc_resources():
err_dma:
free_dma_rx_desc_resources(priv, dma_conf);
walks every queue including Q+1..N-1, where buf_pool, dma_rx/dma_erx and
dma_rx_phy still hold the stale values left behind by the prior
__free_dma_rx_desc_resources() call.
Does __free_dma_rx_desc_resources() on those queues then:
- read rx_q->buf_pool[i] in dma_free_rx_skbufs() ->
stmmac_free_rx_buffer() from already-freed slab memory:
static void stmmac_free_rx_buffer(...)
{
struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
if (buf->page)
page_pool_put_full_page(rx_q->page_pool, buf->page, false);
...
}
- call dma_free_coherent() a second time on the stale
rx_q->dma_rx/dma_erx and rx_q->dma_rx_phy?
- call kfree() a second time on the stale rx_q->buf_pool?
Would also nulling rx_q->buf_pool, rx_q->dma_rx, rx_q->dma_erx, and
rx_q->dma_rx_phy in __free_dma_rx_desc_resources() (or having the alloc
path defensively reset them before any early return) be appropriate to
cover the same failure mode the commit message describes?
--
This is an AI-generated review.
^ permalink raw reply [flat|nested] 5+ messages in thread