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

On Mon Jul 27, 2026 at 4:00 AM EDT, Jiayuan Chen wrote:
>
> On 7/27/26 2:42 PM, [email protected] 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 ?

Please do, I think it makes sense as a separate patch targeting the
bpf tree while this patchset can keep targeting bpf-next.