Re: [PATCH bpf-next v5 0/5] bpf: Fix arena memory incoherence

["Alexei Starovoitov" <alexei.starovoitov@gmail.com>]

On Fri, Oct 16, 2026 at 07:36 PM Emil Tsalapatis <emil@etsalapatis.com> wrote:
> - Replace opref (v4 patch 7) with the folio lock. A fault rechecks the
>   kernel PTE with the folio locked and installs the user PTE itself with
>   finish_fault(), and a free waits for the folio lock of each page it
>   removed before zapping (new patch 2). (Alexei)

In v4 I asked to check PageHWPoison() in arena_vm_fault() and return
VM_FAULT_LOCKED. You replied that the page can be poisoned right after
that check and __do_fault() will crash:
https://lore.kernel.org/all/DM6I6ERQ4DJA.25JXLLIZJ75E9@etsalapatis.com/
That's a machine check on this page between two checks that are a few
insns apart, while another process has the page mapped.
tcmu_vma_fault() has the same window since 2022.
It's not a reason to open code do_shared_fault() in arena.
Nothing but mm and dax calls finish_fault().

> - Give faults on a reserved range the scratch_page error. Faults on the
>   range being freed sleep until the free releases it, instead of
>   spinning on VM_FAULT_RETRY. (Alexei)

I asked for the same error in both cases. You replied that the fault
on a range being freed never raised a signal and has to wait:
https://lore.kernel.org/all/DM6IJ1O60GH7.2JQCLY20XU62Q@etsalapatis.com/
It doesn't. Free and realloc is normal use. Touching the page in
between is a bug in user space. In BPF_F_SEGV_ON_FAULT arena it gets
SIGSEGV either way.
retire_pgoff, retire_cnt, retire_seq, arena_wait_for_free() and
bpf_map_inc() in the fault path are there to hand it a zero page
instead.

> - Trylock arena->lock and the mmap read lock of every process that maps
>   the arena before retiring a range, and defer the free if one is
>   contended. The worker waits for the contended lock instead of
>   requeueing itself. The deferred span, allocated with
>   __GFP_RETRY_MAYFAIL when the caller can sleep, is the only allocation
>   before the ptes are cleared. (Alexei)

In v4 I said that mmap_read_lock() in zap_pages() is fine. In the same
reply you said that it has to be a trylock, because iter/task_vma prog
and bpf_find_vma() callback run with mmap_lock held.
zap_pages() does mmap_read_lock() today. If such progs can deadlock
there it's a separate bug with a separate fix.
With the trylocks sleepable bpf_arena_free_pages() is deferred when
another cpu is freeing any page of the arena, or when a writer holds or
waits for mmap_lock of any process that maps it.
arena_race.c has to cope with that:
  /* The reader may populate the slot first, or free may defer. */
The page_cnt == 1 shortcut is gone too, so every free takes mmap_lock
of every process.

[...]
>  kernel/bpf/arena.c                            | 702 +++++++++++++-----

mmap_read_lock() in zap_pages() and the page_cnt == 1 shortcut stay.
The worker doesn't requeue itself, so no need for patch 1.
What is missing: the reader in arena_race.c needs a SIGBUS handler.
Something like this (not even compile tested):

diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
index 0ff7077..3689900 100644
--- a/kernel/bpf/arena.c
+++ b/kernel/bpf/arena.c
@@ -543,6 +543,12 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
 		goto out_err_locked;
 	}
 
+	if (is_range_tree_set(&arena->rt, vmf->pgoff, 1)) {
+		/* Allocated without a page: reserved or being freed */
+		fault_ret = VM_FAULT_SIGBUS;
+		goto out_err_locked;
+	}
+
 	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
 
 	if (!new_page) {
@@ -584,8 +590,22 @@ out:
 	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
 	if (new_page)
 		free_pages_nolock(new_page, 0);
-	vmf->page = page;
-	return 0;
+
+	/*
+	 * arena_free_range() clears the pte and then takes the page lock
+	 * before it zaps user vma-s. Either it waits for this fault and zaps
+	 * its pte, or the page is gone from here. See tcmu_vma_fault().
+	 */
+	lock_page(page);
+	if (vmalloc_to_page((void *)kaddr) == page && !PageHWPoison(page)) {
+		vmf->page = page;
+		return VM_FAULT_LOCKED;
+	}
+	/* The page was freed meanwhile. Let the fault start over. */
+	fault_ret = PageHWPoison(page) ? VM_FAULT_HWPOISON : VM_FAULT_NOPAGE;
+	unlock_page(page);
+	put_page(page);
+	return fault_ret;
 
 out_err_locked_memcg:
 	bpf_map_memcg_exit(old_memcg, new_memcg);
@@ -854,7 +874,7 @@ static void zap_pages(struct bpf_arena *arena, long uaddr, long page_cnt)
 	 * otherwise call B's mark would make call A skip a vml that A has
 	 * not yet zapped for A's uaddr range.
 	 */
-	mutex_lock(&arena->zap_mutex);
+	lockdep_assert_held(&arena->zap_mutex);
 	mutex_lock(&arena->lock);
 	my_gen = ++arena->zap_gen;
 	for (;;) {
@@ -888,49 +908,41 @@ static void zap_pages(struct bpf_arena *arena, long uaddr, long page_cnt)
 		mutex_lock(&arena->lock);
 	}
 	mutex_unlock(&arena->lock);
-	mutex_unlock(&arena->zap_mutex);
 }
 
-static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt, bool sleepable)
+/*
+ * The range stays allocated in the range tree until the pages are unmapped
+ * from vmalloc area and from all user space vma-s, so that nobody can
+ * populate it while stale TLB entries or user ptes are still around.
+ */
+static int arena_free_range(struct bpf_arena *arena, long uaddr, long page_cnt)
 {
-	struct mem_cgroup *new_memcg, *old_memcg;
-	u64 full_uaddr, uaddr_end;
-	long kaddr, pgoff;
-	struct page *page;
-	struct llist_head free_pages;
-	struct llist_node *pos, *t;
-	struct arena_free_span *s;
+	u64 full_uaddr = clear_lo32(arena->user_vm_start) + uaddr;
+	long kaddr = bpf_arena_get_kern_vm_start(arena) + uaddr;
+	long pgoff = compute_pgoff(arena, uaddr);
 	struct clear_range_data cdata;
+	struct llist_node *pos, *t;
+	LLIST_HEAD(free_pages);
 	unsigned long flags;
-	int ret = 0;
-
-	/* only aligned lower 32-bit are relevant */
-	uaddr = (u32)uaddr;
-	uaddr &= PAGE_MASK;
-	kaddr = bpf_arena_get_kern_vm_start(arena) + uaddr;
-	full_uaddr = clear_lo32(arena->user_vm_start) + uaddr;
-	if (full_uaddr < arena->user_vm_start)
-		return;
-	uaddr_end = min(arena->user_vm_end, full_uaddr + (page_cnt << PAGE_SHIFT));
-	if (full_uaddr >= uaddr_end)
-		return;
-
-	page_cnt = (uaddr_end - full_uaddr) >> PAGE_SHIFT;
-	pgoff = compute_pgoff(arena, uaddr);
-	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
+	struct page *page;
+	int ret;
 
-	if (!sleepable)
-		goto defer;
+	/* An overlapping free must not release the range before this one is done */
+	guard(mutex)(&arena->zap_mutex);
 
 	ret = raw_res_spin_lock_irqsave(&arena->spinlock, flags);
-
-	/* Can't proceed without holding the spinlock so defer the free */
 	if (ret)
-		goto defer;
+		return ret;
 
-	range_tree_set(&arena->rt, pgoff, page_cnt);
+	/*
+	 * Take the free pages of the range too. It allocates, and can fail,
+	 * only when the whole range is free. Then there is nothing to do.
+	 */
+	if (range_tree_clear(&arena->rt, pgoff, page_cnt)) {
+		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
+		return 0;
+	}
 
-	init_llist_head(&free_pages);
 	cdata.arena = arena;
 	cdata.free_pages = &free_pages;
 	/* clear ptes and collect struct pages */
@@ -940,6 +952,12 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
 	/* drop the lock to do the tlb flush and zap pages */
 	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
 
+	/* wait for arena_vm_fault() that found the page before its pte was cleared */
+	llist_for_each_entry(page, free_pages.first, pcp_llist) {
+		lock_page(page);
+		unlock_page(page);
+	}
+
 	/* ensure no stale TLB entries */
 	flush_tlb_kernel_range(kaddr, kaddr + (page_cnt * PAGE_SIZE));
 
@@ -958,8 +976,39 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
 			zap_pages(arena, full_uaddr, 1);
 		__free_page(page);
 	}
-	bpf_map_memcg_exit(old_memcg, new_memcg);
 
+	/* The callers hold no other rqspinlock, so this one can only time out */
+	while (raw_res_spin_lock_irqsave(&arena->spinlock, flags))
+		cond_resched();
+	range_tree_set(&arena->rt, pgoff, page_cnt);
+	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
+	return 0;
+}
+
+static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt, bool sleepable)
+{
+	struct mem_cgroup *new_memcg, *old_memcg;
+	u64 full_uaddr, uaddr_end;
+	struct arena_free_span *s;
+
+	/* only aligned lower 32-bit are relevant */
+	uaddr = (u32)uaddr;
+	uaddr &= PAGE_MASK;
+	full_uaddr = clear_lo32(arena->user_vm_start) + uaddr;
+	if (full_uaddr < arena->user_vm_start)
+		return;
+	uaddr_end = min(arena->user_vm_end, full_uaddr + (page_cnt << PAGE_SHIFT));
+	if (full_uaddr >= uaddr_end)
+		return;
+
+	page_cnt = (uaddr_end - full_uaddr) >> PAGE_SHIFT;
+	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
+
+	/* Can't proceed without holding the spinlock so defer the free */
+	if (!sleepable || arena_free_range(arena, uaddr, page_cnt))
+		goto defer;
+
+	bpf_map_memcg_exit(old_memcg, new_memcg);
 	return;
 
 defer:
@@ -1021,66 +1070,18 @@ static void arena_free_worker(struct work_struct *work)
 {
 	struct bpf_arena *arena = container_of(work, struct bpf_arena, free_work);
 	struct mem_cgroup *new_memcg, *old_memcg;
-	struct llist_node *list, *pos, *t;
+	struct llist_node *pos, *t;
 	struct arena_free_span *s;
-	u64 arena_vm_start, user_vm_start;
-	struct llist_head free_pages;
-	struct clear_range_data cdata;
-	struct page *page;
-	unsigned long full_uaddr;
-	long kaddr, page_cnt, pgoff;
-	unsigned long flags;
-
-	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {
-		schedule_work(work);
-		return;
-	}
 
 	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
 
-	init_llist_head(&free_pages);
-	cdata.arena = arena;
-	cdata.free_pages = &free_pages;
-	arena_vm_start = bpf_arena_get_kern_vm_start(arena);
-	user_vm_start = bpf_arena_get_user_vm_start(arena);
-
-	list = llist_del_all(&arena->free_spans);
-	llist_for_each(pos, list) {
+	llist_for_each_safe(pos, t, llist_del_all(&arena->free_spans)) {
 		s = llist_entry(pos, struct arena_free_span, node);
-		page_cnt = s->page_cnt;
-		kaddr = arena_vm_start + s->uaddr;
-		pgoff = compute_pgoff(arena, s->uaddr);
-
-		/* clear ptes and collect pages in free_pages llist */
-		apply_to_existing_page_range(&init_mm, kaddr, page_cnt << PAGE_SHIFT,
-					     apply_range_clear_cb, &cdata);
-
-		range_tree_set(&arena->rt, pgoff, page_cnt);
-	}
-	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
-
-	/* Iterate the list again without holding spinlock to do the tlb flush and zap_pages */
-	llist_for_each_safe(pos, t, list) {
-		s = llist_entry(pos, struct arena_free_span, node);
-		page_cnt = s->page_cnt;
-		full_uaddr = clear_lo32(user_vm_start) + s->uaddr;
-		kaddr = arena_vm_start + s->uaddr;
-
-		/* ensure no stale TLB entries */
-		flush_tlb_kernel_range(kaddr, kaddr + (page_cnt * PAGE_SIZE));
-
-		/* remove pages from user vmas */
-		zap_pages(arena, full_uaddr, page_cnt);
-
+		while (arena_free_range(arena, s->uaddr, s->page_cnt))
+			cond_resched();
 		kfree_nolock(s);
 	}
 
-	/* free all pages collected by apply_to_existing_page_range() in the first loop */
-	llist_for_each_safe(pos, t, __llist_del_all(&free_pages)) {
-		page = llist_entry(pos, struct page, pcp_llist);
-		__free_page(page);
-	}
-
 	bpf_map_memcg_exit(old_memcg, new_memcg);
 }
 

pw-bot: cr