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
