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

On Wed, Aug 5, 2026 at 4:11 PM Jakub Kicinski <[email protected]> wrote:
>
> On Tue, 4 Aug 2026 20:50:46 -0700 Mina Almasry wrote:
> > > 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.
>
> from memory so likely wrong -
> ring scrub is freeing pages from the cache
> DMA unamp is clearing _all_ mappings, including actively on used pages
> the are different sets

Yes, my bad, after taking a closer look, I was completely wrong. We
can't put the dma-unmapping in page_pool_return_netmem (because that
can run after the netdev is destroyed), and yes, there is indeed a
race with the page being freed.

FWIW, Jijie this restriction—that the scrub path should not touch the
page, only dma-unmap it—is worth documenting in a comment so as not to
trip the next guy.

FWIW, I also have a rough idea of how to fix this race? I'm guessing
we could grab an extra ref on the page before putting it in the
dma_mapped xarray, and then have the winner of the xa_cmpchg be
responsible for dropping that reference. That should work I think? But
it adds overhead. I maybe wouldn't want to add the overhead unless
someone is actually running into this bug in practice. My
understanding is that this is an AI code review theoretical bug.

-- 
Thanks,
Mina