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

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Sat Aug 8, 2026 at 10:05 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.
>
> Rework the fault handler:
>
> - Preallocate the page before taking the lock, like do_anonymous_page()
>   does, so it can sleep, reclaim and go through the OOM path, and return
>   VM_FAULT_OOM on failure so the memcg OOM handler runs instead of a fake
>   segfault.
>
> - A lockless probe skips that preallocation when a page is already mapped
>   (e.g. allocated by the bpf program), so the common case wastes no
>   allocation. The rare race where such a page is freed before we take the
>   lock falls back to the non-blocking allocator under the lock.
>
> - Return VM_FAULT_SIGBUS for the non-recoverable errors (lock failure,
>   range-tree and page-table failures) instead of VM_FAULT_SIGSEGV; only
>   BPF_F_SEGV_ON_FAULT, and a scratch-page hole under that flag, is a real
>   user addressing error and keeps VM_FAULT_SIGSEGV.
>
> - Tidy up the error labels.
>
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>

Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>

> ---
>  kernel/bpf/arena.c | 92 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 71 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 555ee2531ef9..09a718ca4c8b 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,59 +490,108 @@ 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 && !(arena->map.map_flags & BPF_F_SEGV_ON_FAULT)) {
> +		/*
> +		 * We run in process context here, so preallocate the page
> +		 * outside the lock with an explicitly sleepable allocator. It
> +		 * can then go through reclaim (both memcg and global) and the
> +		 * OOM path, the way do_anonymous_page() does; under
> +		 * arena->spinlock only the non-blocking allocator is available,
> +		 * which never reclaims. That also decides the return value:
> +		 * VM_FAULT_OOM below is only meaningful if the OOM machinery was
> +		 * actually engaged, which the non-blocking allocator never does.
> +		 */
> +		bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
> +		new_page = bpf_map_alloc_page_sleepable(map);
> +		bpf_map_memcg_exit(old_memcg, new_memcg);
> +		if (!new_page)
> +			return VM_FAULT_OOM;
> +	}
> +
> +	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {
>  		/*
>  		 * A failed lock means a possible deadlock was detected. Don't
>  		 * return VM_FAULT_RETRY: this handler never took mmap_lock, but
>  		 * the fault path would re-take it on retry and deadlock. Fail.
>  		 */
> +		if (new_page)
> +			free_pages_nolock(new_page, 0);
>  		return VM_FAULT_SIGBUS;
> +	}
>  
>  	page = vmalloc_to_page((void *)kaddr);
>  	if (page) {
> -		if (page == arena->scratch_page)
> -			/* BPF triggered scratch here; don't lazy-alloc over it */
> -			goto out_sigsegv;
> +		if (page == arena->scratch_page) {
> +			/*
> +			 * A scratch page marks a hole. Segfault only if the user
> +			 * asked for it; otherwise we could lazy-allocate but
> +			 * choose not to over a hole, so report a bus error.
> +			 */
> +			fault_ret = (arena->map.map_flags & BPF_F_SEGV_ON_FAULT) ?
> +				    VM_FAULT_SIGSEGV : VM_FAULT_SIGBUS;
> +			goto out_err_locked;
> +		}
>  		/* already have a page vmap-ed */
>  		goto out;
>  	}
>  
> +	if (arena->map.map_flags & BPF_F_SEGV_ON_FAULT) {
> +		/* User space requested to segfault when page is not allocated by bpf prog */
> +		fault_ret = VM_FAULT_SIGSEGV;
> +		goto out_err_locked;
> +	}
> +
>  	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
>  
> -	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;
> +	if (!new_page) {
> +		/*
> +		 * Very rare race: the bpf program had allocated a page here, so
> +		 * the lockless probe saw it and we skipped preallocation, but it
> +		 * freed the page before we took the lock. Now we do need one;
> +		 * sleeping is not allowed here, so fall back to the non-blocking
> +		 * allocator and give up if it fails.
> +		 */
> +		ret = bpf_map_alloc_pages(map, map->numa_node, 1, &new_page);
> +		if (ret) {
> +			fault_ret = VM_FAULT_SIGBUS;
> +			goto out_err_locked_memcg;
> +		}
> +	}
>  
>  	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_SIGBUS;
> +		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_SIGBUS;
> +		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;
> +	if (new_page)
> +		free_pages_nolock(new_page, 0);
> +	return fault_ret;
>  }
>  
>  static const struct vm_operations_struct arena_vm_ops = {