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

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Mon Aug 10, 2026 at 10:08 AM EDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Physical memory leak in the BPF arena allocation rollback and deferred free paths when `range_tree_set()` fails.
> --
>
> --- Patch [3]: [PATCH bpf-next v3 3/3] bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker ---
> commit 2ed41c8c8f7aaa211a3d44f83cea88768b2f5819
> Author: Yuan Chen <chenyuan@kylinos.cn>
>
> bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker
>
> This commit adds error checking for range_tree_set() in the BPF arena
> allocation rollback and free paths. If the tree update fails, it aborts
> the page free operation or skips clearing PTEs to prevent untracked pages.
>
>> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
>> index 7b6847200b431..1e519c3c229ca 100644
>> --- a/kernel/bpf/arena.c
>> +++ b/kernel/bpf/arena.c
>
> [ ... ]
>
>> @@ -881,7 +883,12 @@ 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) {
>> +		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>> +		bpf_map_memcg_exit(old_memcg, new_memcg);
>> +		return;
>> +	}
>
> [Severity: High]
> Does this early return cause a physical memory leak bounded by the lifetime
> of the BPF map?
>
> If arena_alloc_pages() fails midway due to memory limits, it attempts to
> roll back the partially allocated physical pages by calling
> arena_free_pages(). Under memory pressure, the range_tree_set() call is
> highly likely to fail due to its internal kmalloc_nolock() allocation.
>
> Aborting here leaves the physical pages mapped in the kernel VMA and
> completely orphaned. Exchanging a virtual address space leak for a physical
> memory leak on an error path under OOM conditions worsens memory pressure.
>
> The pre-patch behavior of freeing pages and clearing PTEs appeared safe,
> since bpf_arena_fault_handler() handles unmapped PTEs by mapping the
> scratch_page.
>

Imo this is debatable. If we are at the point where we can't allocate
entries in the range tree, we are close to an OOM anyway and so it is
better to keep the range tree and actual mappings consistent.

>>  	init_llist_head(&free_pages);
>>  	cdata.arena = arena;
>
> [ ... ]
>
>> @@ -1004,11 +1011,18 @@ static void arena_free_worker(struct work_struct *work)
>>  		kaddr = arena_vm_start + s->uaddr;
>>  		pgoff = compute_pgoff(arena, s->uaddr);
>>  
>> +		/*
>> +		 * Mark the range as free before clearing PTEs so that
>> +		 * if the range tree update fails we leave the PTEs
>> +		 * intact, avoiding an unrecoverable state where pages
>> +		 * are freed but the arena free tree does not track them.
>> +		 */
>> +		if (range_tree_set(&arena->rt, pgoff, page_cnt))
>> +			continue;
>> +
>
> [Severity: High]
> Can this continue statement permanently leak the physical pages from the
> free system?
>
> When this skips PTE clearing and page collection, the subsequent loop in
> arena_free_worker():
>
> arena_free_worker() {
>     ...
>     llist_for_each_safe(pos, t, list) {
>         s = llist_entry(pos, struct arena_free_span, node);
>         ...
>         kfree_nolock(s);
>     }
>     ...
> }
>
> unconditionally destroys the tracking span 's'. This abandons the pages,
> leaving them orphaned rather than avoiding an unrecoverable state.


This, however, seems valid. You can make it so that we do not free
ranges for which we haven't called apply_range_clear_cb.

pw-bot: cr

>
>>  		/* 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);
>>  	}
>>  	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);