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

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

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.