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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A race condition between dropping the arena spinlock and flushing the TLB allows concurrent reallocation and stale TLB access, leading to an information leak.
--

--- Patch [3]: [PATCH bpf-next v2 3/3] bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker ---
commit d52cc998404e8cd2b8a37ee307bab5c587ce8336
Author: Yuan Chen <chenyuan@kylinos.cn>

bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker

arena_free_pages() and arena_free_worker() now handle range_tree_set()
errors. arena_free_pages() aborts the free on error, and
arena_free_worker() moves range_tree_set() before PTE clearing so that a
failed tree update leaves the PTEs intact instead of freeing pages that
the arena free tree does not track.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 7b6847200b431..4597d9becb97a 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -881,7 +881,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]
This is a pre-existing issue, but does marking the range as free here,
before dropping the spinlock and flushing the TLB, create a race condition?

If CPU 1 calls arena_free_pages(), marks the pages as free via
range_tree_set(), clears the PTEs, and drops the spinlock:

        ret = range_tree_set(&arena->rt, pgoff, page_cnt);
        ...
        raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);

Before CPU 1 executes flush_tlb_kernel_range(), could CPU 2 allocate the
same virtual address via bpf_arena_alloc_pages() and populate new PTEs? 

If CPU 2 (or another CPU) previously accessed this virtual address, its
TLB might still point to the old physical page. Writes to this address
would then incorrectly modify the old physical page.

CPU 1 would then flush the TLB and free the old physical page to the buddy
allocator, potentially leaking sensitive data to other processes.

[ ... ]

> @@ -1004,11 +1009,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]
This isn't a bug introduced by this patch, but does the same race
condition exist here in the asynchronous worker?

The worker marks the range as free via range_tree_set() and drops the
spinlock:

        if (range_tree_set(&arena->rt, pgoff, page_cnt))
                continue;
        ...
        raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);

Before iterating the list again to call flush_tlb_kernel_range(), this
seems to expose the old physical pages to the same concurrent reallocation
race window as arena_free_pages().

> +
>  		/* 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/20260810094634.2723213-1-chenyuan_fl@163.com?part=3