Re: [PATCH bpf-next v4 3/3] bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker

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

New issues:
- [Critical] Re-arming irq_work in arena_free_worker() bypasses teardown synchronization, leading to UAF of the bpf_arena structure on map destruction.
- [High] Immediate, unconditional retry in arena_free_worker() when range_tree_set() fails creates a tight CPU busy-loop and livelock.
- [High] Aborting arena_free_pages() on range_tree_set() failure silently leaks physical memory and memcg quota during the arena's lifetime.

Pre-existing issues:
- [High] Calling apply_to_existing_page_range() while holding raw_res_spin_lock_irqsave violates lock wait-type nesting rules (LD_WAIT_SPIN > LD_WAIT_CONFIG).
--

--- Patch [3]: [PATCH bpf-next v4 3/3] bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker ---
commit 5874fa92054ab81b6a7b82ddf6fae39167bcb8d3
Author: Yuan Chen <[email protected]>
Subject: bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker

This commit handles errors from range_tree_set() in arena_free_pages() and
arena_free_worker(). It aborts the free operation on failure to avoid clearing
PTEs for untracked pages, and modifies arena_free_worker() to requeue failed
spans for later retry.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 7b6847200b431..169b2e71e9cbb 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c

[ ... ]

> @@ -881,7 +883,18 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
>  	if (ret)
>  		goto defer;
>  
> -	range_tree_set(&arena->rt, pgoff, page_cnt);
> +	ret = range_tree_set(&arena->rt, pgoff, page_cnt);
> +	if (ret) {
> +		/*
> +		 * range_tree_set() is failure-atomic, so -ENOMEM leaves the
> +		 * range allocated and the pages mapped. Abort the free rather
> +		 * than returning pages the free tree does not track; a later
> +		 * free of the same range can succeed.
> +		 */
> +		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +		bpf_map_memcg_exit(old_memcg, new_memcg);
> +		return;
> +	}

[Severity: High]
When range_tree_set() fails here, does returning early permanently leak
physical pages and memcg quota if this is called from the unwind path in
arena_alloc_pages()?

If arena_alloc_pages() fails partway through, it calls arena_free_pages()
to unwind. If this early return is hit, the physical pages remain mapped and
allocated. Since the original allocation returns 0, the BPF program cannot
free these pages later.

>  
>  	init_llist_head(&free_pages);
>  	cdata.arena = arena;

[ ... ]

> @@ -992,28 +1006,43 @@ 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)) {
> +			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);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
apply_to_existing_page_range() here violate lock wait-type nesting rules?

This code executes while holding the raw_res_spin_lock_irqsave() raw
spinlock. apply_to_existing_page_range() walks page tables and acquires the
page table lock (PTL). The PTL is a spinlock_t, which maps to a sleepable
rt_mutex on PREEMPT_RT kernels. Acquiring a sleepable lock inside a raw lock
can cause a scheduling while atomic bug.

> -
> -		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);

[Severity: High]
Can this unconditional retry create a tight CPU busy-loop and livelock?

If range_tree_set() fails (e.g., from -ENOMEM due to memory cgroup limits),
it sets retry = true and queues arena->free_irq without any backoff. This
immediately reschedules the worker. Since no memory was freed, it seems it
will likely hit -ENOMEM again indefinitely.

[Severity: Critical]
Does re-arming irq_work here bypass teardown synchronization and lead to a
use-after-free of the bpf_arena structure?

During map destruction, arena_map_free() calls flush_work(&arena->free_work)
to wait for the worker to finish. However, flush_work() does not wait for
newly queued irq_work. 

If arena_map_free() proceeds to free the arena, the pending IPI might
execute arena_free_irq() and schedule_work(), dereferencing the freed
arena pointer.

> +
>  	/* 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;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3