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

[Jakub Raczynski <j.raczynski@samsung.com>]

[-- 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 --]