Re: [PATCH 2/3] bpf: Add sleepable arena page allocation path

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b3cd28d9e3b5..fcb21c29aef2 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -710,6 +710,12 @@ void bpf_map_free_internal_structs(struct bpf_map *map, void *obj);
>  int bpf_dynptr_from_file_sleepable(struct file *file, u32 flags,
>  				   struct bpf_dynptr *ptr__uninit);
>
> +static inline bool is_bpf_alloc_nonsleepable(void)
> +{
> +	return preempt_count() > 0 || irqs_disabled() ||
> +		IS_ENABLED(CONFIG_PREEMPT_RT);
> +}
> +
>  #if defined(CONFIG_MMU) && defined(CONFIG_64BIT)
>  void *bpf_arena_alloc_pages_non_sleepable(void *p__map, void *addr__ign, u32 page_cnt, int node_id,
>  					  u64 flags);
> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index da356989786a..126ddae763f9 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -683,8 +683,73 @@ static int arena_adjust_tree(struct bpf_arena *arena, long uaddr, long page_cnt,
>  	return range_tree_clear(&arena->rt, *pgoff, page_cnt);
>  }
>
> -static long arena_alloc_pages_internal(struct bpf_arena *arena, long page_cnt,
> -		long uaddr, long pgoff, int node_id, bool sleepable)
> +static long arena_alloc_pages_sleepable(struct bpf_arena *arena, long page_cnt,
> +		long uaddr, long pgoff, int node_id)
> +{
> +	u64 kern_vm_start = bpf_arena_get_kern_vm_start(arena);
> +	struct apply_range_data data;
> +	struct page **pages = NULL;
> +	unsigned long flags;
> +	u32 uaddr32;
> +	int ret, i;
> +
> +	pages = kvcalloc(page_cnt, sizeof(struct page *), GFP_KERNEL_ACCOUNT);
> +	if (!pages)
> +		return 0;
> +
> +	ret = bpf_map_alloc_pages(&arena->map, node_id, page_cnt, pages);
> +	if (ret) {
> +		kvfree(pages);
> +		return 0;
> +	}
> +
> +	data.i = 0;
> +	data.pages = pages;
> +	data.arena = arena;
> +
> +	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags))
> +		goto out_free_pages;
> +
> +	ret = arena_adjust_tree(arena, uaddr, page_cnt, &pgoff);
> +	if (ret) {
> +		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +		goto out_free_pages;
> +	}
> +
> +	uaddr32 = (u32)(arena->user_vm_start + pgoff * PAGE_SIZE);
> +
> +	ret = apply_to_page_range(&init_mm, kern_vm_start + uaddr32,
> +			page_cnt << PAGE_SHIFT, apply_range_set_cb, &data);
> +
> +	if (ret)
> +		goto out_unmap;
> +
> +	flush_vmap_cache(kern_vm_start + uaddr32, page_cnt << PAGE_SHIFT);
> +	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +
> +	kvfree(pages);
> +
> +	return clear_lo32(arena->user_vm_start) + uaddr32;
> +
> +out_unmap:
> +	/* data.i pages were mapped, undo only those who failed to map */
> +	flush_vmap_cache(kern_vm_start + uaddr32, data.i << PAGE_SHIFT);
> +	range_tree_set(&arena->rt, pgoff + data.i, page_cnt - data.i);
> +
> +	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +
> +	arena_free_pages(arena, uaddr32, data.i, true);

This isn't a bug, but does the comment describe the right thing?

The comment says "undo only those who failed to map", but the code below
unmaps the pages that succeeded:

  arena_free_pages(arena, uaddr32, data.i, true);

This unmaps exactly data.i pages starting at uaddr32, which are the ones
that apply_range_set_cb() installed (data.i is incremented once per
successful PTE). The pages that failed to map get freed in the
out_free_pages loop below:

  for (i = data.i; i < page_cnt; i++)
          __free_page(pages[i]);

Should the comment say something like "data.i pages were mapped, undo
those" instead?

> +
> +out_free_pages:
> +	for (i = data.i; i < page_cnt; i++)
> +		__free_page(pages[i]);
> +
> +	kvfree(pages);
> +	return 0;
> +}
> +
> +static long arena_alloc_pages_non_sleepable(struct bpf_arena *arena, long page_cnt,
> +		long uaddr, long pgoff, int node_id)
>  {
>  	u64 kern_vm_start = bpf_arena_get_kern_vm_start(arena);
>  	struct apply_range_data data;

[ ... ]

> @@ -802,7 +867,10 @@ static long arena_alloc_pages(struct bpf_arena *arena, long uaddr, long page_cnt
>
>  	bpf_map_memcg_enter(&arena->map, &old_memcg, &new_memcg);
>
> -	addr = arena_alloc_pages_internal(arena, page_cnt, uaddr, pgoff, node_id, sleepable);
> +	if (sleepable && !is_bpf_alloc_nonsleepable())
> +		addr = arena_alloc_pages_sleepable(arena, page_cnt, uaddr, pgoff, node_id);
> +	else
> +		addr = arena_alloc_pages_non_sleepable(arena, page_cnt, uaddr, pgoff, node_id);

Can this dispatch block while in an RCU read-side critical section?

The sleepable parameter is hardcoded to true by the kfunc entry point
in kernel/bpf/arena.c:

  void *bpf_arena_alloc_pages(struct bpf_map *map, void *addr__ign,
                              u32 page_cnt, int node_id) {
    return (void *)arena_alloc_pages(arena, (long)addr__ign,
                                     page_cnt, node_id, true);
  }

and that kfunc is registered without KF_SLEEPABLE:

  BTF_ID_FLAGS(func, bpf_arena_alloc_pages,
               KF_ARENA_RET | KF_ARENA_ARG2 | KF_SPINLOCK_SAFE)

The verifier checks in kernel/bpf/verifier.c specifically prevent
non-sleepable programs from calling sleepable kfuncs:

  sleepable = bpf_is_kfunc_sleepable(&meta);
  if (sleepable && !in_sleepable(env)) {
          verbose(env, "program must be sleepable to call sleepable "
                  "kfunc %s\n", func_name);
          return -EACCES;
  }

but that check never triggers because bpf_arena_alloc_pages() is not
tagged KF_SLEEPABLE. So non-sleepable programs can call it.

is_bpf_alloc_nonsleepable() checks for:

  preempt_count() > 0 || irqs_disabled() || IS_ENABLED(CONFIG_PREEMPT_RT)

That detects hardirq/softirq/NMI/preempt-disabled/IRQ-disabled/RT
contexts, but it does not detect the RCU read-side critical section
that every non-sleepable BPF program runs inside.

Non-sleepable trampoline programs (fentry/fexit/fmod_ret/raw_tp/
struct_ops) enter via __bpf_prog_enter() in kernel/bpf/trampoline.c,
which calls rcu_read_lock_dont_migrate():

  rcu_read_lock_dont_migrate() {
    if (IS_ENABLED(CONFIG_PREEMPT_RCU))
            migrate_disable();
    rcu_read_lock();
  }

On CONFIG_PREEMPT_RCU=y kernels (the default for CONFIG_PREEMPT_DYNAMIC
distro kernels), rcu_read_lock() is __rcu_read_lock() which only
increments current->rcu_read_lock_nesting, and migrate_disable() uses
a scoped guard(preempt) so it does not leave preempt_count() elevated.

So with CONFIG_PREEMPT_RT=n, is_bpf_alloc_nonsleepable() returns false
and arena_alloc_pages_sleepable() is selected.

arena_alloc_pages_sleepable() then calls:

  kvcalloc(page_cnt, sizeof(struct page *), GFP_KERNEL_ACCOUNT)
  bpf_map_alloc_pages() -> alloc_pages_node(..., GFP_KERNEL, ...)
  arena_free_pages() -> mutex_lock() and flush_tlb_kernel_range()

All of these can sleep. Documentation/RCU/whatisRCU.rst states "It is
illegal to block while in an RCU read-side critical section" even under
CONFIG_PREEMPT_RCU. Being preempted is permitted, but voluntarily
blocking is not.

The blocked reader is queued on rnp->blkd_tasks, so every concurrent
synchronize_rcu() on the system waits for the duration of the direct
reclaim. Under memory pressure (which the commit message says triggers
this path for "heavy BPF arena users like scx"), this yields unbounded
RCU grace-period extension, RCU CPU stall warnings, and delayed freeing
kernel-wide. The task also blocks with current->migration_disabled set
and rq->nr_pinned elevated, which stalls CPU hotplug for the same
duration.

The in-tree selftests confirm that non-sleepable programs are accepted:
tools/testing/selftests/bpf/progs/verifier_arena.c has several
SEC("socket") __success programs calling bpf_arena_alloc_pages(), and
tools/testing/selftests/bpf/progs/struct_ops_arena.c shows non-sleepable
SEC("struct_ops/...") callbacks bound to an arena map.

Should the sleepability decision be based on the verifier-known program
context rather than this runtime heuristic? For example, tagging
bpf_arena_alloc_pages() with KF_SLEEPABLE and adding a separate
non-sleepable kfunc, or plumbing the prog's sleepable flag down to
arena_alloc_pages()?

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6874ba1424af..700ed7a99579 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -596,15 +596,9 @@ static void bpf_map_release_memcg(struct bpf_map *map)
>  }
>  #endif
>
> -static bool can_alloc_pages(void)
> -{
> -	return preempt_count() == 0 && !irqs_disabled() &&
> -		!IS_ENABLED(CONFIG_PREEMPT_RT);
> -}
> -
>  static struct page *__bpf_alloc_page(int nid)
>  {
> -	if (!can_alloc_pages())
> +	if (is_bpf_alloc_nonsleepable())
>  		return alloc_pages_nolock(__GFP_ACCOUNT, nid, 0);
>
>  	return alloc_pages_node(nid,


---
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/32709208315