> arena_vm_fault() allocated the page while holding arena->spinlock, so it > could only use the non-blocking allocator, which never reclaims. 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 over memory that > reclaim could have freed. > > Rework the fault handler: > > - Preallocate the page before taking the lock, like do_anonymous_page() > does, so it can sleep and reclaim, instead of turning a routine > memory.max into a fake segfault. The allocation uses > __GFP_RETRY_MAYFAIL so it never invokes the OOM killer: the page is > charged to the map's memcg, which need not be the faulting task's, so > an OOM there could kill unrelated tasks in the map's cgroup while a > foreign faulter could never be its victim. Like the other bpf map > allocators it places the page on the map's numa_node rather than > following the task's mempolicy; arena memory is shared, so the map's > node is the right placement policy.
This isn't a bug, but since arena_map_alloc() rejects BPF_F_NUMA_NODE, map->numa_node is always NUMA_NO_NODE here. Is the numa_node rationale in the changelog saying more than 'unchanged, still the local node'? > - On allocation failure fall through to the locked recheck rather than > failing right away: a page a concurrent allocator installed meanwhile > is used, otherwise the non-blocking fallback fails and we return > VM_FAULT_SIGBUS. Not VM_FAULT_OOM: nothing ran the OOM killer, and the > fault path would just retry it forever. > > - 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 other 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. Small changelog nit: the lock-failure path already returned VM_FAULT_SIGBUS before this patch -- would it be clearer to list only the range-tree/ page-table/allocation failures as the ones changing from VM_FAULT_SIGSEGV? > - Tidy up the error labels. > > Reviewed-by: Emil Tsalapatis <[email protected]> > Signed-off-by: Jiayuan Chen <[email protected]> > diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c > index 7b6847200b431..2d0efc0232afd 100644 > --- a/kernel/bpf/arena.c > +++ b/kernel/bpf/arena.c [ ... ] > @@ -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)) { > + /* > + * Preallocate outside the lock so the allocation can sleep and > + * reclaim, which the non-blocking allocator under > + * arena->spinlock cannot. __GFP_RETRY_MAYFAIL: reclaim, but > + * never OOM-kill in the map's memcg, which need not be the > + * faulting task's. On failure, fallthrough: the locked > + * recheck below picks up a page a concurrent allocator may > + * have installed meanwhile, and otherwise the non-blocking > + * fallback fails and we return VM_FAULT_SIGBUS. Not > + * VM_FAULT_OOM: nothing ran the OOM killer, and the fault path > + * would just retry it forever. > + */ > + bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg); > + new_page = alloc_pages_node(map->numa_node, > + GFP_KERNEL | __GFP_ZERO | > + __GFP_ACCOUNT | __GFP_NOWARN | > + __GFP_RETRY_MAYFAIL, 0); > + bpf_map_memcg_exit(old_memcg, new_memcg); > + } > + > + 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; > + } [ ... ] --- 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/35327041280
