Re: [PATCH bpf-next v2 2/4] bpf: arena: use kmalloc_nolock() in place of kvcalloc()

[Puranjay Mohan <puranjay@kernel.org>]

bot+bpf-ci@kernel.org writes:

>> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
>> index 48b8ffba3..7fa6e40ab 100644
>> --- a/kernel/bpf/arena.c
>> +++ b/kernel/bpf/arena.c
>
> [ ... ]
>
>> @@ -528,32 +537,51 @@ static long arena_alloc_pages(struct bpf_arena *arena, long uaddr, long page_cnt
>>  	if (ret)
>>  		goto out_free_pages;
>>
>> -	struct apply_range_data data = { .pages = pages, .i = 0 };
>> -	ret = bpf_map_alloc_pages(&arena->map, node_id, page_cnt, pages);
>> -	if (ret)
>> -		goto out;
>> -
>> +	remaining = page_cnt;
>>  	uaddr32 = (u32)(arena->user_vm_start + pgoff * PAGE_SIZE);
>> -	/* Earlier checks made sure that uaddr32 + page_cnt * PAGE_SIZE - 1
>> -	 * will not overflow 32-bit. Lower 32-bit need to represent
>> -	 * contiguous user address range.
>> -	 * Map these pages at kern_vm_start base.
>> -	 * kern_vm_start + uaddr32 + page_cnt * PAGE_SIZE - 1 can overflow
>> -	 * lower 32-bit and it's ok.
>> -	 */
>> -	ret = apply_to_page_range(&init_mm, kern_vm_start + uaddr32,
>> -				  page_cnt << PAGE_SHIFT, apply_range_set_cb, &data);
>> -	if (ret) {
>> -		for (i = 0; i < page_cnt; i++)
>> -			__free_page(pages[i]);
>> -		goto out;
>> +
>> +	while(remaining) {
>> +		long this_batch = min(remaining, alloc_pages);
>> +		/* zeroing is needed, since alloc_pages_bulk() only fills in non-zero entries */
>> +		memset(pages, 0, this_batch * sizeof(struct page *));
>> +		data.i = 0;
>> +
>> +		ret = bpf_map_alloc_pages(&arena->map, node_id, this_batch, pages);
>> +		if (ret)
>> +			goto out;
>> +
>> +		/* Earlier checks made sure that uaddr32 + page_cnt * PAGE_SIZE - 1
>> +		 * will not overflow 32-bit. Lower 32-bit need to represent
>> +		 * contiguous user address range.
>> +		 * Map these pages at kern_vm_start base.
>> +		 * kern_vm_start + uaddr32 + page_cnt * PAGE_SIZE - 1 can overflow
>> +		 * lower 32-bit and it's ok.
>> +		 */
>> +		ret = apply_to_page_range(&init_mm,
>> +					  kern_vm_start + uaddr32 + (mapped << PAGE_SHIFT),
>> +					  this_batch << PAGE_SHIFT, apply_range_set_cb, &data);
>> +		if (ret) {
>> +			/* data.i pages were mapped, account them and free the remaining */
>> +			mapped += data.i;
>> +			for (i = data.i; i < this_batch; i++)
>> +				__free_page(pages[i]);
>> +			goto out;
>> +		}
>> +
>> +		mapped += this_batch;
>> +		remaining -= this_batch;
>>  	}
>> -	kvfree(pages);
>> +	mutex_unlock(&arena->lock);
>> +	kfree_nolock(pages);
>>  	return clear_lo32(arena->user_vm_start) + uaddr32;
>>  out:
>> -	range_tree_set(&arena->rt, pgoff, page_cnt);
>> +	range_tree_set(&arena->rt, pgoff + mapped, page_cnt - mapped);
>> +	mutex_unlock(&arena->lock);
>> +	if (mapped)
>> +		arena_free_pages(arena, clear_lo32(arena->user_vm_start) + uaddr32, mapped);
>                                       ^^^^
>
> Can arena_free_pages() be called here while holding arena->lock?
> arena_free_pages() at line 618 acquires arena->lock using guard(mutex),
> which will deadlock since the lock was already acquired at line 525 in
> arena_alloc_pages().

arena_free_pages() will not be called with arena->lock taken, there is
mutex_unlock() before calling arena_free_pages()

>> +out_free_pages:
>> +	mutex_unlock(&arena->lock);
>              ^^^^
>
> Does this code unlock arena->lock twice in the error path? When the
> "goto out" executes from the while loop, the mutex is unlocked at the
> "out:" label above, then falls through to "out_free_pages:" and unlocks
> again here.

This is fixed by the next patch by adding another label, but I missed it
here.
Will fix it in the next version.

>> +	kfree_nolock(pages);
>>  	return 0;
>>  }
>
>
> ---
> 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