Re: [PATCH bpf-next 2/3] bpf: arena: allocate the fault-in page outside the lock

On 7/28/26 9:22 AM, Emil Tsalapatis wrote:
> On Mon Jul 27, 2026 at 2:24 AM EDT, Jiayuan Chen wrote:
>> arena_vm_fault() allocated the page while holding arena->spinlock, so it
>> could only use the non-blocking allocator. Once the memcg is at
>> memory.max that allocation just fails, the fault turns into
>> VM_FAULT_SIGSEGV, and the process gets a SIGSEGV on a perfectly valid
>> arena address. Hitting memory.max is routine (e.g. page cache from
>> reading a big file), so this kills innocent processes.
>>
>> Preallocate the page before taking the lock, like do_anonymous_page()
>> does, so the allocation can sleep and go through reclaim and the OOM
>> path, and return VM_FAULT_OOM on failure so the memcg OOM handler runs
>> instead of a fake segfault. Also tidy up the error labels.
>>
>> Signed-off-by: Jiayuan Chen <[email protected]>
>> ---
>>   kernel/bpf/arena.c | 83 +++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 63 insertions(+), 20 deletions(-)
>>
>> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
>> index 34f023a537fe..22a41e3c53b8 100644
>> --- a/kernel/bpf/arena.c
>> +++ b/kernel/bpf/arena.c
>> @@ -481,7 +481,8 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>>   	struct bpf_map *map = vmf->vma->vm_file->private_data;
>>   	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
>>   	struct mem_cgroup *new_memcg, *old_memcg;
>> -	struct page *page;
>> +	struct page *page, *new_page = NULL;
>> +	vm_fault_t fault_ret;
>>   	long kbase, kaddr;
>>   	unsigned long flags;
>>   	int ret;
>> @@ -489,55 +490,97 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>>   	kbase = bpf_arena_get_kern_vm_start(arena);
>>   	kaddr = kbase + (u32)(vmf->address);
>>   
>> -	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags))
>> +	page = vmalloc_to_page((void *)kaddr);
>> +	if (!page) {
>> +		/*
>> +		 * Preallocate outside the lock so the allocation can sleep and go
>> +		 * through reclaim (both memcg and global), the way do_anonymous_page()
>> +		 * does. Under arena->spinlock only the non-blocking allocator is
>> +		 * available, which never reclaims.
>> +		 *
> Can you add here that we're in process context which is why we make an
> explicitly sleepable call? That also strengthens the explanation of why
> we want to preallocate in the first place.
>
>> +		 * This has to be the sleepable variant: VM_FAULT_OOM below is only
>> +		 * meaningful if the OOM machinery was actually engaged. A failure
>> +		 * from the non-blocking allocator engages nothing, so the fault
>> +		 * would be retried forever.
>> +		 */
>> +		bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
>> +		new_page = bpf_map_alloc_page_sleepable(map, NUMA_NO_NODE);
>> +		bpf_map_memcg_exit(old_memcg, new_memcg);
>> +		if (!new_page)
>> +			return VM_FAULT_OOM;
>> +	}
>> +
>> +	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {
>>   		/* Make a reasonable effort to address impossible case */
>> -		return VM_FAULT_RETRY;
>> +		fault_ret = VM_FAULT_RETRY;
>> +		goto out_err;
>> +	}
>>   
>>   	page = vmalloc_to_page((void *)kaddr);
>>   	if (page) {
>> -		if (page == arena->scratch_page)
>> +		if (page == arena->scratch_page) {
>>   			/* BPF triggered scratch here; don't lazy-alloc over it */
>> -			goto out_sigsegv;
>> +			fault_ret = VM_FAULT_SIGSEGV;
>> +			goto out_err_locked;
>> +		}
>>   		/* already have a page vmap-ed */
>>   		goto out;
>>   	}
>>   
>> +	/*
>> +	 * The lockless probe was racy: it saw a page, so nothing was
>> +	 * preallocated, but the re-check under the lock finds it gone - a
>> +	 * concurrent free must have run in between. There is nothing to
>> +	 * install and we cannot allocate under the lock, so retry the fault
>> +	 * and preallocate next time.
>> +	 */
>> +	if (!new_page) {
>> +		fault_ret = VM_FAULT_RETRY;
>> +		goto out_err_locked;
>> +	}
>> +
>>   	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
>>   
>> -	if (arena->map.map_flags & BPF_F_SEGV_ON_FAULT)
>> +	if (arena->map.map_flags & BPF_F_SEGV_ON_FAULT) {
>>   		/* User space requested to segfault when page is not allocated by bpf prog */
>> -		goto out_sigsegv_memcg;
>> +		fault_ret = VM_FAULT_SIGSEGV;
>> +		goto out_err_locked_memcg;
>> +	}
> Let's hoist this further up, I don't see why we're waiting to get so
> much into handling the fault before checking if we're even allowed to
> do it in the first place.
>
>>   
>>   	ret = range_tree_clear(&arena->rt, vmf->pgoff, 1);
>> -	if (ret)
>> -		goto out_sigsegv_memcg;
>> -
>> -	struct apply_range_data data = { .arena = arena, .pages = &page, .i = 0 };
>> -	/* Account into memcg of the process that created bpf_arena */
>> -	ret = bpf_map_alloc_pages(map, NUMA_NO_NODE, 1, &page);
>>   	if (ret) {
>> -		range_tree_set(&arena->rt, vmf->pgoff, 1);
>> -		goto out_sigsegv_memcg;
>> +		fault_ret = VM_FAULT_OOM;
> Can you add a comment that VM_FAULT_OOM is because the only way clearing
> the tree can fail is if there is a failed range allocation?
>
>> +		goto out_err_locked_memcg;
>>   	}
>> +	struct apply_range_data data = { .arena = arena, .pages = &new_page, .i = 0 };
>>   
>>   	ret = apply_to_page_range(&init_mm, kaddr, PAGE_SIZE, apply_range_set_cb, &data);
>>   	if (ret) {
>>   		range_tree_set(&arena->rt, vmf->pgoff, 1);
>> -		free_pages_nolock(page, 0);
>> -		goto out_sigsegv_memcg;
>> +		fault_ret = VM_FAULT_SIGSEGV;
>> +		goto out_err_locked_memcg;
>>   	}
>>   	flush_vmap_cache(kaddr, PAGE_SIZE);
>>   	bpf_map_memcg_exit(old_memcg, new_memcg);
>> +	/* new_page was consumed */
>> +	page = new_page;
>> +	new_page = NULL;
>>   out:
>>   	page_ref_add(page, 1);
>>   	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>> +	if (new_page)
>> +		free_pages_nolock(new_page, 0);
>>   	vmf->page = page;
>>   	return 0;
>> -out_sigsegv_memcg:
>> +
>> +out_err_locked_memcg:
>>   	bpf_map_memcg_exit(old_memcg, new_memcg);
>> -out_sigsegv:
>> +out_err_locked:
>>   	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>> -	return VM_FAULT_SIGSEGV;
>> +out_err:
>> +	if (new_page)
>> +		free_pages_nolock(new_page, 0);
>> +	return fault_ret;
> The out_err label is used only once. Can we replace the label with
> the code itself, even if this means some mild duplication? Four
> different labels are too many imo.
>
>>   }
>>   
>>   static const struct vm_operations_struct arena_vm_ops = {
> pw-bot: cr



Sorry for the delay. I missed this mail earlier...

Everything makes sense.