Re: [PATCH net] net: page_pool: fix DMA index not cleared on xa_cmpxchg race

[Mina Almasry]

On Fri, Jul 24, 2026 at 2:22 AM Jijie Shao <shaojijie@xxxxxxxxxx> wrote:
>
> page_pool_release_dma_index() uses xa_cmpxchg() to atomically remove a
> page from pool->dma_mapped.  When the cmpxchg loses the race (the entry
> was already removed by a concurrent release path, e.g. page_pool_scrub()
> during page_pool_destroy()), the function returns -1 *without* clearing
> the DMA index bits stored in pp_magic.  Its caller then skips dma_unmap
> based on that -1, which is correct, but the stale DMA index bits left
> in pp_magic are wrong: the xarray entry they index no longer exists.
> A page recycled back into the pool then carries a dangling DMA index,
> and kernels that validate pp_magic in the return path surface this as
> a WARN.
>
> Move netmem_set_dma_index(netmem, 0) to execute unconditionally before
> return, so both the winner and the loser of the xa_cmpxchg race clear
> the DMA index bits.  The return value still distinguishes the two
> cases (-1 loser / 0 winner), preserving the contract that exactly one
> side performs dma_unmap.
>
> The race window opens when page_pool_destroy() runs concurrently with
> late page returns from SKBs held in per-CPU defer lists, TCP receive
> queues or GRO hashes -- e.g. a driver reconfiguring channels/ring
> depth while traffic is flowing.
>
> Observed on arm64 (7.2.0-rc1) with the hns3 driver:
>
>   WARNING: net/core/netmem_priv.h:18 at page_pool_clear_pp_info+0x20/0x38,
>            CPU#83: iperf/3874878
>
> Return path (CPU_B, process context, triggering the WARN).  The page
> was held in a per-CPU SKB defer list and is being released through
> tcp_recvmsg():
>
>   page_pool_clear_pp_info+0x20/0x38
>   page_pool_put_unrefed_netmem+0x11c/0x2e8
>   napi_pp_put_page+0xf0/0x120
>   skb_release_data+0x170/0x228
>   skb_attempt_defer_free+0x7c/0x1f0
>   tcp_recvmsg_locked+0x710/0x9a0
>   tcp_recvmsg+0x74/0x1c8
>   inet_recvmsg+0x2c/0xf0
>   __sys_recvfrom+0xdc/0x198
>   __arm64_sys_recvfrom+0x2c/0x48
>   invoke_syscall+0x5c/0x120
>   el0_svc_common.constprop.0+0xc8/0xf0
>   do_el0_svc+0x24/0x38
>   el0_svc+0x34/0x1e0
>   el0t_64_sync_handler+0xa0/0xe8
>   el0t_64_sync+0x1ac/0x1b0
>
> Scrub path (CPU_A, racing with the return path, derived from code):
>
>   page_pool_destroy
>   -> page_pool_scrub
>      -> xa_for_each(dma_mapped)
>         -> __page_pool_release_netmem_dma
>            -> page_pool_release_dma_index   <- race window
>
> Debug approach and evidence:
> Reproduced with a stability test that repeatedly reconfigures the hns3
> channel count and ring depth (ethtool -L/-G) while running iperf3 with
> multiple parallel streams, then closes the iperf3 sockets to release
> the SKBs held in TCP receive queues and per-CPU defer lists.
>
> On the unmodified kernel the WARN reproduces after hours to tens of
> hours. To make the race window observable, a debugfs knob injects a
> controlled udelay() right after the winning xa_cmpxchg() in
> page_pool_release_dma_index(), widening the gap between the xarray
> entry removal and the pp_magic cleanup.  With the delay injected the
> WARN reproduces 4-5 times within about half an hour.
>
> Instrumentation added to page_pool_release_dma_index() logs the per-
> page pp_magic, DMA index, refcount and dma_addr at release time.  A
> representative log line from a WARN-triggering page:
>
>   PP_DMA_IDX: magic=0xdead0000000cbec0 idx=6525 refcnt=1 dma_addr=0x0
>
>   magic=0xdead0000000cbec0  -> PP_SIGNATURE | (6525 << shift)
>   idx=6525                  -> DMA index bits still set in pp_magic
>   dma_addr=0x0              -> scrub already did dma_unmap + cleared addr
>   refcnt=1                  -> inflight page, unrefed release path
>
> The combination "dma_addr == 0 but DMA index != 0" is the signature of
> the race: CPU_A (scrub) won the xa_cmpxchg, performed dma_unmap and
> cleared dma_addr; CPU_B (return) lost the cmpxchg, returned -1 and
> skipped netmem_set_dma_index(0), leaving the DMA index bits stale --
> exactly the gap this fix closes.
>
> With this fix applied the WARN no longer triggers under the same
> workload (verified overnight), both with and without the debugfs
> delay knob enabled.
>
> Fixes: 95920c2ed02b ("page_pool: Fix PP_MAGIC_MASK to avoid crashing on some 32-bit arches")
> 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>

If at all possible, when generating fixes via LLMs, spend time
reviewing the patch yourself before sending to make sure the patch
makes sense. In this case, I was taken back that this 10+, 3- fix came
with such a huge huge commit message.

I did not review my self but my LLM - which does not tire in reading
the long commit message - came up with this review. It looks like it
may be correct. I think the fact that we do not touch the netmem at
all if we lose the race is intentional, because the netmem may not be
allocated:

```
  While this fixes the WARN_ON_ONCE, it introduces a critical
Use-After-Free (UAF) and memory corruption bug in the scrub path.

  page_pool_scrub() iterates over pool->dma_mapped without holding a
reference to the page. If the scrub path races with the normal unref
path and wins xa_cmpxchg(), the unref path will proceed to free the
page.

  If the scrub path is preempted immediately after xa_cmpxchg(), the
page will be freed before the scrub path can continue. When the scrub
path resumes, it will execute your new netmem_set_dma_index(netmem,
0), read the dma_addr,
  and clear it—all on a freed page that may now belong to another
subsystem. This will corrupt memory and unmap garbage DMA addresses.

  To safely fix this:

  1. __page_pool_release_netmem_dma() must cache dma_addr to a local
variable before calling xa_cmpxchg().
  2. The scrub path (winner) must not touch netmem after a successful
xa_cmpxchg().
  3. The unref path (loser), which safely holds the page reference,
should handle clearing the DMA index and dma_addr to satisfy the WARN,
regardless of the race outcome.
```

-- 
Thanks,
Mina