Re: [PATCH bpf-next v3 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 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 <[email protected]>

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.

>  	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.

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

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