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

[Jiayuan Chen]

On 7/27/26 2:42 PM, sashiko-bot@xxxxxxxxxx wrote:
[...]
> > @@ -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?

Once VM_FAULT_SIGSEGV is returned, the process gets a SIGSEGV and by 
default is killed,
so this isn't a realistic DoS.

But, I think it's worth moving the "arena->map.map_flags & 
BPF_F_SEGV_ON_FAULT" check before the allocation.


> > +		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.


Yes, it's true. arena_vm_fault() never touches mmap_lock, so returning 
VM_FAULT_RETRY violates the contract.

'''
do_user_addr_fault()
{
    fault = handle_mm_fault(...);          // call arena_vm_fault
    ...
    if (unlikely(fault & VM_FAULT_RETRY)) {
        flags |= FAULT_FLAG_TRIED;
        goto retry;                    // lock_mm_and_find_vma() will 
call mmap_read_lock again !
    }
    mmap_read_unlock(mm);
}
'''

I think I should fix it as a separate patch with high priority ?