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

[Mina Almasry]

On Fri, Jul 31, 2026 at 4:15 AM Jijie Shao <shaojijie@xxxxxxxxxx> 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().
>
> 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>

Gemini provided no feedback and to my eye the code looks great too.

Reviewed-by: Mina Almasry <almasrymina@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;
>
>         id = netmem_get_dma_index(netmem);
>         if (!id)
> -               return -1;
> +               return;
>
>         if (in_softirq())
>                 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;
>
> -       return 0;
> +unmap:
> +       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);
>  }
>
>  static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t gfp)
> @@ -728,24 +739,13 @@ void page_pool_clear_pp_info(netmem_ref netmem)
>  static __always_inline void __page_pool_release_netmem_dma(struct page_pool *pool,
>                                                            netmem_ref netmem)
>  {
> -       dma_addr_t dma;
> -
>         if (!pool->dma_map)
> -               /* Always account for inflight pages, even if we didn't
> -                * map them
> -                */
>                 return;
>
> -       if (page_pool_release_dma_index(pool, netmem))
> -               return;
> -
> -       dma = page_pool_get_dma_addr_netmem(netmem);
> -
> -       /* 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_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.

-- 
Thanks,
Mina