Re: [PATCH bpf-next v3 5/6] bpf: Atomically update PTE and range tree in arena VM fault handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: memory isolation breach on arena spinlock -EDEADLK
- [Medium] bpf: spurious VM_FAULT_SIGBUS on concurrent arena page faults
--

commit a5d5ccfbb2dd50c63d33c5a9de680c3dad099509
Author: Emil Tsalapatis <emil@etsalapatis.com>

bpf: Atomically update PTE and range tree in arena VM fault handler

This commit modifies the arena VM fault handler to atomically adjust the
range tree and insert PTEs, avoiding deadlocks with the range tree spinlock
under PREEMPT_RT. It does this by marking the range as unavailable, dropping
the lock to insert the PTE, then re-acquiring the lock to fully remove the
range.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 69c8924f4c306..5438b68d269a8 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -562,7 +565,7 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>  	/* If a range is unavailable, try again. */
>  	if (ret == -EAGAIN) {
>  		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> -		goto retry;
> +		goto retry_memcg;
>  	} else if (ret) {
[ ... ]
> @@ -583,12 +586,40 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
[ ... ]
> +	fault_ret = vmf_insert_page(vmf->vma, vmf->address, page);
> +	while ((ret = raw_res_spin_lock_irqsave(&arena->spinlock, flags))) {
> +		/* If we somehow deadlocked stop trying to take the lock. */
> +		if (ret == -EDEADLK) {
> +			range_node_mark_available(unavail_node);
> +			return VM_FAULT_SIGBUS;
> +		}

[Severity: Critical]
Does this error path leave memory isolation broken? By calling
range_node_mark_available(), it marks the range as free in the arena's range
tree allocator. 

However, the page was already mapped into the kernel vmap via
apply_to_page_range() and the user page tables via vmf_insert_page().

If the lock returns -EDEADLK, subsequent allocations might hand out this same
page to other BPF programs or userspace, potentially causing two independent
users to share and corrupt the same memory. Should this path unmap the page
before returning?

> +
> +		cond_resched();
> +	}
[ ... ]
> @@ -598,8 +629,9 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
[ ... ]
> -retry:
> +retry_memcg:
>  	bpf_map_memcg_exit(old_memcg, new_memcg);
> +retry:
>  	if (new_page)
>  		free_pages_nolock(new_page, 0);
>  
>  	if (!(vmf->flags & FAULT_FLAG_ALLOW_RETRY))
>  		return VM_FAULT_SIGBUS;

[Severity: Medium]
Will this lead to spurious process crashes for concurrent faults? When two
threads concurrently page-fault on the same arena page, the first thread marks
the range as unavailable and drops the spinlock to call vmf_insert_page().

The second thread sees the unavailable range via range_tree_clear(), which
returns -EAGAIN. This sends the second thread to retry_memcg and it returns
VM_FAULT_RETRY. 

When the kernel fault handler drops the mmap lock and retries the fault, it
clears FAULT_FLAG_ALLOW_RETRY. If the first thread hasn't removed the
unavailable node yet, the second thread hits -EAGAIN again, fails this flag
check, and returns VM_FAULT_SIGBUS instead of waiting.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923191125.5311-1-emil@etsalapatis.com?part=5