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

[Jijie Shao]

on 2026/7/27 21:26, 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:
>
> 1. Cache dma_addr to a local variable before xa_cmpxchg() in
>     __page_pool_release_netmem_dma(), so the scrub winner uses the
>     cached address for dma_unmap and never touches netmem fields after
>     the cmpxchg.
>
> 2. Rename page_pool_release_dma_index() to page_pool_remove_dma_mapping()
>     and strip it down to a pure cmpxchg wrapper; it no longer clears DMA
>     index bits.
>
> 3. Move dma_addr and DMA index cleanup to page_pool_return_netmem(),
>     which holds a page ref and can safely write netmem fields regardless
>     of the cmpxchg outcome.
>
> The scrub path (no ref) only does cmpxchg + dma_unmap on the cached
> address; the unref path (holds ref) clears dma_addr and DMA index bits
> unconditionally before put_page().
>
> Suggested-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> Fixes: ee62ce7a1d90 ("page_pool: Track DMA-mapped pages and unmap them when destroying the pool")
> Assisted-by: OhMyOpenCode:GLM-5.2
> Signed-off-by: Jijie Shao <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 | 33 +++++++++++++++++++--------------
>   1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 21dc4a9c8714..a0bac2bfde74 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -500,8 +500,8 @@ 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 int page_pool_remove_dma_mapping(struct page_pool *pool,
> +					netmem_ref netmem)
>   {
>   	struct page *old, *page = netmem_to_page(netmem);
>   	unsigned long id;
> @@ -517,12 +517,7 @@ static int page_pool_release_dma_index(struct page_pool *pool,
>   		old = xa_cmpxchg(&pool->dma_mapped, id, page, NULL, 0);
>   	else
>   		old = xa_cmpxchg_bh(&pool->dma_mapped, id, page, NULL, 0);
> -	if (old != page)
> -		return -1;
> -
> -	netmem_set_dma_index(netmem, 0);
> -
> -	return 0;
> +	return (old == page) ? 0 : -1;
>   }
>   
>   static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t gfp)
> @@ -736,16 +731,19 @@ static __always_inline void __page_pool_release_netmem_dma(struct page_pool *poo
>   		 */
>   		return;
>   
> -	if (page_pool_release_dma_index(pool, netmem))
> -		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 (page_pool_remove_dma_mapping(pool, netmem))
> +		return;
> +
>   	/* When page is unmapped, it cannot be returned to our pool */
>   	dma_unmap_page_attrs(pool->p.dev, dma,
>   			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
>   			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
> -	page_pool_set_dma_addr_netmem(netmem, 0);
>   }
>   
>   /* Disconnects a page (from a page_pool).  API users can have a need
> @@ -759,10 +757,17 @@ static void page_pool_return_netmem(struct page_pool *pool, netmem_ref netmem)
>   	bool put;
>   
>   	put = true;
> -	if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
> +	if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops) {
>   		put = pool->mp_ops->release_netmem(pool, netmem);
> -	else
> +	} else {
>   		__page_pool_release_netmem_dma(pool, netmem);
> +		/* clear dma_addr/DMA index; safe because we hold a ref */
> +		if (pool->dma_map) {
> +			page_pool_set_dma_addr_netmem(netmem, 0);
> +			if (unlikely(PP_DMA_INDEX_BITS))
> +				netmem_set_dma_index(netmem, 0);

the unlikely() on the PP_DMA_INDEX_BITS check should be likely() — BITS > 0 is the common case. My mistake, will fix in v3.

> +		}
> +	}
>   
>   	/* This may be the last page returned, releasing the pool, so
>   	 * it is not safe to reference pool afterwards.
>
> base-commit: 04026c998c24ac47eb76886b9790c5710b603eb4