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

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 ?