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

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 7b6847200b43..fa462a0ff1ad 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c

[ ... ]

> @@ -489,59 +490,106 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>  	kbase = bpf_arena_get_kern_vm_start(arena);
>  	kaddr = kbase + (u32)(vmf->address);
>
> +	page = vmalloc_to_page((void *)kaddr);
> +	if (!page && !(arena->map.map_flags & BPF_F_SEGV_ON_FAULT)) {
> +		/*
> +		 * Preallocate outside the lock with a sleepable allocator so it
> +		 * can reclaim and run the memcg OOM killer, which the
> +		 * non-blocking allocator under arena->spinlock cannot. A NULL
> +		 * return is non-recoverable, so fail with VM_FAULT_SIGBUS;
> +		 * VM_FAULT_OOM would be retried by the fault path and can
> +		 * livelock when the charged memcg is not the faulting task's.
> +		 */
> +		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_SIGBUS;
> +	}
> +
> +	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {

[ ... ]

>  	page = vmalloc_to_page((void *)kaddr);
>  	if (page) {
> +		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;
> +		}

This isn't a bug, but could this comment spell out why a scratch-marked
hole is not lazy-allocated even though an untouched hole is? A reader might
wonder why the two cases are treated differently since both are equally
free as far as the range tree is concerned.

>  		/* 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 (!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);

This isn't a bug, but would it be clearer to describe the condition
generically (the lockless probe's result was invalidated before the lock
was taken) rather than naming only the bpf-alloc-then-free case? The
fallback is also reached if the probe observed arena->scratch_page and the
PTE was cleared by arena_free_pages() before the lock was taken, which is
not a bpf program page allocation.

[ ... ]


---
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/32713810092