Re: [PATCH bpf-next 4/5] 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 4 potential issue(s) to consider:

New issues:
- [High] Concurrent free operations are silently dropped if they race with page faults, causing unrecoverable memory leaks.
- [High] Spurious SIGBUS on transient page contention for concurrent page faults.
- [High] Lock imbalance (underflow) on retry when called under the per-VMA lock.

Pre-existing issues:
- [High] Sleeping in atomic context during page table allocation/mapping.
--

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

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

This patch addresses a race condition in the arena VM fault handler by
temporarily marking the page range as unavailable while dropping the
range tree spinlock to insert the PTE. It also adds logic to return
VM_FAULT_RETRY for transient allocation failures.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index d22b71a791db3..d7006cdb98990 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -514,6 +510,14 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>  		goto out_sigsegv_memcg;
>  
>  	ret = range_tree_clear(&arena->rt, vmf->pgoff, 1);
> +	/* If a range is unavailable, try again. */
> +	if (ret == -EAGAIN) {
> +		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +		bpf_map_memcg_exit(old_memcg, new_memcg);
> +
> +		goto retry;
> +	}
> +
>  	if (ret)
>  		goto out_sigsegv_memcg;

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to call
bpf_map_alloc_pages() and apply_to_page_range() directly after this block?

The spinlock is acquired via raw_res_spin_lock_irqsave() at the start of
arena_vm_fault(), which disables hardware interrupts. Just below this diff,
we have:

    ret = bpf_map_alloc_pages(map, NUMA_NO_NODE, 1, &page);
    if (ret) {
        ...
    }
    ret = apply_to_page_range(&init_mm, kaddr, PAGE_SIZE, ...);

Since both of these functions can allocate memory and take sleepable locks
(especially under PREEMPT_RT), could this cause a sleeping-in-atomic-context
violation?

[ ... ]

> @@ -534,15 +538,41 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
> +retry:
> +
> +	/* Only for special cases (GUP/device drivers). */
> +	if (!(vmf->flags & FAULT_FLAG_ALLOW_RETRY))
> +		return VM_FAULT_SIGBUS;

[Severity: High]
Could this lead to spurious SIGBUS crashes for userspace applications?

When two threads fault on the same arena page concurrently, the first
thread sets the page to unavailable. The second thread receives -EAGAIN
and jumps to the retry label.

However, if this is the second attempt (where FAULT_FLAG_ALLOW_RETRY has
been cleared, e.g., in do_user_addr_fault), it seems this will escalate
the transient contention into a fatal VM_FAULT_SIGBUS instead of waiting
for the first thread to finish.

> +
> +	if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
> +		release_fault_lock(vmf);

[Severity: High]
Does this properly handle faults entered under the per-VMA lock?

The release_fault_lock() helper unconditionally calls mmap_read_unlock().
If the fault was entered under the RCU-based per-VMA lock (indicated by
FAULT_FLAG_VMA_LOCK), it appears this would mistakenly unlock the unheld
mmap_lock and leak the VMA lock, potentially causing an imbalance. 

Should this check for FAULT_FLAG_VMA_LOCK and call vma_end_read() when
appropriate?

> +
> +	return VM_FAULT_RETRY;
>  }

[ ... ]

> @@ -888,7 +918,9 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
>  		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>  		if (ret == -ENOMEM)
>  			goto defer;
> -		WARN_ON_ONCE(ret);
> +		/* An overlapping fault reserves the range before installing its PTE. */
> +		if (ret != -EAGAIN)
> +			WARN_ON_ONCE(ret);
>  		bpf_map_memcg_exit(old_memcg, new_memcg);
>  		return;
>  	}

[Severity: High]
Will this silently drop concurrent free operations?

If range_tree_set_unavail() returns -EAGAIN because an overlapping fault
has temporarily reserved the range, the function returns immediately without
freeing the page or deferring the work.

Since the fault handler does not free the memory either, it looks like the
page might permanently leak. Should the -EAGAIN case be retried or deferred
similar to the -ENOMEM case?

[ ... ]

> @@ -1043,7 +1075,7 @@ static void arena_free_worker(struct work_struct *work)
>  			 * the defer: path of arena_free_pages(). Do not treat
>  			 * the leak as a bug.
>  			 */
> -			if (ret != -ENOMEM)
> +			if (ret != -ENOMEM && ret != -EAGAIN)
>  				WARN_ON_ONCE(ret);
>  
>  			kfree_nolock(s);
>  			continue;

[Severity: High]
Similar to the previous concern, does this permanently leak deferred free
operations that encounter an unavailable range?

If the deferred worker gets -EAGAIN, it frees the tracking span and
continues, effectively discarding the free request.

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