Re: [PATCH 2/3] bpf: arena: use page_ref_count() instead of page_mapped() in arena_free_pages()

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Replacing `page_mapped()` with `page_ref_count() > 1` removes the synchronous mapcount check, allowing the lockless `zap_pages()` page table walk to execute concurrently with `free_pgtables()` during `munmap()`, leading to a page table use-after-free.
- [Medium] The patch incorrectly uses a lifetime counter (`page_ref_count`) as a semantic indicator to determine if a page is mapped in user space, violating MM guidelines.
--

commit 7d590c0af75e613441c8863162be310388ecc992
Author: David Hildenbrand (Arm) <david@kernel.org>

bpf: arena: use page_ref_count() instead of page_mapped() in arena_free_pages()

This commit changes the heuristic in arena_free_pages() to use
page_ref_count(page) > 1 instead of page_mapped() to determine whether
to call zap_pages(). This avoids mapcount usage as pages will not have
a mapcount in the future.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 802656c6fd3c..608c55c260bc 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c

[ ... ]

> @@ -729,7 +729,7 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
>  
>  	llist_for_each_safe(pos, t, __llist_del_all(&free_pages)) {
>  		page = llist_entry(pos, struct page, pcp_llist);
> -		if (page_cnt == 1 && page_mapped(page)) /* mapped by some user process */
> +		if (page_cnt == 1 && page_ref_count(page) > 1) /* maybe mapped by user space */

Can this lead to a page table use-after-free during concurrent munmap()?

zap_pages() iterates over arena->vma_list and walks user page tables locklessly.
During a concurrent munmap(), unmap_region() executes unmap_vmas() (which drops
mapcounts), free_pgtables() (which unlinks and frees page tables), and finally
tlb_finish_mmu() (which drops the batched page refcounts via mmu_gather).

The VMA is only removed from arena->vma_list later during arena_vm_close(),
meaning it is still visible to zap_pages() during free_pgtables().

Since mmu_gather retains the page reference until tlb_finish_mmu(),
page_ref_count(page) > 1 will remain true during free_pgtables().

With the old page_mapped() check, it would correctly evaluate to false because
unmap_vmas() had already dropped the mapcount to 0. It appears this change
could allow zap_pages() to walk page tables while they are actively being freed.
This could also be forced by artificially inflating the refcount using GUP pins.

Also, does this use of page_ref_count() conflict with the MM guideline that
refcount should be treated as a lifetime counter rather than a semantic
indicator? 

Speculative references (like GUP pins or page_idle) can transiently inflate
the refcount independently of user space mappings.

>  			/* Optimization for the common case of page_cnt==1:
>  			 * If page wasn't mapped into some user vma there
>  			 * is no need to call zap_pages which is slow. When

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260427-page_mapped-v1-0-e89c3592c74c@kernel.org?part=2