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

[Jijie Shao]

on 2026/8/5 10:17, Jakub Kicinski wrote:
> On Fri, 31 Jul 2026 19:15:07 +0800 Jijie Shao wrote:
> > page_pool_scrub() iterates pool->dma_mapped via xa_for_each() with no
> > page ref held. __page_pool_release_netmem_dma() currently reads and
> > writes netmem fields (dma_addr, DMA index bits in pp_magic) after
> > xa_cmpxchg() returns. The unref path calls put_page() unconditionally
> > regardless of the cmpxchg outcome; when it loses the cmpxchg, it still
> > frees the page before the scrub winner finishes these netmem accesses,
> > so scrub touches a freed page -- a Use-After-Free.
> >
> > Fix this by splitting the DMA release into two functions:
> >
> > 1. __page_pool_unmap_netmem_dma() caches dma_addr before xa_cmpxchg(),
> >     does the cmpxchg to remove the DMA mapping, and calls dma_unmap on
> >     the cached address. It never touches netmem fields after the cmpxchg,
> >     making it safe for the scrub path which holds no page ref.
> >
> > 2. __page_pool_release_netmem_dma() wraps the above and additionally
> >     clears dma_addr and DMA index bits in netmem fields. This is safe
> >     only when the caller holds a page ref, so it is used by the return
> >     path (page_pool_return_netmem).
> >
> > The scrub path calls __page_pool_unmap_netmem_dma() directly; the return
> > path calls __page_pool_release_netmem_dma().
> Please clearly state what led you to discovering this bug?
> Was it directly hit in production?
> Was there a prod issue which made you investigate?
> Were you able to trigger the race and if so -- how?
>
> > Fixes: ee62ce7a1d90 ("page_pool: Track DMA-mapped pages and unmap them when destroying the pool")
> > Suggested-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> > Assisted-by: OhMyOpenCode:GLM-5.2
> > Signed-off-by: Jijie Shao <shaojijie@xxxxxxxxxx>
> > ---
> > Changes in v4:
> > - Restructure per Mina's review: merge page_pool_remove_dma_mapping()
> >    into __page_pool_unmap_netmem_dma() with dma_unmap inlined via goto
> >    label; simplify __page_pool_release_netmem_dma() to a thin wrapper.
> > - Link to v3: https://lore.kernel.org/r/20260729110249.2824835-1-shaojijie@xxxxxxxxxx
> >
> > Changes in v3:
> > - Fix unlikely() to likely() for PP_DMA_INDEX_BITS to match
> >    file convention.
> > - Link to v2: https://lore.kernel.org/r/20260727132612.3277927-1-shaojijie@xxxxxxxxxx
> >
> > Changes in v2:
> > - Redesign the fix per Mina's review: v1's unconditional
> >    netmem_set_dma_index() introduced a UAF when the scrub path
> >    (no page ref) writes to a page freed by the unref path.
> > - Cache dma_addr before xa_cmpxchg; move dma_addr/DMA index
> >    cleanup to page_pool_return_netmem() which holds a page ref.
> > - Rename page_pool_release_dma_index() to
> >    page_pool_remove_dma_mapping() to reflect its new role as a
> >    pure cmpxchg wrapper.
> > - Link to v1: https://lore.kernel.org/r/20260724092135.414699-1-shaojijie@xxxxxxxxxx
> > ---
> >   net/core/page_pool.c | 46 ++++++++++++++++++++++----------------------
> >   1 file changed, 23 insertions(+), 23 deletions(-)
> >
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 21dc4a9c8714..497bb1906fc3 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -500,29 +500,40 @@ static int page_pool_register_dma_index(struct page_pool *pool,
> >   	return err;
> >   }
> >   
> > -static int page_pool_release_dma_index(struct page_pool *pool,
> > -				       netmem_ref netmem)
> > +static void __page_pool_unmap_netmem_dma(struct page_pool *pool,
> > +					 netmem_ref netmem)
> >   {
> >   	struct page *old, *page = netmem_to_page(netmem);
> >   	unsigned long id;
> > +	dma_addr_t dma;
> > +
> > +	if (!pool->dma_map)
> > +		return;
> > +
> > +	/* Cache dma_addr before xa_cmpxchg. The scrub path holds no page ref;
> > +	 * the unref path calls put_page() regardless of cmpxchg outcome, so
> > +	 * after the cmpxchg we cannot safely touch netmem fields.
> > +	 */
> > +	dma = page_pool_get_dma_addr_netmem(netmem);
> >   
> >   	if (unlikely(!PP_DMA_INDEX_BITS))
> > -		return 0;
> > +		goto unmap;
> nit: just indent the intervening lines please.
> Don't use goto where adding a code block would do.

Hi Jakub,

Thanks for the review. v5 incoming addressing both points:

- Added bug discovery context to the commit  message.
- Replaced goto with if (likely(PP_DMA_INDEX_BITS)) block.

Jijie