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

[Mina Almasry]

On Wed, Jul 15, 2026 at 5:42 AM Jakub Raczynski <j.raczynski@xxxxxxxxxxx> 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.
> Also remove NULL check as page_pool_destroy() does check for NULL param.
>
> Fixes: da5ec7f22a0f1 ("net: stmmac: refactor stmmac_init_rx_buffers for stmmac_reinit_rx_buffers")
> Signed-off-by: Yashwant Varur <yashwant.v@xxxxxxxxxxx>
> Signed-off-by: Jakub Raczynski <j.raczynski@xxxxxxxxxxx>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2a0d7eff88d3..3098971e0b66 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2172,8 +2172,8 @@ 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)
> -               page_pool_destroy(rx_q->page_pool);
> +       page_pool_destroy(rx_q->page_pool);
> +       rx_q->page_pool = NULL;

I wonder if it's reasonable to add pool = NULL; to the end of
page_pool_destroy. I don't know that any caller wants to ever retain
the pool pointer after that function returns, and grepping this
function I see almost every call site nulling after destroy anyway.
But anyway:

Reviewed-by: Mina Almasry <almasrymina@xxxxxxxxxx>

-- 
Thanks,
Mina