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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unnecessary sleepable memory allocation introduces a Denial of Service and OOM vector when BPF_F_SEGV_ON_FAULT is set.

Pre-existing issues:
- [High] Missing lock release before returning VM_FAULT_RETRY causes a lock leak and deadlocks.
--

--- Patch [2]: [PATCH bpf-next 2/3] bpf: arena: allocate the fault-in page outside the lock ---
commit 618b434e77e16e8a5277e1b3cc85c17fc5ef5e7a
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

bpf: arena: allocate the fault-in page outside the lock

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.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 34f023a537fe6..22a41e3c53b81 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.
> +		 *
> +		 * 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);

[Severity: High]
Is there a potential Denial of Service vector when BPF_F_SEGV_ON_FAULT is set?

This unconditionally preallocates a page using
bpf_map_alloc_page_sleepable() before checking the BPF_F_SEGV_ON_FAULT
flag later in the function.

This allocation can block, trigger direct memory reclaim, and invoke the
OOM killer. Only after this expensive allocation does the code lock, check
the flag, return VM_FAULT_SIGSEGV, and discard the page.

Could this allow an unprivileged user to force severe memory pressure by
repeatedly triggering page faults on unallocated addresses?

> +		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;
> +	}

[Severity: High]
This is a pre-existing issue, but does returning VM_FAULT_RETRY here
without releasing the fault lock cause a lock leak?

The memory management subsystem strictly assumes the lock was released
if VM_FAULT_RETRY is returned. Retrying the fault will leak the read lock
reference, and if any thread subsequently attempts to acquire the mmap_lock
for writing, the system could permanently deadlock.

>  	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;
> +	}

[Severity: High]
Could returning VM_FAULT_RETRY here in this new race handling path also
leak the fault lock and risk deadlocks, similar to the lock acquisition
failure above?

> +
>  	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;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727062521.376231-1-jiayuan.chen@linux.dev?part=2