[PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[Jakub Raczynski]

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;
+	}
 }
 
 static void free_dma_rx_desc_resources(struct stmmac_priv *priv,
-- 
2.34.1

Re: [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[Maxime Chevallier]

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

Re: [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[Jakub Raczynski]

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

On Thu, Jul 02, 2026 at 11:24:41AM +0200, Maxime Chevallier wrote:
> >  
> >  	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
>

Agreed, will send V2 soon.

BR
Jakub Raczynski 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[Paolo Abeni]

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.

Re: [PATCH net] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[Jakub Raczynski]

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

For starters I would like to comment that I am sending this fix because this
actually happened in our environment, so this is not some fake AI report,
which I would credit with Reported-by if it was.
Sadly, no panic logs available since it happened long ago and this fix has
been for a while in our code. 

Comments for AI review below

On Fri, Jul 03, 2026 at 09:26:51AM +0200, Paolo Abeni wrote:
> From: AI Reviewer <ai@example.com>
> 
>   - 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);
>             ...
>         }

Comment invalid, I love how AI ignored literally next line, a bit more code:

>> static void stmmac_free_rx_buffer(struct stmmac_priv *priv,
>>				  struct stmmac_rx_queue *rx_q,
>>				  int i)
>>{
>>	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);
>>	buf->page = NULL;

> 
>   - call dma_free_coherent() a second time on the stale
>     rx_q->dma_rx/dma_erx and rx_q->dma_rx_phy?

That is completely separate issue and fix is not what AI suggets.
Because if we zero rx_q->dma_erx or rx_q->dma_rx, dma_free_coherent() will run
regardless on NULL pointer then. We probably should lock dma_free_coherent()
behind simple NULL check and zero it, but this will be separate patch if so.

> 
>   - call kfree() a second time on the stale rx_q->buf_pool?

Invalid, kfree() does have 'if (unlikely(ZERO_OR_NULL_PTR(object)))' check

> 
> 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?

"Would this be appropriate to cover same failures in same commit"
- Nah
"[...] in same patchset
- Maybe

BR
Jakub Raczynski

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]