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

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 <[email protected]>
>> Assisted-by: OhMyOpenCode:GLM-5.2
>> Signed-off-by: Jijie Shao <[email protected]>
>> ---
>> 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/[email protected]
>>
>> Changes in v3:
>> - Fix unlikely() to likely() for PP_DMA_INDEX_BITS to match
>>    file convention.
>> - Link to v2: https://lore.kernel.org/r/[email protected]
>>
>> 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/[email protected]
>> ---
>>   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