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

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

[Emil Tsalapatis]

Setting up arena memory for a task currently requires two operations:
Adjusting its range tree, used for tracking which memory is allocated;
and adjusting its page tables/flushing its TLB state. These operations
cannot happen atomically because their critical sections do not nest.
This lack of atomicity is the source of two bugs that can lead to
incoherence between different users of the same arena, wherein they
observe different pages for the same address.

Address the problem by more finely tracking the state of each address.
Expand the range tree used for address state tracking with a third
state, used to denote whether an address range is unavailable, either
because it is being freed or because it is being populated by a VM
fault. Use this extra state in the arena page freeing/fault logic
to ensure that operations on a single address properly serialize.

Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>

Emil Tsalapatis (5):
  bpf: Update is_range_tree_set to work for consecutive ranges
  bpf: Track availability information for ranges in range tree
  bpf: Fix arena race between page free and alloc leading to incoherency
  bpf: Atomically update PTE and range tree in arena VM fault handler
  selftests/bpf: Add arena allocation race tests

 kernel/bpf/arena.c                            | 154 +++++++++--
 kernel/bpf/range_tree.c                       | 211 ++++++++++++---
 kernel/bpf/range_tree.h                       |   5 +-
 .../selftests/bpf/prog_tests/arena_race.c     | 251 ++++++++++++++++++
 .../testing/selftests/bpf/progs/arena_race.c  | 163 ++++++++++++
 5 files changed, 728 insertions(+), 56 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/arena_race.c
 create mode 100644 tools/testing/selftests/bpf/progs/arena_race.c

-- 
2.55.0

[PATCH bpf-next 1/5] bpf: Update is_range_tree_set to work for consecutive ranges

[Emil Tsalapatis]

The arena range tree currently does not handle consecutive
ranges present in the tree. This is by design: Consecutive
ranges get merged into one by default. However, this design
only lets us track a single bit's worth of state for each
address range, encoded by whether the range is present in
the tree or not (i.e., is it allocated).

We require more fine-grained state tracking for each range.
This means possibly having in the tree consecutive ranges
that cannot be merged because they have different states.
However, existing code implicitly assumes that this scenario
is not possible in its logic.

Expand the logic of is_range_tree_set to handle consecutive
ranges in the tree. The logic change does not affect existing
users and amounts to a defensive check until we enable unmergeable
consecutive ranges in subsequent patches.

Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>
---
 kernel/bpf/range_tree.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
index 2f28886f3ff7..6f6ba718887c 100644
--- a/kernel/bpf/range_tree.c
+++ b/kernel/bpf/range_tree.c
@@ -180,12 +180,21 @@ int range_tree_clear(struct range_tree *rt, u32 start, u32 len)
 int is_range_tree_set(struct range_tree *rt, u32 start, u32 len)
 {
 	u32 last = start + len - 1;
-	struct range_node *left;
+	struct range_node *rn;
 
-	/* Is this whole range set ? */
-	left = range_it_iter_first(rt, start, last);
-	if (left && left->rn_start <= start && left->rn_last >= last)
-		return 0;
+	while ((rn = range_it_iter_first(rt, start, last))) {
+		/* Make sure the range covers the start */
+		if (rn->rn_start > start)
+			return -ESRCH;
+
+		/* If it covers the entire range we're done. */
+		if (rn->rn_last >= last)
+			return 0;
+
+		start = rn->rn_last + 1;
+	}
+
+	/* No range to cover [start, last] */
 	return -ESRCH;
 }
 
-- 
2.55.0

Re: [PATCH bpf-next 1/5] bpf: Update is_range_tree_set to work for consecutive ranges

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

> diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
> index 2f28886f3ff75..6f6ba718887c4 100644
> --- a/kernel/bpf/range_tree.c
> +++ b/kernel/bpf/range_tree.c
> @@ -180,12 +180,21 @@ int range_tree_clear(struct range_tree *rt, u32 start, u32 len)
>  int is_range_tree_set(struct range_tree *rt, u32 start, u32 len)
>  {
>  	u32 last = start + len - 1;
> -	struct range_node *left;
> +	struct range_node *rn;
>
> -	/* Is this whole range set ? */
> -	left = range_it_iter_first(rt, start, last);
> -	if (left && left->rn_start <= start && left->rn_last >= last)
> -		return 0;
> +	while ((rn = range_it_iter_first(rt, start, last))) {
> +		/* Make sure the range covers the start */
> +		if (rn->rn_start > start)
> +			return -ESRCH;
> +
> +		/* If it covers the entire range we're done. */
> +		if (rn->rn_last >= last)
> +			return 0;
> +
> +		start = rn->rn_last + 1;
> +	}
> +
> +	/* No range to cover [start, last] */
>  	return -ESRCH;
>  }

This isn't a bug, but since this loop doesn't modify the tree (unlike
range_tree_clear() just above), would __range_it_iter_next() work here
instead of re-descending from the root with range_it_iter_first() each
iteration?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33603499110

[PATCH bpf-next 2/5] bpf: Track availability information for ranges in range tree

[Emil Tsalapatis]

Arena address ranges are currently encoded in a range tree: Ranges
present in the tree are free and available for allocation, while
absent ranges are allocated. However, this opens up the arena code to
subtle races between page table updates, range tree updates, and
concurrent allocations that can cause permanent inconsistencies.

Avoiding such races involves distinguishing between memory ranges that
are free and ready to be allocated, and ranges that are being freed but
should not be reused yet. Such tracking is cleanly possible through the
arena's range tree. The range tree is only consumed by arena and has no
additional future consumers, so it can be tailored towards tracking more
state. Alternatives such as deferring range freeing require more
asynchrony and per-range state tracking in the main arena code, and can
introduce additional race conditions.

Expand the tree to track whether a range present in the tree is
available for allocation. For now, all ranges are available: We add
the option to add back a range in an unavailable state, and to move
already present ranges from unavailable to available. We do not
implement other transitions, since they are not required to support
BPF arenas.

The patch adds two operations: Adding a range as unavailable, and
turning a range from unavailable to available. Unavailable ranges are
not mergable, and will be imminently be turned available by the ongoing
arena free() operation that created them. Turning ranges from
unavailable to available is a simple flag change on the range with an
optional merge with adjacent available ranges.

Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>
---
 kernel/bpf/arena.c      |  12 +--
 kernel/bpf/range_tree.c | 180 +++++++++++++++++++++++++++++++++-------
 kernel/bpf/range_tree.h |   4 +-
 3 files changed, 161 insertions(+), 35 deletions(-)

diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
index 7b6847200b43..f49b52fa8586 100644
--- a/kernel/bpf/arena.c
+++ b/kernel/bpf/arena.c
@@ -317,7 +317,7 @@ static struct bpf_map *arena_map_alloc(union bpf_attr *attr)
 		goto err_free_arena;
 
 	range_tree_init(&arena->rt);
-	err = range_tree_set(&arena->rt, 0, attr->max_entries);
+	err = range_tree_set_avail(&arena->rt, 0, attr->max_entries);
 	if (err)
 		goto err_free_scratch;
 	mutex_init(&arena->lock);
@@ -520,13 +520,13 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
 	/* Account into memcg of the process that created bpf_arena */
 	ret = bpf_map_alloc_pages(map, NUMA_NO_NODE, 1, &page);
 	if (ret) {
-		range_tree_set(&arena->rt, vmf->pgoff, 1);
+		range_tree_set_avail(&arena->rt, vmf->pgoff, 1);
 		goto out_sigsegv_memcg;
 	}
 
 	ret = apply_to_page_range(&init_mm, kaddr, PAGE_SIZE, apply_range_set_cb, &data);
 	if (ret) {
-		range_tree_set(&arena->rt, vmf->pgoff, 1);
+		range_tree_set_avail(&arena->rt, vmf->pgoff, 1);
 		free_pages_nolock(page, 0);
 		goto out_sigsegv_memcg;
 	}
@@ -766,7 +766,7 @@ static long arena_alloc_pages(struct bpf_arena *arena, long uaddr, long page_cnt
 	bpf_map_memcg_exit(old_memcg, new_memcg);
 	return clear_lo32(arena->user_vm_start) + uaddr32;
 out:
-	range_tree_set(&arena->rt, pgoff + mapped, page_cnt - mapped);
+	range_tree_set_avail(&arena->rt, pgoff + mapped, page_cnt - mapped);
 	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
 	if (mapped) {
 		flush_vmap_cache(kern_vm_start + uaddr32, mapped << PAGE_SHIFT);
@@ -881,7 +881,7 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
 	if (ret)
 		goto defer;
 
-	range_tree_set(&arena->rt, pgoff, page_cnt);
+	range_tree_set_avail(&arena->rt, pgoff, page_cnt);
 
 	init_llist_head(&free_pages);
 	cdata.arena = arena;
@@ -1008,7 +1008,7 @@ static void arena_free_worker(struct work_struct *work)
 		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);
+		range_tree_set_avail(&arena->rt, pgoff, page_cnt);
 	}
 	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
 
diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
index 6f6ba718887c..515e72f054f5 100644
--- a/kernel/bpf/range_tree.c
+++ b/kernel/bpf/range_tree.c
@@ -39,8 +39,15 @@ struct range_node {
 	u32 rn_start;
 	u32 rn_last; /* inclusive */
 	u32 __rn_subtree_last;
+	bool available; /* range is available for allocating. */
 };
 
+/* Is the range available for merging? */
+static inline bool range_available(struct range_node *rn)
+{
+	return rn && rn->available;
+}
+
 static struct range_node *rb_to_range_node(struct rb_node *rb)
 {
 	return rb_entry(rb, struct range_node, rb_range_size);
@@ -55,20 +62,30 @@ static u32 rn_size(struct range_node *rn)
 static inline struct range_node *__find_range(struct range_tree *rt, u32 len)
 {
 	struct rb_node *rb = rt->range_size_root.rb_root.rb_node;
-	struct range_node *best = NULL;
+	struct rb_node *best = NULL;
+	struct range_node *rn;
 
 	while (rb) {
-		struct range_node *rn = rb_to_range_node(rb);
+		rn = rb_to_range_node(rb);
 
 		if (len <= rn_size(rn)) {
-			best = rn;
+			best = rb;
 			rb = rb->rb_right;
 		} else {
 			rb = rb->rb_left;
 		}
 	}
 
-	return best;
+	/* Filter unavailable ranges. */
+	while (best) {
+		rn = rb_to_range_node(best);
+		if (range_available(rn))
+			return rn;
+
+		best = rb_prev(best);
+	}
+
+	return NULL;
 }
 
 s64 range_tree_find(struct range_tree *rt, u32 len)
@@ -135,10 +152,19 @@ range_it_iter_first(struct range_tree *rt, u32 start, u32 last)
 /* Clear the range in this range tree */
 int range_tree_clear(struct range_tree *rt, u32 start, u32 len)
 {
+	u32 first = start;
 	u32 last = start + len - 1;
 	struct range_node *new_rn;
 	struct range_node *rn;
 
+	/* Scan for unavailable ranges and try again if so. */
+	while ((rn = range_it_iter_first(rt, first, last))) {
+		if (!range_available(rn))
+			return -EAGAIN;
+
+		first = rn->rn_last + 1;
+	}
+
 	while ((rn = range_it_iter_first(rt, start, last))) {
 		if (rn->rn_start < start && rn->rn_last > last) {
 			u32 old_last = rn->rn_last;
@@ -153,6 +179,7 @@ int range_tree_clear(struct range_tree *rt, u32 start, u32 len)
 						NUMA_NO_NODE);
 			if (!new_rn)
 				return -ENOMEM;
+			new_rn->available = rn->available;
 			new_rn->rn_start = last + 1;
 			new_rn->rn_last = old_last;
 			range_it_insert(new_rn, rt);
@@ -182,7 +209,8 @@ int is_range_tree_set(struct range_tree *rt, u32 start, u32 len)
 	u32 last = start + len - 1;
 	struct range_node *rn;
 
-	while ((rn = range_it_iter_first(rt, start, last))) {
+	for (rn = range_it_iter_first(rt, start, last); rn;
+			rn = __range_it_iter_next(rn, start, last)) {
 		/* Make sure the range covers the start */
 		if (rn->rn_start > start)
 			return -ESRCH;
@@ -198,23 +226,12 @@ int is_range_tree_set(struct range_tree *rt, u32 start, u32 len)
 	return -ESRCH;
 }
 
-/* Set the range in this range tree */
-int range_tree_set(struct range_tree *rt, u32 start, u32 len)
+/* Do we have adjacent ranges (and do not overlap with them)? */
+static int range_get_adjacent(struct range_tree *rt, u32 start, u32 last,
+		struct range_node **leftp, struct range_node **rightp)
 {
-	u32 last = start + len - 1;
 	struct range_node *right;
 	struct range_node *left;
-	int err;
-
-	/* Is this whole range already set ? */
-	left = range_it_iter_first(rt, start, last);
-	if (left && left->rn_start <= start && left->rn_last >= last)
-		return 0;
-
-	/* Clear out everything in the range we want to set. */
-	err = range_tree_clear(rt, start, len);
-	if (err)
-		return err;
 
 	/* Do we have a left-adjacent range ? */
 	left = range_it_iter_first(rt, start - 1, start - 1);
@@ -226,34 +243,141 @@ int range_tree_set(struct range_tree *rt, u32 start, u32 len)
 	if (right && right->rn_start != last + 1)
 		return -EFAULT;
 
-	if (left && right) {
+	*leftp = left;
+	*rightp = right;
+
+	return 0;
+}
+
+/*
+ * Merge with adjacent available ranges if possible. The new [start, last]
+ * has already been confirmed to be adjacent with left/right by the caller.
+ */
+static int range_tree_merge(struct range_tree *rt, u32 start, u32 last,
+		struct range_node *left, struct range_node *right)
+{
+	if (range_available(left) && range_available(right)) {
 		/* Combine left and right adjacent ranges */
 		range_it_remove(left, rt);
 		range_it_remove(right, rt);
 		left->rn_last = right->rn_last;
 		range_it_insert(left, rt);
 		kfree_nolock(right);
-	} else if (left) {
+	} else if (range_available(left)) {
 		/* Combine with the left range */
 		range_it_remove(left, rt);
 		left->rn_last = last;
 		range_it_insert(left, rt);
-	} else if (right) {
+	} else if (range_available(right)) {
 		/* Combine with the right range */
 		range_it_remove(right, rt);
 		right->rn_start = start;
 		range_it_insert(right, rt);
 	} else {
-		left = kmalloc_nolock(sizeof(struct range_node), __GFP_ACCOUNT, NUMA_NO_NODE);
-		if (!left)
-			return -ENOMEM;
-		left->rn_start = start;
-		left->rn_last = last;
-		range_it_insert(left, rt);
+		/* No merge available. */
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+/* Make a range available, possibly merging. */
+int range_tree_make_avail(struct range_tree *rt, u32 start, u32 len)
+{
+	u32 last = start + len - 1;
+	struct range_node *rn;
+	struct range_node *right;
+	struct range_node *left;
+	int err;
+
+	/*
+	 * Confirm the range exists is unavailable,
+	 * and fits the requested range exactly.
+	 */
+	rn = range_it_iter_first(rt, start, last);
+	if (!rn || rn->available)
+		return -EINVAL;
+
+	if (rn->rn_start != start || rn->rn_last != last)
+		return -EINVAL;
+
+	err = range_get_adjacent(rt, start, last, &left, &right);
+	if (err)
+		return err;
+
+	/* If no merging required, just make available. */
+	if (!range_available(left) && !range_available(right)) {
+		rn->available = true;
+		return 0;
+	}
+
+	/* Can merge, remove the range already. */
+	start = rn->rn_start;
+	last = rn->rn_last;
+	range_it_remove(rn, rt);
+	kfree_nolock(rn);
+
+	return range_tree_merge(rt, start, last, left, right);
+}
+
+/* Set the range in this range tree */
+static int range_tree_set(struct range_tree *rt, u32 start, u32 len, bool available)
+{
+	u32 last = start + len - 1;
+	struct range_node *right;
+	struct range_node *left;
+	int err;
+
+	/* Is this whole range already set ? */
+	left = range_it_iter_first(rt, start, last);
+	if (left && left->rn_start <= start && left->rn_last >= last &&
+	    range_available(left) && available)
+		return 0;
+
+	/* Clear out everything in the range we want to set. */
+	err = range_tree_clear(rt, start, len);
+	if (err)
+		return err;
+
+	/* Get adjacent ranges and check for overlaps. */
+	err = range_get_adjacent(rt, start, last, &left, &right);
+	if (err)
+		return err;
+
+	/*
+	 * If the range is not available for allocation, don't merge.
+	 * Unavailable ranges are in the process of being freed and should
+	 * be imminently marked available, so merging them with other
+	 * unavailable ranges will just lead to splitting the range back
+	 * almost immediately.
+	 */
+	if (available) {
+		err = range_tree_merge(rt, start, last, left, right);
+		if (!err)
+			return 0;
 	}
+
+	left = kmalloc_nolock(sizeof(struct range_node), __GFP_ACCOUNT, NUMA_NO_NODE);
+	if (!left)
+		return -ENOMEM;
+	left->available = available;
+	left->rn_start = start;
+	left->rn_last = last;
+	range_it_insert(left, rt);
+
 	return 0;
 }
 
+int range_tree_set_avail(struct range_tree *rt, u32 start, u32 len)
+{
+	return range_tree_set(rt, start, len, true);
+}
+
+int range_tree_set_unavail(struct range_tree *rt, u32 start, u32 len)
+{
+	return range_tree_set(rt, start, len, false);
+}
+
 void range_tree_destroy(struct range_tree *rt)
 {
 	struct range_node *rn;
diff --git a/kernel/bpf/range_tree.h b/kernel/bpf/range_tree.h
index ff0b9110eb71..aa27edf451bc 100644
--- a/kernel/bpf/range_tree.h
+++ b/kernel/bpf/range_tree.h
@@ -14,7 +14,9 @@ void range_tree_init(struct range_tree *rt);
 void range_tree_destroy(struct range_tree *rt);
 
 int range_tree_clear(struct range_tree *rt, u32 start, u32 len);
-int range_tree_set(struct range_tree *rt, u32 start, u32 len);
+int range_tree_set_avail(struct range_tree *rt, u32 start, u32 len);
+int range_tree_set_unavail(struct range_tree *rt, u32 start, u32 len);
+int range_tree_make_avail(struct range_tree *rt, u32 start, u32 len);
 int is_range_tree_set(struct range_tree *rt, u32 start, u32 len);
 s64 range_tree_find(struct range_tree *rt, u32 len);
 
-- 
2.55.0

Re: [PATCH bpf-next 2/5] bpf: Track availability information for ranges in range tree

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 7136 bytes --]

>     The patch adds two operations: Adding a range as unavailable, and
>     turning a range from unavailable to available.

The changelog describes only the two new range states, but the diff makes
two further changes that it never mentions, both of which are the
load-bearing part for reviewers of kernel/bpf/arena.c:

First, range_tree_clear() gains a new return value, -EAGAIN. That changes
the contract of a function called from three places in arena.c
(arena_vm_fault(), arena_alloc_pages() and arena_reserve_pages()), none of
which handle it in this commit. A reader of this changelog has no way to
know the clear() contract moved, and the two follow-up commits in the series
(779ba112cb6ed, 0b79959d464ca) depend on exactly that behaviour.

Second, is_range_tree_set() is rewritten from 'while ((rn =
range_it_iter_first(rt, start, last)))' to 'for (rn =
range_it_iter_first(...); rn; rn = __range_it_iter_next(rn, start, last))'.
This is not mentioned at all, and the immediately preceding commit in the
series -- 780d99e4d42da ("bpf: Update is_range_tree_set to work for
consecutive ranges") -- had just rewritten that same loop into the while()
form this commit replaces. Either the iterator switch belongs in
780d99e4d42da, or this changelog should say why the loop needed changing
again?

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 7b6847200b431..f49b52fa85860 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c

[ ... ]

> @@ -766,7 +766,7 @@ static long arena_alloc_pages(struct bpf_arena *arena, long uaddr, long page_cnt
>  	bpf_map_memcg_exit(old_memcg, new_memcg);
>  	return clear_lo32(arena->user_vm_start) + uaddr32;
>  out:
> -	range_tree_set(&arena->rt, pgoff + mapped, page_cnt - mapped);
> +	range_tree_set_avail(&arena->rt, pgoff + mapped, page_cnt - mapped);
>  	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>  	if (mapped) {
>  		flush_vmap_cache(kern_vm_start + uaddr32, mapped << PAGE_SHIFT);

Looking at arena_alloc_pages() around line 718, this commit gives
range_tree_clear() a brand-new return value, -EAGAIN, emitted by the
pre-scan loop it adds:

kernel/bpf/range_tree.c:range_tree_clear() {
    /* Scan for unavailable ranges and try again if so. */
    while ((rn = range_it_iter_first(rt, first, last))) {
            if (!range_available(rn))
                    return -EAGAIN;
            first = rn->rn_last + 1;
    }
}

The name -EAGAIN and the comment ("try again") say the condition is
transient, but neither arena.c caller that reaches range_tree_clear() from
a kfunc distinguishes it from a permanent failure.

At arena_alloc_pages() (uaddr path): is_range_tree_set() deliberately keeps
no availability check, so it returns 0 for a range that is
present-but-unavailable. range_tree_clear() then returns -EAGAIN, control
falls to 'if (ret) goto out_unlock_free_pages', and arena_alloc_pages()
returns 0 -- bpf_arena_alloc_pages() hands the BPF program NULL. The check
at line 715 and the act at line 718 now disagree about what "set" means.

At arena_reserve_pages() around line 966: -EAGAIN is returned verbatim
through the bpf_arena_reserve_pages() kfunc, whose error set was previously
only {0, -EINVAL, -EBUSY}.

At this commit the condition is structurally impossible -- every range_node
is created with available=true. Per the forward-search rule the git range
198c2bf..667144ef was checked, and the path IS enabled two commits later by
779ba112cb6ed ("bpf: Fix arena race between page free and alloc leading to
incoherency"), where arena_free_pages()/arena_free_worker() mark ranges
unavailable across a flush_tlb_kernel_range() + zap_pages() window
(zap_pages() takes mmap_read_lock() and mmput(), so the window is long, not
narrow).

That same series adds explicit -EAGAIN handling everywhere else it matters
-- arena_vm_fault() gets '-EAGAIN -> VM_FAULT_RETRY' and
arena_free_pages()/arena_free_worker() get 'if (ret != -EAGAIN)
WARN_ON_ONCE(ret)' -- but at the series tip 667144ef the two sites above
are unchanged (667144ef:kernel/bpf/arena.c lines 749 and 1019). So the gap
is not work-in-progress that the series completes: a BPF program allocating
at a fixed uaddr, or reserving a vaddr range, transiently overlapping a
range another CPU is mid-free will see a spurious NULL / an undocumented
-EAGAIN with no retry loop anywhere in the kernel or in the arena selftest
allocators (buddy_init, buddy_unreserve_arena_vaddr). No crash or
corruption -- hence low severity -- but it is a silent,
previously-impossible failure mode.

Worth confirming whether these two paths were meant to retry like the fault
handler does, or whether -EAGAIN should be documented on
bpf_arena_reserve_pages()?

>     bpf: Track availability information for ranges in range tree
>
>     Arena address ranges are currently encoded in a range tree: Ranges
>     present in the tree are free and available for allocation, while
>     absent ranges are allocated. However, this opens up the arena code to
>     subtle races between page table updates, range tree updates, and
>     concurrent allocations that can cause permanent inconsistencies.
>     Avoiding such races involves distinguishing between memory ranges that
>     are free and ready to be allocated, and ranges that are being freed but
>     should not be reused yet. Such tracking is cleanly possible through the
>     arena's range tree. The range tree is only consumed by arena and has no
>     additional future consumers, so it can be tailored towards tracking more
>     state. Alternatives such as deferring range freeing require more
>     asynchrony and per-range state tracking in the main arena code, and can
>     introduce additional race conditions.
>     Expand the tree to track whether a range present in the tree is
>     available for allocation. For now, all ranges are available: We add
>     the option to add back a range in an unavailable state, and to move
>     already present ranges from unavailable to available. We do not
>     implement other transitions, since they are not required to support
>     BPF arenas.
>     The patch adds two operations: Adding a range as unavailable, and
>     turning a range from unavailable to available. Unavailable ranges are
>     not mergable, and will be imminently be turned available by the ongoing
>     arena free() operation that created them. Turning ranges from
>     unavailable to available is a simple flag change on the range with an
>     optional merge with adjacent available ranges.
>
>     Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>

This looks like a bug fix for the race condition in the range tree design
(where ranges cannot distinguish between 'free and ready to allocate' vs
'being freed but not yet reusable'). The race condition was introduced when
the range tree was first added.

Should this include:

  Fixes: 341eef908d04 ("adding ci files")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33603499110

[PATCH bpf-next 3/5] bpf: Fix arena race between page free and alloc leading to incoherency

[Emil Tsalapatis]

Existing arena kfunc code has an underlying race condition
that can lead to writes being lost from the BPF program's
point of view:

a) A memory range gets gets freed by operation (1), and its
range is added back to the arena range tree.

b) A concurrent allocation (2) reallocates the range, and does
writes to it. Writes from that CPU may follow the stale TLB
entries into the pages that are about to be freed.

c) (1) invalidates the TLB. The old pages, and any writes done
to them, are now inaccessible. zap_pages() simlarly removes the
mappings for userspace threads.

This can be triggered by particularly demanding BPF arena data
structures that constantly allocate and deallocate memory, like
hash table allocations.

Solve this ABA problem by preventing range reallocation until
TLB invalidation/unmapping is complete. First, mark the range
freed but unavailable. Afterwards, drop the spinlock lock and
flush the kernel TLB and zap user page tables. Then pick up
the lock again and mark the ranges as available once again,
completing the free operation.

Reported-by: Mykola Lysenko <nickolay.lysenko@gmail.com>
Fixes: 317460317a02 ("bpf: Introduce bpf_arena.")
Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>
---
 kernel/bpf/arena.c | 94 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
index f49b52fa8586..d22b71a791db 100644
--- a/kernel/bpf/arena.c
+++ b/kernel/bpf/arena.c
@@ -76,6 +76,7 @@ struct arena_free_span {
 	struct llist_node node;
 	unsigned long uaddr;
 	u32 page_cnt;
+	bool release_only;
 };
 
 u64 bpf_arena_get_kern_vm_start(struct bpf_arena *arena)
@@ -855,6 +856,7 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
 	struct arena_free_span *s;
 	struct clear_range_data cdata;
 	unsigned long flags;
+	bool release_only = false;
 	int ret = 0;
 
 	/* only aligned lower 32-bit are relevant */
@@ -881,7 +883,15 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
 	if (ret)
 		goto defer;
 
-	range_tree_set_avail(&arena->rt, pgoff, page_cnt);
+	ret = range_tree_set_unavail(&arena->rt, pgoff, page_cnt);
+	if (ret) {
+		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
+		if (ret == -ENOMEM)
+			goto defer;
+		WARN_ON_ONCE(ret);
+		bpf_map_memcg_exit(old_memcg, new_memcg);
+		return;
+	}
 
 	init_llist_head(&free_pages);
 	cdata.arena = arena;
@@ -911,6 +921,16 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
 			zap_pages(arena, full_uaddr, 1);
 		__free_page(page);
 	}
+
+	ret = raw_res_spin_lock_irqsave(&arena->spinlock, flags);
+	if (ret) {
+		release_only = true;
+		goto defer;
+	}
+
+	ret = range_tree_make_avail(&arena->rt, pgoff, page_cnt);
+	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
+	WARN_ON_ONCE(ret);
 	bpf_map_memcg_exit(old_memcg, new_memcg);
 
 	return;
@@ -928,6 +948,7 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
 
 	s->page_cnt = page_cnt;
 	s->uaddr = uaddr;
+	s->release_only = release_only;
 	llist_add(&s->node, &arena->free_spans);
 	irq_work_queue(&arena->free_irq);
 }
@@ -977,12 +998,13 @@ static void arena_free_worker(struct work_struct *work)
 	struct llist_node *list, *pos, *t;
 	struct arena_free_span *s;
 	u64 arena_vm_start, user_vm_start;
-	struct llist_head free_pages;
+	struct llist_head free_pages, teardown_spans, release_spans;
 	struct clear_range_data cdata;
 	struct page *page;
 	unsigned long full_uaddr;
 	long kaddr, page_cnt, pgoff;
 	unsigned long flags;
+	int ret;
 
 	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {
 		schedule_work(work);
@@ -992,28 +1014,51 @@ static void arena_free_worker(struct work_struct *work)
 	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
 
 	init_llist_head(&free_pages);
+	init_llist_head(&teardown_spans);
+	init_llist_head(&release_spans);
 	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, list) {
 		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);
 
+		if (s->release_only) {
+			ret = range_tree_make_avail(&arena->rt, pgoff, page_cnt);
+			WARN_ON_ONCE(ret);
+			kfree_nolock(s);
+			continue;
+		}
+
+		kaddr = arena_vm_start + s->uaddr;
+
+		ret = range_tree_set_unavail(&arena->rt, pgoff, page_cnt);
+		if (ret) {
+			/*
+			 * An -ENOMEM failure is the same failure mode as in
+			 * the defer: path of arena_free_pages(). Do not treat
+			 * the leak as a bug.
+			 */
+			if (ret != -ENOMEM)
+				WARN_ON_ONCE(ret);
+
+			kfree_nolock(s);
+			continue;
+		}
+
 		/* 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_avail(&arena->rt, pgoff, page_cnt);
+		__llist_add(pos, &teardown_spans);
 	}
 	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) {
+	/* Keep ranges unavailable until their stale translations are gone. */
+	llist_for_each_safe(pos, t, __llist_del_all(&teardown_spans)) {
 		s = llist_entry(pos, struct arena_free_span, node);
 		page_cnt = s->page_cnt;
 		full_uaddr = clear_lo32(user_vm_start) + s->uaddr;
@@ -1025,7 +1070,7 @@ static void arena_free_worker(struct work_struct *work)
 		/* remove pages from user vmas */
 		zap_pages(arena, full_uaddr, page_cnt);
 
-		kfree_nolock(s);
+		__llist_add(pos, &release_spans);
 	}
 
 	/* free all pages collected by apply_to_existing_page_range() in the first loop */
@@ -1034,6 +1079,37 @@ static void arena_free_worker(struct work_struct *work)
 		__free_page(page);
 	}
 
+	if (!llist_empty(&release_spans)) {
+		if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {
+			llist_for_each_safe(pos, t, __llist_del_all(&release_spans)) {
+				s = llist_entry(pos, struct arena_free_span, node);
+				s->release_only = true;
+				llist_add(pos, &arena->free_spans);
+			}
+
+			schedule_work(work);
+			bpf_map_memcg_exit(old_memcg, new_memcg);
+			return;
+		}
+
+		llist_for_each_safe(pos, t, __llist_del_all(&release_spans)) {
+			s = llist_entry(pos, struct arena_free_span, node);
+			page_cnt = s->page_cnt;
+			pgoff = compute_pgoff(arena, s->uaddr);
+			/*
+			 * This range tree operation does not allocate memory,
+			 * and so should never fail regardless of contention
+			 * or memory pressure. This is in contrast to regular
+			 * inserts that _can_ fail under memory pressure and
+			 * force us to defer the free.
+			 */
+			ret = range_tree_make_avail(&arena->rt, pgoff, page_cnt);
+			WARN_ON_ONCE(ret);
+			kfree_nolock(s);
+		}
+		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
+	}
+
 	bpf_map_memcg_exit(old_memcg, new_memcg);
 }
 
-- 
2.55.0

Re: [PATCH bpf-next 3/5] bpf: Fix arena race between page free and alloc leading to incoherency

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 8702 bytes --]

> bpf: Fix arena race between page free and alloc leading to incoherency
>
> Existing arena kfunc code has an underlying race condition
> that can lead to writes being lost from the BPF program's
> point of view:
>
> a) A memory range gets gets freed by operation (1), and its
> range is added back to the arena range tree.
>
> b) A concurrent allocation (2) reallocates the range, and does
> writes to it. Writes from that CPU may follow the stale TLB
> entries into the pages that are about to be freed.
>
> c) (1) invalidates the TLB. The old pages, and any writes done
> to them, are now inaccessible. zap_pages() simlarly removes the
> mappings for userspace threads.
>
> This can be triggered by particularly demanding BPF arena data
> structures that constantly allocate and deallocate memory, like
> hash table allocations.
>
> Solve this ABA problem by preventing range reallocation until
> TLB invalidation/unmapping is complete. First, mark the range
> freed but unavailable. Afterwards, drop the spinlock lock and
> flush the kernel TLB and zap user page tables. Then pick up
> the lock again and mark the ranges as available once again,
> completing the free operation.

There are three typos in the changelog: "gets gets freed" (duplicated
word in paragraph a), "simlarly" (should be "similarly" in paragraph c),
and "drop the spinlock lock" (redundant "lock" in the solution
paragraph).

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index f49b52fa85860..d22b71a791db3 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -76,6 +76,7 @@ struct arena_free_span {
>  	struct llist_node node;
>  	unsigned long uaddr;
>  	u32 page_cnt;
> +	bool release_only;
>  };

[ ... ]

> @@ -881,7 +883,15 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
>  	if (ret)
>  		goto defer;
>
> -	range_tree_set_avail(&arena->rt, pgoff, page_cnt);
> +	ret = range_tree_set_unavail(&arena->rt, pgoff, page_cnt);
> +	if (ret) {
> +		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +		if (ret == -ENOMEM)
> +			goto defer;
> +		WARN_ON_ONCE(ret);
> +		bpf_map_memcg_exit(old_memcg, new_memcg);
> +		return;
> +	}

Does this handle -EAGAIN correctly? range_tree_set_unavail() calls
range_tree_set() with available=false, which calls range_tree_clear().
That function returns -EAGAIN when any overlapping range is already
marked !available:

    while ((rn = range_it_iter_first(rt, first, last))) {
            if (!range_available(rn))
                    return -EAGAIN;
            first = rn->rn_last + 1;
    }

Since only -ENOMEM is filtered here, -EAGAIN reaches WARN_ON_ONCE(ret)
and the free is abandoned. The overlapping !available range can come
from any concurrent free that is in its teardown window (the window this
patch introduces: set_unavail -> unlock -> flush/zap -> relock ->
make_avail), or from a deferred free in arena_free_worker's first loop.

Two CPUs running bpf_arena_free_pages() on overlapping ranges would hit
this: CPU0 freeing pages [0,8) and CPU1 freeing [4,12) is enough. The
partially overlapping case means the non-overlapping remainder is never
torn down, so those pages stay mapped and their range stays allocated.

(Fixed in the next commit, 0b79959d464ca, which adds: "if (ret !=
-EAGAIN) WARN_ON_ONCE(ret);")

>
>  	init_llist_head(&free_pages);
>  	cdata.arena = arena;

[ ... ]

> @@ -911,6 +921,16 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
>  			zap_pages(arena, full_uaddr, 1);
>  		__free_page(page);
>  	}
> +
> +	ret = raw_res_spin_lock_irqsave(&arena->spinlock, flags);
> +	if (ret) {
> +		release_only = true;
> +		goto defer;
> +	}
> +
> +	ret = range_tree_make_avail(&arena->rt, pgoff, page_cnt);
> +	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +	WARN_ON_ONCE(ret);
>  	bpf_map_memcg_exit(old_memcg, new_memcg);

[ ... ]

After this commit, arena_vm_fault() can spuriously fail with SIGSEGV.
Before this commit nothing inserted !available nodes into arena->rt, so
range_tree_clear() in arena_vm_fault() never returned -EAGAIN.

This commit makes every freed range spend a window marked !available.
During that window, a userspace thread that faults on a page in that
range calls range_tree_clear() which returns -EAGAIN, and
arena_vm_fault() does:

    ret = range_tree_clear(&arena->rt, vmf->pgoff, 1);
    if (ret)
            goto out_sigsegv_memcg;

and hands the thread VM_FAULT_SIGSEGV. The window is not short: it
spans flush_tlb_kernel_range() plus zap_pages() (which takes
arena->zap_mutex, arena->lock, and mmap_read_lock of every mapped
process), and if the re-acquire fails the range stays !available until
the irq_work/workqueue round trip completes (release_only span), or
forever if kmalloc_nolock() in the defer path fails.

Should arena_vm_fault() retry on -EAGAIN instead of returning SIGSEGV?
The next commit in the series (0b79959d464ca) adds exactly that:

    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;
    }

Similarly, arena_alloc_pages() and arena_reserve_pages() can now fail
spuriously. is_range_tree_set() does not look at rn->available, so a
fixed-address allocation over a range in its free teardown window passes
the "is this range free" test and then hits:

    ret = range_tree_clear(&arena->rt, pgoff, page_cnt);

which returns -EAGAIN, and arena_alloc_pages() returns 0 (allocation
failure) to the BPF program. arena_reserve_pages() returns -EAGAIN
verbatim instead of the previous 0/-EBUSY/-ENOMEM set.

Unlike the arena_vm_fault() case, the follow-up commit 0b79959d464ca
does not add retry logic here, so fixed-address bpf_arena_alloc_pages()
and bpf_arena_reserve_pages() can fail spuriously (and with a new errno)
while an overlapping free is being torn down.

> @@ -992,28 +1014,51 @@ static void arena_free_worker(struct work_struct *work)
>  	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
>
>  	init_llist_head(&free_pages);
> +	init_llist_head(&teardown_spans);
> +	init_llist_head(&release_spans);
>  	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, list) {
>  		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);
>
> +		if (s->release_only) {
> +			ret = range_tree_make_avail(&arena->rt, pgoff, page_cnt);
> +			WARN_ON_ONCE(ret);
> +			kfree_nolock(s);
> +			continue;
> +		}
> +
> +		kaddr = arena_vm_start + s->uaddr;
> +
> +		ret = range_tree_set_unavail(&arena->rt, pgoff, page_cnt);
> +		if (ret) {
> +			/*
> +			 * An -ENOMEM failure is the same failure mode as in
> +			 * the defer: path of arena_free_pages(). Do not treat
> +			 * the leak as a bug.
> +			 */
> +			if (ret != -ENOMEM)
> +				WARN_ON_ONCE(ret);
> +
> +			kfree_nolock(s);
> +			continue;
> +		}

Same -EAGAIN issue here. range_tree_set_unavail() returns -EAGAIN when
an overlapping range is already marked !available, and only -ENOMEM is
filtered, so -EAGAIN hits WARN_ON_ONCE(ret) and the span is dropped (its
pages are never unmapped/freed and its range stays permanently
allocated, since apply_to_existing_page_range() is skipped by the
continue).

This is reachable without concurrency: a BPF program that calls
bpf_arena_free_pages() or bpf_arena_free_pages_non_sleepable() twice on
the same or overlapping range from non-sleepable context queues two
struct arena_free_span entries on arena->free_spans before the worker
runs. In the worker's first loop, the first span calls
range_tree_set_unavail() successfully; the second span sees the
!available node and gets -EAGAIN, which fires WARN_ON_ONCE (kernel panic
with panic_on_warn) and skips that span's teardown.

Before this commit the same sequence was benign: the loop called
range_tree_set_avail(), whose fast path returned 0 for an
already-available range and never warned.

(Fixed in the next commit, 0b79959d464ca, which replaces this with: "if
(ret != -ENOMEM && ret != -EAGAIN) WARN_ON_ONCE(ret);")

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33603499110

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

[Emil Tsalapatis]

The arena allocation code currently has a race in the fault handler
that can cause userspace threads to write to the wrong arena page.

a) The fault handler removes a range from the range tree to mark the
addresses as allocated, then installs a page A into the kernel page
tables.

b) A concurrent free/reallocation removes A and installs a page B.

c) The fault handler still goes ahead with installing page A in the
page table. The kernel sees page B, while userspace sees page A.

There is no way to protect the PTE installation and the range tree
modification simultaneously, because we cannot nest the synchronization
primitives for their respective critical sections. PTE allocation
may require allocations due to PTE reclamation, and its spinlock
becomes sleepable under PREEMPT_RT. Thus we cannot do this operation
while holding the range tree spinlock. There is no public API for
manually taking this spinlock, so we nest the range tree operation
inside it.

Solve this issue by adjusting the range tree in two steps. First,
mark the address of the page being allocated as unavailable. Then
drop the range spinlock, insert the PTE, take the range spinlock
again, and fully remove it from the range tree. Concurrent free
operations get serialized to before the fault handler, while
it is not possible to allocate the page once it has been reserved.
Concurrent page fault handler calls retry until the page is fully
allocated by the original call.

Also return VM_FAULT_RETRY for transient allocation failures. These
are a) faulting on pages that are temporarily marked unavailable in
the range tree and b) rqspinlock acquisition failures.

Fixes: b8467290edab ("bpf: arena: make arena kfuncs any context safe")
Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>
---
 kernel/bpf/arena.c      | 56 ++++++++++++++++++++++++++++++++---------
 kernel/bpf/range_tree.c | 14 +++++++++++
 kernel/bpf/range_tree.h |  1 +
 3 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
index d22b71a791db..d7006cdb9899 100644
--- a/kernel/bpf/arena.c
+++ b/kernel/bpf/arena.c
@@ -485,18 +485,14 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
 	struct page *page;
 	long kbase, kaddr;
 	unsigned long flags;
+	vm_fault_t ret_fault;
 	int ret;
 
 	kbase = bpf_arena_get_kern_vm_start(arena);
 	kaddr = kbase + (u32)(vmf->address);
 
 	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags))
-		/*
-		 * A failed lock means a possible deadlock was detected. Don't
-		 * return VM_FAULT_RETRY: this handler never took mmap_lock, but
-		 * the fault path would re-take it on retry and deadlock. Fail.
-		 */
-		return VM_FAULT_SIGBUS;
+		goto retry;
 
 	page = vmalloc_to_page((void *)kaddr);
 	if (page) {
@@ -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;
 
@@ -534,15 +538,41 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
 	flush_vmap_cache(kaddr, PAGE_SIZE);
 	bpf_map_memcg_exit(old_memcg, new_memcg);
 out:
-	page_ref_add(page, 1);
+	/* Reserve the page while installing its user PTE without the arena lock. */
+	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
+	ret = range_tree_set_unavail(&arena->rt, vmf->pgoff, 1);
+	bpf_map_memcg_exit(old_memcg, new_memcg);
 	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
-	vmf->page = page;
-	return 0;
+	if (ret) {
+		if (ret == -EAGAIN)
+			goto retry;
+		return VM_FAULT_OOM;
+	}
+
+	ret_fault = vmf_insert_page(vmf->vma, vmf->address, page);
+
+	while (raw_res_spin_lock_irqsave(&arena->spinlock, flags))
+		cond_resched();
+	ret = range_tree_remove_unavail(&arena->rt, vmf->pgoff, 1);
+	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
+	WARN_ON_ONCE(ret);
+	return ret_fault;
 out_sigsegv_memcg:
 	bpf_map_memcg_exit(old_memcg, new_memcg);
 out_sigsegv:
 	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
 	return VM_FAULT_SIGSEGV;
+
+retry:
+
+	/* Only for special cases (GUP/device drivers). */
+	if (!(vmf->flags & FAULT_FLAG_ALLOW_RETRY))
+		return VM_FAULT_SIGBUS;
+
+	if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
+		release_fault_lock(vmf);
+
+	return VM_FAULT_RETRY;
 }
 
 static const struct vm_operations_struct arena_vm_ops = {
@@ -622,7 +652,7 @@ static int arena_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
 	 * of user_vm_start. Set VM_DONTCOPY to prevent arena VMA from
 	 * being copied into the child process on fork.
 	 */
-	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY);
+	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY | VM_MIXEDMAP);
 	vma->vm_ops = &arena_vm_ops;
 	return 0;
 }
@@ -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;
 	}
@@ -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);
diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
index 515e72f054f5..1c23937041a6 100644
--- a/kernel/bpf/range_tree.c
+++ b/kernel/bpf/range_tree.c
@@ -378,6 +378,20 @@ int range_tree_set_unavail(struct range_tree *rt, u32 start, u32 len)
 	return range_tree_set(rt, start, len, false);
 }
 
+int range_tree_remove_unavail(struct range_tree *rt, u32 start, u32 len)
+{
+	u32 last = start + len - 1;
+	struct range_node *rn;
+
+	rn = range_it_iter_first(rt, start, last);
+	if (!rn || rn->available || rn->rn_start != start || rn->rn_last != last)
+		return -EINVAL;
+
+	range_it_remove(rn, rt);
+	kfree_nolock(rn);
+	return 0;
+}
+
 void range_tree_destroy(struct range_tree *rt)
 {
 	struct range_node *rn;
diff --git a/kernel/bpf/range_tree.h b/kernel/bpf/range_tree.h
index aa27edf451bc..4b12ef51cc0b 100644
--- a/kernel/bpf/range_tree.h
+++ b/kernel/bpf/range_tree.h
@@ -16,6 +16,7 @@ void range_tree_destroy(struct range_tree *rt);
 int range_tree_clear(struct range_tree *rt, u32 start, u32 len);
 int range_tree_set_avail(struct range_tree *rt, u32 start, u32 len);
 int range_tree_set_unavail(struct range_tree *rt, u32 start, u32 len);
+int range_tree_remove_unavail(struct range_tree *rt, u32 start, u32 len);
 int range_tree_make_avail(struct range_tree *rt, u32 start, u32 len);
 int is_range_tree_set(struct range_tree *rt, u32 start, u32 len);
 s64 range_tree_find(struct range_tree *rt, u32 len);
-- 
2.55.0

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

[sashiko-bot]

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

[PATCH bpf-next 5/5] selftests/bpf: Add arena allocation race tests

[Emil Tsalapatis]

Add selftests to handle arena page allocation-related races.
Ensure that concurrent frees and nonsleepable/sleepable page
allocations, as well as allocations from userspace, do not
lead to inconsistent or lost data.

Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>
---
 .../selftests/bpf/prog_tests/arena_race.c     | 251 ++++++++++++++++++
 .../testing/selftests/bpf/progs/arena_race.c  | 163 ++++++++++++
 2 files changed, 414 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/arena_race.c
 create mode 100644 tools/testing/selftests/bpf/progs/arena_race.c

diff --git a/tools/testing/selftests/bpf/prog_tests/arena_race.c b/tools/testing/selftests/bpf/prog_tests/arena_race.c
new file mode 100644
index 000000000000..c3a2a4397315
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/arena_race.c
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <sys/syscall.h>
+#include <test_progs.h>
+
+#include "arena_race.skel.h"
+
+struct free_thread_ctx {
+	struct arena_race *skel;
+	int err;
+	__u32 retval;
+};
+
+struct fault_thread_ctx {
+	__u64 *addr;
+	int stop;
+};
+
+static int run_prog(struct bpf_program *prog, const char *name)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+	int err;
+
+	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &opts);
+	return ASSERT_OK(err, name) && ASSERT_OK(opts.retval, name) ? 0 : -1;
+}
+
+/* Trigger the sleepable free page path. */
+static void *run_free_thread(void *arg)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+	struct free_thread_ctx *ctx = arg;
+
+	ctx->skel->bss->target_tid = sys_gettid();
+	ctx->err = bpf_prog_test_run_opts(
+		bpf_program__fd(ctx->skel->progs.free_page), &opts);
+	ctx->retval = opts.retval;
+	return NULL;
+}
+
+/* Continuously fault in the address. */
+static void *fault_reader_thread(void *arg)
+{
+	struct fault_thread_ctx *ctx = arg;
+
+	while (!READ_ONCE(ctx->stop))
+		(void)READ_ONCE(*ctx->addr);
+	return NULL;
+}
+
+static int wait_for(int *p)
+{
+	__u64 deadline = get_time_ns() + 5ULL * 1000 * 1000 * 1000;
+
+	while (!READ_ONCE(*p)) {
+		if (get_time_ns() > deadline)
+			return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static struct arena_race *setup_arena(__u64 **addr)
+{
+	struct arena_race *skel;
+	size_t arena_sz;
+	char *base;
+	int err;
+
+	skel = arena_race__open();
+	if (!ASSERT_OK_PTR(skel, "open"))
+		return NULL;
+
+	err = arena_race__load(skel);
+	if (!ASSERT_OK(err, "load"))
+		goto err_out;
+	err = arena_race__attach(skel);
+	if (!ASSERT_OK(err, "attach"))
+		goto err_out;
+
+	if (run_prog(skel->progs.alloc_old, "alloc_old"))
+		goto err_out;
+	if (skel->bss->skip) {
+		test__skip();
+		goto err_out;
+	}
+
+	base = bpf_map__initial_value(skel->maps.arena, &arena_sz);
+	if (!ASSERT_OK_PTR(base, "arena_base"))
+		goto err_out;
+	*addr = (__u64 *)(base + getpagesize());
+	if (!ASSERT_EQ((unsigned long)skel->bss->ptr, (unsigned long)*addr,
+		       "arena_ptr"))
+		goto err_out;
+	return skel;
+
+err_out:
+	arena_race__destroy(skel);
+	return NULL;
+}
+
+static void test_free_before_flush(bool deferred)
+{
+	struct free_thread_ctx ctx = {};
+	struct arena_race *skel;
+	pthread_t thread;
+	__u64 *addr;
+	bool thread_created = false, flush_seen = false, completed = false;
+	int err;
+
+	skel = setup_arena(&addr);
+	if (!skel)
+		return;
+
+	/* Pause during a TLB flush to widen the race window. */
+	skel->bss->pause_on_flush = 1;
+
+	if (deferred) {
+		/*
+		 * Test the nonsleepable free path that gets
+		 * deferred to a worker in the kernel. We do
+		 * so by triggering the arena operation from
+		 * a nonsleepable tracepoint context.
+		 */
+		skel->bss->trigger_pid_tgid =
+			((__u64)getpid() << 32) | (__u32)sys_gettid();
+		skel->bss->trigger_syscall = SYS_getpgid;
+		skel->bss->deferred_free = 1;
+		if (!ASSERT_GE(syscall(SYS_getpgid, 0), 0, "deferred_free"))
+			goto release;
+	} else {
+		/*
+		 * Test the sleepable arena free path through a
+		 * syscall test prog.
+		 */
+		ctx.skel = skel;
+		err = pthread_create(&thread, NULL, run_free_thread, &ctx);
+		if (!ASSERT_OK(err, "pthread_create")) {
+			skel->bss->release = 1;
+			goto out;
+		}
+		thread_created = true;
+	}
+
+	/* Wait until the worker thread triggers a flush. */
+	err = wait_for(&skel->bss->flush_entered);
+	if (!ASSERT_OK(err, "flush_entered"))
+		goto release;
+
+	flush_seen = true;
+
+	/* Force a reallocation during the flush. */
+	run_prog(skel->progs.try_realloc, "realloc_before_flush");
+	ASSERT_NULL(skel->bss->realloc_ptr, "realloc_before_flush");
+
+release:
+	skel->bss->release = 1;
+	if (thread_created) {
+		ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
+		thread_created = false;
+		completed = ASSERT_OK(ctx.err, "free_run") &&
+			    ASSERT_OK(ctx.retval, "free_retval");
+	} else if (skel->bss->target_tid) {
+		err = wait_for(&skel->bss->worker_exited);
+		completed = ASSERT_OK(err, "worker_exited");
+	}
+	ASSERT_FALSE(skel->bss->timed_out, "flush_timed_out");
+
+	if (flush_seen && completed &&
+	    !run_prog(skel->progs.try_realloc, "realloc_after_flush")) {
+		ASSERT_EQ((unsigned long)skel->bss->realloc_ptr,
+			  (unsigned long)addr, "realloc_after_flush");
+		ASSERT_EQ(*addr, skel->rodata->new_marker, "new_marker");
+	}
+out:
+
+	if (thread_created)
+		pthread_join(thread, NULL);
+
+	arena_race__destroy(skel);
+}
+
+/*
+ * Force a race between a faulting thread in userspace and a
+ * free operation on the arena.
+ */
+static void test_fault_free_realloc(void)
+{
+	struct fault_thread_ctx fault = {};
+	struct arena_race *skel;
+	pthread_t fault_thread;
+	bool fault_created = false;
+	__u64 *addr;
+	__u64 expected, value;
+	int err, i;
+
+	skel = setup_arena(&addr);
+	if (!skel)
+		return;
+
+	skel->bss->realloc_after_free = 1;
+
+	fault.addr = addr;
+	err = pthread_create(&fault_thread, NULL, fault_reader_thread, &fault);
+	if (!ASSERT_OK(err, "pthread_create_fault"))
+		goto out;
+	fault_created = true;
+
+	for (i = 0; i < 1000 && !READ_ONCE(fault.stop); i++) {
+		LIBBPF_OPTS(bpf_test_run_opts, opts);
+
+		err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.free_page),
+					     &opts);
+		if (err) {
+			ASSERT_OK(err, "free_realloc");
+			break;
+		}
+		if (opts.retval) {
+			ASSERT_OK(opts.retval, "free_realloc");
+			break;
+		}
+		value = *addr;
+		expected = skel->bss->current_marker;
+		if (value != expected) {
+			ASSERT_EQ(value, expected, "marker_after_realloc");
+			break;
+		}
+	}
+
+	WRITE_ONCE(fault.stop, 1);
+	if (fault_created) {
+		ASSERT_OK(pthread_join(fault_thread, NULL), "pthread_join_fault");
+		fault_created = false;
+	}
+	ASSERT_GE(i, 1, "race_iterations");
+out:
+	WRITE_ONCE(fault.stop, 1);
+	if (fault_created)
+		pthread_join(fault_thread, NULL);
+	arena_race__destroy(skel);
+}
+
+void serial_test_arena_race(void)
+{
+	if (test__start_subtest("free_before_flush"))
+		test_free_before_flush(false);
+	if (test__start_subtest("deferred_free_before_flush"))
+		test_free_before_flush(true);
+	if (test__start_subtest("fault_free_realloc"))
+		test_fault_free_realloc();
+}
diff --git a/tools/testing/selftests/bpf/progs/arena_race.c b/tools/testing/selftests/bpf/progs/arena_race.c
new file mode 100644
index 000000000000..df5b54ef5b4c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/arena_race.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define BPF_NO_KFUNC_PROTOTYPES
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_experimental.h"
+#include <bpf_arena_common.h>
+
+const volatile __u64 old_marker = 0x1111222233334444ULL;
+const volatile __u64 new_marker = 0x5555666677778888ULL;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARENA);
+	__uint(map_flags, BPF_F_MMAPABLE);
+	__uint(max_entries, 4);
+#ifdef __TARGET_ARCH_arm64
+	__ulong(map_extra, 0x1ull << 32);
+#else
+	__ulong(map_extra, 0x1ull << 44);
+#endif
+} arena SEC(".maps");
+
+#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) || defined(BPF_ARENA_FORCE_ASM)
+bool skip;
+#else
+bool skip = true;
+#endif
+
+void __arena *ptr;
+void __arena *realloc_ptr;
+bool realloc_after_free;
+int free_started;
+__u64 current_marker;
+__u64 marker_seq;
+
+int target_tid;
+int pause_on_flush;
+int flush_entered;
+int release;
+int timed_out;
+
+__u64 trigger_pid_tgid;
+long trigger_syscall;
+int deferred_free;
+int worker_armed;
+int worker_exited;
+
+static __always_inline void wait_for_release(void)
+{
+	while (!*(volatile int *)&release && can_loop)
+		;
+	if (!*(volatile int *)&release)
+		timed_out = 1;
+}
+
+SEC("syscall")
+int alloc_old(void *ctx)
+{
+#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) || defined(BPF_ARENA_FORCE_ASM)
+	__u64 __arena *p;
+	char __arena *base = arena_base(&arena);
+
+	realloc_ptr = NULL;
+	ptr = bpf_arena_alloc_pages(&arena, base + __PAGE_SIZE, 1,
+				    NUMA_NO_NODE, 0);
+	if (!ptr)
+		return 1;
+	p = (__u64 __arena *)ptr;
+	*p = old_marker;
+	current_marker = old_marker;
+#endif
+	return 0;
+}
+
+SEC("syscall")
+int free_page(void *ctx)
+{
+#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) || defined(BPF_ARENA_FORCE_ASM)
+	__u64 __arena *p;
+	__u64 marker;
+
+	if (!ptr)
+		return 1;
+	free_started = 1;
+	bpf_arena_free_pages(&arena, ptr, 1);
+	if (!realloc_after_free)
+		return 0;
+
+	marker = new_marker + ++marker_seq;
+	realloc_ptr = bpf_arena_alloc_pages(&arena, ptr, 1, NUMA_NO_NODE, 0);
+	if (realloc_ptr)
+		ptr = realloc_ptr;
+	else
+		realloc_ptr = ptr;
+	p = (__u64 __arena *)ptr;
+	*p = marker;
+	current_marker = marker;
+#endif
+	return 0;
+}
+
+SEC("syscall")
+int try_realloc(void *ctx)
+{
+#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) || defined(BPF_ARENA_FORCE_ASM)
+	__u64 __arena *p;
+
+	realloc_ptr = bpf_arena_alloc_pages(&arena, ptr, 1, NUMA_NO_NODE, 0);
+	if (realloc_ptr) {
+		ptr = realloc_ptr;
+		p = (__u64 __arena *)realloc_ptr;
+		*p = new_marker;
+		current_marker = new_marker;
+	}
+#endif
+	return 0;
+}
+
+SEC("tp_btf/sys_enter")
+int BPF_PROG(deferred_free_prog, struct pt_regs *regs, long id)
+{
+#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) || defined(BPF_ARENA_FORCE_ASM)
+	if (!deferred_free || bpf_get_current_pid_tgid() != trigger_pid_tgid ||
+	    id != trigger_syscall)
+		return 0;
+
+	deferred_free = 0;
+	/* The worker can run on another CPU before the kfunc returns. */
+	worker_armed = 1;
+	bpf_arena_free_pages(&arena, ptr, 1);
+#endif
+	return 0;
+}
+
+SEC("fentry/arena_free_worker")
+int BPF_PROG(trace_free_worker, struct work_struct *work)
+{
+	if (worker_armed && !target_tid)
+		target_tid = (__u32)bpf_get_current_pid_tgid();
+	return 0;
+}
+
+SEC("fexit/arena_free_worker")
+int BPF_PROG(trace_free_worker_ret, struct work_struct *work)
+{
+	if ((__u32)bpf_get_current_pid_tgid() == target_tid)
+		worker_exited = 1;
+	return 0;
+}
+
+SEC("fentry/flush_tlb_kernel_range")
+int BPF_PROG(trace_flush, unsigned long start, unsigned long end)
+{
+	if (!pause_on_flush ||
+	    (__u32)bpf_get_current_pid_tgid() != target_tid)
+		return 0;
+	flush_entered = 1;
+	wait_for_release();
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.55.0

Re: [PATCH bpf-next 5/5] selftests/bpf: Add arena allocation race tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The selftest fails to load on ARM64 because `flush_tlb_kernel_range` is a static inline function.
- [Medium] Busy-waiting in an fentry BPF program for a userspace thread guarantees a test timeout on UP non-preemptible kernels.
--

commit 03dd0a4c6fda5e1c4877d7ca2aff23509f39c69f
Author: Emil Tsalapatis <emil@etsalapatis.com>
Subject: selftests/bpf: Add arena allocation race tests

This commit adds selftests to handle concurrent accesses to arena
allocations, including sleepable and non-sleepable page allocation
paths, to ensure no inconsistent or lost data occurs.

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_race.c b/tools/testing/selftests/bpf/prog_tests/arena_race.c
> index 0000000000000..c3a2a4397315a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_race.c
[ ... ]
> +	/* Force a reallocation during the flush. */
> +	run_prog(skel->progs.try_realloc, "realloc_before_flush");
> +	ASSERT_NULL(skel->bss->realloc_ptr, "realloc_before_flush");
> +
> +release:
> +	skel->bss->release = 1;

[Severity: Medium]
Will this test deterministically timeout or deadlock on uniprocessor systems
when CONFIG_PREEMPT_NONE is set?

The userspace thread needs CPU time to execute this assignment, but the BPF
program may be hogging the only CPU by spinning in wait_for_release().

[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/arena_race.c b/tools/testing/selftests/bpf/progs/arena_race.c
> index 0000000000000..df5b54ef5b4c0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/arena_race.c
[ ... ]
> +static __always_inline void wait_for_release(void)
> +{
> +	while (!*(volatile int *)&release && can_loop)
> +		;

[Severity: Medium]
Could this tight loop starve the CPU if preemption is disabled during the
fentry program?

On uniprocessor non-preemptible kernels, the userspace thread cannot be
scheduled to set the release variable while the BPF program runs. The loop
terminates only when can_loop runs out, which sets timed_out = 1 and causes
the test to fail.

> +	if (!*(volatile int *)&release)
> +		timed_out = 1;
> +}
[ ... ]
> +SEC("fentry/flush_tlb_kernel_range")
> +int BPF_PROG(trace_flush, unsigned long start, unsigned long end)

[Severity: Medium]
Does this fentry program fail to load on ARM64?

On ARM64, flush_tlb_kernel_range is defined as a static inline function in
<asm/tlbflush.h>. Because inline functions do not generate BTF IDs in
vmlinux, libbpf will fail to resolve the attach target during
arena_race__load(skel), which breaks the selftest CI on ARM64.

> +{
> +	if (!pause_on_flush ||
> +	    (__u32)bpf_get_current_pid_tgid() != target_tid)
> +		return 0;
> +	flush_entered = 1;
> +	wait_for_release();
> +	return 0;
> +}

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

Re: [PATCH bpf-next 5/5] selftests/bpf: Add arena allocation race tests

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_race.c b/tools/testing/selftests/bpf/prog_tests/arena_race.c
> new file mode 100644
> index 0000000000000..c3a2a4397315a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_race.c

[ ... ]

> +static void test_free_before_flush(bool deferred)
> +{
> +	struct free_thread_ctx ctx = {};
> +	struct arena_race *skel;
> +	pthread_t thread;
> +	__u64 *addr;
> +	bool thread_created = false, flush_seen = false, completed = false;
> +	int err;

[ ... ]

> +release:
> +	skel->bss->release = 1;
> +	if (thread_created) {
> +		ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
> +		thread_created = false;

[ ... ]

> +out:
> +
> +	if (thread_created)
> +		pthread_join(thread, NULL);
> +
> +	arena_race__destroy(skel);
> +}

This isn't a bug, but could the join at the out: label be dropped? On both
paths reaching out: the thread_created flag is already false - the goto
precedes any assignment to thread_created, and the fall-through clears it at
the release: block after joining.

> +/*
> + * Force a race between a faulting thread in userspace and a
> + * free operation on the arena.
> + */
> +static void test_fault_free_realloc(void)
> +{
> +	struct fault_thread_ctx fault = {};
> +	struct arena_race *skel;
> +	pthread_t fault_thread;
> +	bool fault_created = false;
> +	__u64 *addr;
> +	__u64 expected, value;
> +	int err, i;

[ ... ]

> +	fault.addr = addr;
> +	err = pthread_create(&fault_thread, NULL, fault_reader_thread, &fault);
> +	if (!ASSERT_OK(err, "pthread_create_fault"))
> +		goto out;
> +	fault_created = true;
> +
> +	for (i = 0; i < 1000 && !READ_ONCE(fault.stop); i++) {

This isn't a bug, but was fault_reader_thread() meant to set fault.stop on
some condition (e.g. an unexpected value)? As written nothing but this thread
writes fault.stop, so the loop guard is always true.

> +		LIBBPF_OPTS(bpf_test_run_opts, opts);
> +
> +		err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.free_page),
> +					     &opts);
> +		if (err) {
> +			ASSERT_OK(err, "free_realloc");
> +			break;
> +		}
> +		if (opts.retval) {
> +			ASSERT_OK(opts.retval, "free_realloc");
> +			break;
> +		}
> +		value = *addr;
> +		expected = skel->bss->current_marker;
> +		if (value != expected) {
> +			ASSERT_EQ(value, expected, "marker_after_realloc");
> +			break;
> +		}
> +	}
> +
> +	WRITE_ONCE(fault.stop, 1);
> +	if (fault_created) {
> +		ASSERT_OK(pthread_join(fault_thread, NULL), "pthread_join_fault");
> +		fault_created = false;
> +	}
> +	ASSERT_GE(i, 1, "race_iterations");

This isn't a bug, but is race_iterations meant to assert something stronger,
such as that at least one iteration observed the free/realloc race? As
written i is 1000 on the normal path and 0 only when another assertion has
already failed.

> +out:
> +	WRITE_ONCE(fault.stop, 1);
> +	if (fault_created)
> +		pthread_join(fault_thread, NULL);
> +	arena_race__destroy(skel);
> +}

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/arena_race.c b/tools/testing/selftests/bpf/progs/arena_race.c
> new file mode 100644
> index 0000000000000..df5b54ef5b4c0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/arena_race.c

[ ... ]

> +void __arena *ptr;
> +void __arena *realloc_ptr;
> +bool realloc_after_free;
> +int free_started;
> +__u64 current_marker;

[ ... ]

> +SEC("syscall")
> +int free_page(void *ctx)
> +{
> +#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) || defined(BPF_ARENA_FORCE_ASM)
> +	__u64 __arena *p;
> +	__u64 marker;
> +
> +	if (!ptr)
> +		return 1;
> +	free_started = 1;
> +	bpf_arena_free_pages(&arena, ptr, 1);

This isn't a bug, but is free_started still needed? Nothing in the BPF
object or the userspace test appears to read it - could it be dropped?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33603499110