Re: [PATCH v4 net] net: page_pool: fix UAF in __page_pool_release_netmem_dma on xa_cmpxchg race

[Mina Almasry]

On Tue, Aug 4, 2026 at 7:16 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Fri, 31 Jul 2026 10:37:58 -0700 Mina Almasry wrote:
> > > +       __page_pool_unmap_netmem_dma(pool, netmem);
> > >         page_pool_set_dma_addr_netmem(netmem, 0);
> > > +       if (likely(PP_DMA_INDEX_BITS))
> > > +               netmem_set_dma_index(netmem, 0);
> >
> > I now notice that maybe another cleanup we could have done is open
> > code __page_pool_unmap_netmem_dma() in this function to cut down 1
> > helper, and just have the scrub function call
> > __page_pool_release_netmem_dma() to reduce some code. But this is more
> > than fine too I think, especially since this is a fix the stable trees
> > are going to want I guess.
>
> Not sure this is a good idea? scrub is trying to touch just the DMA
> mapping, right? It shouldn't try to update the page itself because
> it has no reference to the page, the page may get freed in parallel.
> Hopefully DMA unmap on a freed page is legal..

Scrub actually frees the page in the loop in
page_pool_empty_ring(pool), so it's not true AFAIU that it 'shouldn't
try to update the page'.

My mental model (roughly) is that the pp has a ref and that single
last ref can be dropped in the page_pool_destroy() path or the
page_pool_put_netmem() path, but the ref can't be dropped twice and
the same goes for dma unmapping.

Now that i look closer at the code I have no idea why we didn't 'just'
put the dma-unmapping inside of page_pool_return_netmem(). That code
path is already common between scrub() and put_netmem() and does the
synchronization between these 2 paths.

When I have time I'll take a look to see if an LLM can find a better
way to do this.

--
Thanks,
Mina