Re: [PATCH bpf-next v5 3/3] bpf, arena: handle range_tree_set failures in alloc/free paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] If `arena_map_free()` fails to acquire the lock and aborts, it leaks the arena and leaves the worker in an infinite retry loop on `-ENOMEM`.
- [Low] The `if (arena->dying)` check in `arena_free_worker` is mathematically dead code.

Pre-existing issues:
- [High] Virtual memory range is marked free before TLB flush and user PTE zap, allowing use-after-free and data loss.
--

commit 9b91373ca20b3a032ab090590fde24790048b705
Author: Yuan Chen <chenyuan@kylinos.cn>
bpf, arena: handle range_tree_set failures in alloc/free paths

This commit adds error handling for range_tree_set() in the arena allocation
and free paths. It includes a bounded retry loop for arena_map_free() to
acquire the spinlock and introduces arena->dying to stop the worker thread's
retry loop during map teardown.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 7b6847200b431..b0d1f0facfb2c 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
[ ... ]
> @@ -370,6 +373,9 @@ static int existing_page_cb(pte_t *ptep, unsigned long addr, void *data)
>  static void arena_map_free(struct bpf_map *map)
>  {
>  	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +	struct llist_node *list, *pos, *t;
> +	unsigned long flags;
> +	int ret, i;
>  
>  	/*
>  	 * Check that user vma-s are not around when bpf map is freed.
> @@ -380,7 +386,40 @@ static void arena_map_free(struct bpf_map *map)
>  	if (WARN_ON_ONCE(!list_empty(&arena->vma_list)))
>  		return;
>  
> -	/* Ensure no pending deferred frees */
> +	/*
> +	 * No fallback if this fails, so retry a few times for a long but
> +	 * finite hold; -EDEADLK can't be waited out. Cap the retries:
> +	 * leaking the arena is better than hanging map free.
> +	 */
> +	for (i = 0; i < 10; i++) {
> +		ret = raw_res_spin_lock_irqsave(&arena->spinlock, flags);
> +		if (!ret || ret == -EDEADLK)
> +			break;
> +		msleep(1);
> +	}
> +	if (ret) {
> +		WARN_ONCE(1, "bpf_arena: spinlock acquire failed %d\n", ret);
> +		return;
> +	}

[Severity: High]
If this early return path triggers, it intentionally leaks the arena, but
doesn't set arena->dying. Could this strand the worker in an infinite
retry loop?

If the worker's range_tree_set() call fails continuously (e.g., due to memory
exhaustion), it will endlessly requeue the span and queue an irq_work to
retry. Because arena->dying remains false, this could turn into an infinite
busy loop.

> +	/*
> +	 * Set @dying before draining: the worker checks it under this
> +	 * spinlock before requeueing, so a failed span is either stolen
> +	 * here or dropped by the worker.
> +	 */
> +	arena->dying = true;
[ ... ]
> @@ -977,12 +1031,13 @@ static void arena_free_worker(struct work_struct *work)
>  	struct llist_node *list, *pos, *t;
>  	struct arena_free_span *s;
>  	u64 arena_vm_start, user_vm_start;
> -	struct llist_head free_pages;
> +	struct llist_head free_pages, cleared;
>  	struct clear_range_data cdata;
>  	struct page *page;
>  	unsigned long full_uaddr;
>  	long kaddr, page_cnt, pgoff;
>  	unsigned long flags;
> +	bool retry = false;
>  
>  	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {
>  		schedule_work(work);
> @@ -992,28 +1047,52 @@ static void arena_free_worker(struct work_struct *work)
>  	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
>  
>  	init_llist_head(&free_pages);
> +	init_llist_head(&cleared);
>  	cdata.arena = arena;
>  	cdata.free_pages = &free_pages;
>  	arena_vm_start = bpf_arena_get_kern_vm_start(arena);
>  	user_vm_start = bpf_arena_get_user_vm_start(arena);
>  
>  	list = llist_del_all(&arena->free_spans);
> -	llist_for_each(pos, list) {
> +	llist_for_each_safe(pos, t, list) {
>  		s = llist_entry(pos, struct arena_free_span, node);
>  		page_cnt = s->page_cnt;
>  		kaddr = arena_vm_start + s->uaddr;
>  		pgoff = compute_pgoff(arena, s->uaddr);
>  
> +		/*
> +		 * Set the range free before clearing PTEs, and requeue the
> +		 * span on failure: the PTEs stay intact and the free is
> +		 * retried later. Only spans moved to @cleared (PTE clearing
> +		 * actually ran) reach the flush/zap/release loop below.
> +		 */
> +		if (range_tree_set(&arena->rt, pgoff, page_cnt)) {
> +			if (arena->dying) {
> +				/*
> +				 * The map is being freed. PTEs stay intact
> +				 * and the pages are reclaimed by
> +				 * arena_map_free() via existing_page_cb().
> +				 */
> +				kfree_nolock(s);
> +				continue;
> +			}

[Severity: Low]
Is this check mathematically dead code?

arena->dying is set to true by arena_map_free() while holding arena->spinlock,
after which arena->free_spans is immediately drained via llist_del_all().

In arena_free_worker(), the list is also drained at the very beginning of its
arena->spinlock critical section. Because both functions drain the list under
the same spinlock, if arena_map_free() runs first, the worker's list is empty
and the loop never executes. If the worker runs first, dying is guaranteed to
be false throughout its critical section.

> +			llist_add(&s->node, &arena->free_spans);
> +			retry = true;
> +			continue;
> +		}
> +
>  		/* clear ptes and collect pages in free_pages llist */
>  		apply_to_existing_page_range(&init_mm, kaddr, page_cnt << PAGE_SHIFT,
>  					     apply_range_clear_cb, &cdata);
> -
> -		range_tree_set(&arena->rt, pgoff, page_cnt);
> +		llist_add(&s->node, &cleared);
>  	}
>  	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>  
> +	if (retry)
> +		irq_work_queue(&arena->free_irq);
> +
>  	/* Iterate the list again without holding spinlock to do the tlb flush and zap_pages */
> -	llist_for_each_safe(pos, t, list) {
> +	llist_for_each_safe(pos, t, cleared.first) {
>  		s = llist_entry(pos, struct arena_free_span, node);
>  		page_cnt = s->page_cnt;
>  		full_uaddr = clear_lo32(user_vm_start) + s->uaddr;
> +		kaddr = arena_vm_start + s->uaddr;
> +
> +		/* ensure no stale TLB entries */
> +		flush_tlb_kernel_range(kaddr, kaddr + (page_cnt * PAGE_SIZE));

[Severity: High]
This isn't a bug introduced by this patch, but dropping the spinlock before
the TLB flush opens a race window. Can a concurrent arena_alloc_pages()
allocate the newly freed virtual range and map new physical pages to it?

Since the TLB flush for the old pages has not yet executed, accesses by the
new owner could hit stale TLB entries (or user PTEs). Writes would go to the
old physical pages which are later freed, resulting in a use-after-free and
data loss. The synchronous free path in arena_free_pages() appears to have
the same preexisting vulnerability.

>  
> -		/* remove pages from user vmas */
> -		zap_pages(arena, full_uaddr, page_cnt);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902093740.2338724-1-chenyuan_fl@163.com?part=3