Re: [PATCH bpf-next v1 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 2 potential issue(s) to consider:

New issues:
- [Low] The newly introduced multi-line comment in `arena_free_worker` does not follow the required BPF subsystem comment style (the opening `/*` is not on its own line).

Pre-existing issues:
- [High] TLB flush race in BPF arena free path allows BPF data corruption and leaking data to the page allocator.
--

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

bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker

This commit updates arena_free_pages() and arena_free_worker() to handle
range_tree_set() errors. In arena_free_pages(), it aborts the free on
error, and in arena_free_worker(), it moves range_tree_set() before PTE
clearing.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 555ee2531ef98..30745da9277b0 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 updating the free range tree under
arena->spinlock and then dropping the lock before flushing the TLB create a
race condition?

In both arena_free_pages() and arena_free_worker(), the spinlock is dropped
before flush_tlb_kernel_range() is called. 

If another thread concurrently allocates this same virtual address, maps new
physical pages into the PTE, and returns to the BPF program, could the new
program write to the old physical page due to stale TLB entries on that CPU?

When the first thread resumes, it would execute the TLB flush and free the
old physical page to the buddy allocator, which might cause data corruption
or leak data to the page allocator.

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

[ ... ]

> @@ -1004,11 +1009,17 @@ 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.
> +		 */

[Severity: Low]
Does this multi-line comment follow the BPF subsystem style? 

The BPF subsystem guidelines require the opening /* to be on its own line:

/*
 * Mark the range as free before clearing PTEs so that
 ...

> +		if (range_tree_set(&arena->rt, pgoff, page_cnt))
> +			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);
>  	}
>  	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>

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