Re: [PATCH bpf-next v2 1/4] bpf: arena: populate vm_area without allocating memory

[Puranjay Mohan <puranjay@kernel.org>]

bot+bpf-ci@kernel.org writes:

>> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
>> index 1074ac445..48b8ffba3 100644
>> --- a/kernel/bpf/arena.c
>> +++ b/kernel/bpf/arena.c
>
> [ ... ]
>
>> @@ -92,6 +93,62 @@ static long compute_pgoff(struct bpf_arena *arena, long uaddr)
>>  	return (u32)(uaddr - (u32)arena->user_vm_start) >> PAGE_SHIFT;
>>  }
>>
>> +struct apply_range_data {
>> +	struct page **pages;
>> +	int i;
>> +};
>> +
>> +static int apply_range_set_cb(pte_t *pte, unsigned long addr, void *data)
>> +{
>> +	struct apply_range_data *d = data;
>> +	struct page *page;
>> +
>> +	if (!data)
>> +		return 0;
>> +	/* sanity check */
>> +	if (unlikely(!pte_none(ptep_get(pte))))
>> +		return -EBUSY;
>> +
>> +	page = d->pages[d->i++];
>> +	/* paranoia, similar to vmap_pages_pte_range() */
>> +	if (WARN_ON_ONCE(!pfn_valid(page_to_pfn(page))))
>> +		return -EINVAL;
>> +
>> +	set_pte_at(&init_mm, addr, pte, mk_pte(page, PAGE_KERNEL));
>> +	return 0;
>> +}
>> +
>> +static int apply_range_clear_cb(pte_t *pte, unsigned long addr, void *data)
>> +{
>> +	pte_t old_pte;
>> +	struct page *page;
>> +
>> +	/* sanity check */
>> +	old_pte = ptep_get(pte);
>> +	if (pte_none(old_pte) || !pte_present(old_pte))
>> +		return 0; /* nothing to do */
>> +
>> +	/* get page and free it */
>> +	page = pte_page(old_pte);
>> +	if (WARN_ON_ONCE(!page))
>> +		return -EINVAL;
>> +
>> +	pte_clear(&init_mm, addr, pte);
>> +
>> +	/* ensure no stale TLB entries */
>> +	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>> +
>> +	__free_page(page);
>> +
>> +	return 0;
>> +}
>> +
>> +static int populate_pgtable_except_pte(struct bpf_arena *arena)
>> +{
>> +	return apply_to_page_range(&init_mm, bpf_arena_get_kern_vm_start(arena),
>> +				   KERN_VM_SZ - GUARD_SZ, apply_range_set_cb, NULL);
>> +}
>> +
>>  static struct bpf_map *arena_map_alloc(union bpf_attr *attr)
>>  {
>>  	struct vm_struct *kern_vm;
>
> [ ... ]
>
>> @@ -144,6 +201,11 @@ static struct bpf_map *arena_map_alloc(union bpf_attr *attr)
>>  		goto err;
>>  	}
>>  	mutex_init(&arena->lock);
>> +	err = populate_pgtable_except_pte(arena);
>> +	if (err) {
>> +		bpf_map_area_free(arena);
>> +		goto err;
>> +	}
>                      ^^^^
>
> Can this leak the range tree? Looking at the code, range_tree_set() was
> called earlier in this function and can allocate range_node structures
> via range_tree_clear()->kmalloc_nolock(). If populate_pgtable_except_pte()
> fails here, the error path calls bpf_map_area_free(arena) but doesn't call
> range_tree_destroy(&arena->rt) first. Compare with arena_map_free() which
> always calls range_tree_destroy() before freeing the arena.

As the range tree is empty at this point, we can be sure that
range_tree_clear() in range_tree_set() will not allocate anything. 

>>
>>  	return &arena->map;
>>  err:
>
> [ ... ]
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19363121319