BPF List
 help / color / mirror / Atom feed
From: Ihor Solodrai <ihor.solodrai@linux.dev>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Eduard Zingerman <eddyz87@gmail.com>,
	Emil Tsalapatis <emil@etsalapatis.com>,
	kkd@meta.com, kernel-team@meta.com
Subject: Re: [PATCH bpf-next v1 11/14] bpf: Replace arena kfunc argument flags with suffixes
Date: Wed, 26 Aug 2026 13:42:29 -0700	[thread overview]
Message-ID: <49226345-d2e4-4618-8c9d-9dd1d9b0ff4b@linux.dev> (raw)
In-Reply-To: <20260821233516.3426127-12-memxor@gmail.com>

On 2026-08-21 4:35 p.m., Kumar Kartikeya Dwivedi wrote:
> The arena allocation kfuncs still identify pointer arguments with
> KF_ARENA_ARG2. These flags cover only the first two parameters and
> duplicate the __arena suffix mechanism used by other kfuncs.

This is a little petty, but the paragraph mischaracterizes what
happened :) It's not like the flags duplicate an existing mechanism
for an unknown/dumb reason. There was no __arena and it was the only
mechanism until very recently.

Rephrase please?

> 
> Annotate the optional allocation address with __arena__nullable. Mark the
> free and reserve addresses with __arena so a valid address whose low 32
> bits are zero is rebased unconditionally instead of becoming NULL.
> 
> The JIT now passes kernel arena addresses to these kfuncs. Translate them
> back to the lower-32-bit user addresses expected by the existing arena
> helpers by subtracting kern_vm_start. This preserves allocation-anywhere,
> freeing the first page of a 4 GiB arena, and reservation at address zero.
> 
> Drop KF_ARENA_ARG1 and KF_ARENA_ARG2 from the kernel interface and remove
> the flags from the arena kfunc sets. KF_ARENA_RET remains responsible for
> annotating the allocation return value.
> 
> Keep the affected selftests synchronized with the conversion. Associate
> an arena before the iterator map-pointer failures so they still reach the
> intended diagnostics, account for the extra nullable branch in JIT labels,
> and treat 1ULL << 32 as the same allocation-anywhere request as NULL after
> the required 32-bit truncation.

It's been brought to my attention a few times [1], that we (the linux
contributors) would strongly prefer the commit messages to explain
*why* the change is being made, instead of *what*.

And the llms tend to do the exact opposite.

This applies to other patches in the series as well, particularly the
cover letter.

[1] 
https://lore.kernel.org/bpf/20260730212613.GJamvBdRJ5H98hTWS8@fat_crate.local/

> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>   include/linux/btf.h                           |  2 -
>   kernel/bpf/arena.c                            | 40 ++++++++++++++-----
>   .../selftests/bpf/progs/arena_kfunc_jit.c     | 16 ++++----
>   .../selftests/bpf/progs/verifier_arena.c      |  6 +++
>   .../bpf/progs/verifier_arena_large.c          |  4 +-
>   5 files changed, 46 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 89d5a5c4f117..65e5f11dc27e 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -76,8 +76,6 @@
>   #define KF_RCU_PROTECTED (1 << 11) /* kfunc should be protected by rcu cs when they are invoked */
>   #define KF_FASTCALL     (1 << 12) /* kfunc supports bpf_fastcall protocol */
>   #define KF_ARENA_RET    (1 << 13) /* kfunc returns an arena pointer */
> -#define KF_ARENA_ARG1   (1 << 14) /* kfunc takes an arena pointer as its first argument */
> -#define KF_ARENA_ARG2   (1 << 15) /* kfunc takes an arena pointer as its second argument */
>   #define KF_IMPLICIT_ARGS (1 << 16) /* kfunc has implicit arguments supplied by the verifier */
>   #define KF_SPINLOCK_SAFE (1 << 17) /* kfunc is allowed inside bpf_spin_lock-ed region */
>   
> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 7b6847200b43..6c34a0d34b3f 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -1044,18 +1044,28 @@ static void arena_free_irq(struct irq_work *iw)
>   	schedule_work(&arena->free_work);
>   }
>   
> +static long arena_kaddr_to_uaddr(struct bpf_arena *arena, const void *addr)
> +{
> +	if (!addr)
> +		return 0;
> +
> +	return (long)addr - bpf_arena_get_kern_vm_start(arena);
> +}
> +
>   __bpf_kfunc_start_defs();
>   
> -__bpf_kfunc void *bpf_arena_alloc_pages(void *p__map, void *addr__ign, u32 page_cnt,
> -					int node_id, u64 flags)
> +__bpf_kfunc void *bpf_arena_alloc_pages(void *p__map, void *addr__arena__nullable,
> +					u32 page_cnt, int node_id, u64 flags)
>   {
>   	struct bpf_map *map = p__map;
>   	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +	long addr;
>   
>   	if (map->map_type != BPF_MAP_TYPE_ARENA || flags || !page_cnt)
>   		return NULL;
>   
> -	return (void *)arena_alloc_pages(arena, (long)addr__ign, page_cnt, node_id, true);
> +	addr = arena_kaddr_to_uaddr(arena, addr__arena__nullable);
> +	return (void *)arena_alloc_pages(arena, addr, page_cnt, node_id, true);
>   }
>   
>   void *bpf_arena_alloc_pages_non_sleepable(void *p__map, void *addr__ign, u32 page_cnt,
> @@ -1082,14 +1092,20 @@ void *bpf_arena_alloc_pages_sleepable(void *p__map, void *addr__ign, u32 page_cn
>   	return (void *)arena_alloc_pages(arena, (long)addr__ign, page_cnt, node_id, true);
>   }
>   
> -__bpf_kfunc void bpf_arena_free_pages(void *p__map, void *ptr__ign, u32 page_cnt)
> +/*
> + * A valid arena address can have zero low 32 bits, so ptr must be rebased
> + * unconditionally instead of being treated as nullable.
> + */
> +__bpf_kfunc void bpf_arena_free_pages(void *p__map, void *ptr__arena, u32 page_cnt)
>   {
>   	struct bpf_map *map = p__map;
>   	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +	long ptr;
>   
> -	if (map->map_type != BPF_MAP_TYPE_ARENA || !page_cnt || !ptr__ign)
> +	if (map->map_type != BPF_MAP_TYPE_ARENA || !page_cnt)
>   		return;
> -	arena_free_pages(arena, (long)ptr__ign, page_cnt, true);
> +	ptr = arena_kaddr_to_uaddr(arena, ptr__arena);
> +	arena_free_pages(arena, ptr, page_cnt, true);
>   }
>   
>   void bpf_arena_free_pages_non_sleepable(void *p__map, void *ptr__ign, u32 page_cnt)
> @@ -1102,10 +1118,11 @@ void bpf_arena_free_pages_non_sleepable(void *p__map, void *ptr__ign, u32 page_c
>   	arena_free_pages(arena, (long)ptr__ign, page_cnt, false);
>   }
>   
> -__bpf_kfunc int bpf_arena_reserve_pages(void *p__map, void *ptr__ign, u32 page_cnt)
> +__bpf_kfunc int bpf_arena_reserve_pages(void *p__map, void *ptr__arena, u32 page_cnt)
>   {
>   	struct bpf_map *map = p__map;
>   	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +	long ptr;
>   
>   	if (map->map_type != BPF_MAP_TYPE_ARENA)
>   		return -EINVAL;
> @@ -1113,14 +1130,15 @@ __bpf_kfunc int bpf_arena_reserve_pages(void *p__map, void *ptr__ign, u32 page_c
>   	if (!page_cnt)
>   		return 0;
>   
> -	return arena_reserve_pages(arena, (long)ptr__ign, page_cnt);
> +	ptr = arena_kaddr_to_uaddr(arena, ptr__arena);
> +	return arena_reserve_pages(arena, ptr, page_cnt);

This patch does a couple unrelated things clumped together.

There is a kfunc arena arg identification mechanism change: flag to
suffix. And there is behavioral change: rebasing the pointer etc.

I think it'd be easier to review and would lead to a cleaner git
history if these were separated. In particular, I'd expect the flag
deprecation itself to not require any changes in the jit tests.

>   }
>   __bpf_kfunc_end_defs();
>   
>   BTF_KFUNCS_START(arena_kfuncs)
> -BTF_ID_FLAGS(func, bpf_arena_alloc_pages, KF_ARENA_RET | KF_ARENA_ARG2 | KF_SPINLOCK_SAFE)
> -BTF_ID_FLAGS(func, bpf_arena_free_pages, KF_ARENA_ARG2 | KF_SPINLOCK_SAFE)
> -BTF_ID_FLAGS(func, bpf_arena_reserve_pages, KF_ARENA_ARG2 | KF_SPINLOCK_SAFE)
> +BTF_ID_FLAGS(func, bpf_arena_alloc_pages, KF_ARENA_RET | KF_SPINLOCK_SAFE)
> +BTF_ID_FLAGS(func, bpf_arena_free_pages, KF_SPINLOCK_SAFE)
> +BTF_ID_FLAGS(func, bpf_arena_reserve_pages, KF_SPINLOCK_SAFE)
>   BTF_KFUNCS_END(arena_kfuncs)
>   
>   static const struct btf_kfunc_id_set common_kfunc_set = {
> diff --git a/tools/testing/selftests/bpf/progs/arena_kfunc_jit.c b/tools/testing/selftests/bpf/progs/arena_kfunc_jit.c
> index b5a01cbc33a7..c9af35c683b3 100644
> --- a/tools/testing/selftests/bpf/progs/arena_kfunc_jit.c
> +++ b/tools/testing/selftests/bpf/progs/arena_kfunc_jit.c
> @@ -49,15 +49,15 @@ __arch_x86_64
>   __jited("...")
>   __jited("	movl	%edi, %edi")
>   __jited("	testl	%edi, %edi")
> -__jited("	je	L0")
> +__jited("	je	L1")
>   __jited("	addq	%r12, %rdi")
> -__jited("L0:	callq	{{.*}}")
> +__jited("L1:	callq	{{.*}}")
>   __arch_arm64
>   __jited("...")
>   __jited("	mov	w0, w0")
> -__jited("	cbz	w0, L0")
> +__jited("	cbz	w0, L1")
>   __jited("	add	x0, x28, w0, uxtw")
> -__jited("L0:	{{.*}}")
> +__jited("L1:	{{.*}}")
>   __success
>   int arena_arg_jit_nullable(void *ctx)
>   {
> @@ -79,9 +79,9 @@ __jited("	movl	%ecx, %ecx")
>   __jited("	addq	%r12, %rcx")
>   __jited("	movl	%r8d, %r8d")
>   __jited("	testl	%r8d, %r8d")
> -__jited("	je	L0")
> +__jited("	je	L1")
>   __jited("	addq	%r12, %r8")
> -__jited("L0:	callq	{{.*}}")
> +__jited("L1:	callq	{{.*}}")
>   __arch_arm64
>   __jited("...")
>   __jited("	add	x0, x28, w0, uxtw")
> @@ -89,9 +89,9 @@ __jited("	add	x1, x28, w1, uxtw")
>   __jited("	add	x2, x28, w2, uxtw")
>   __jited("	add	x3, x28, w3, uxtw")
>   __jited("	mov	w4, w4")
> -__jited("	cbz	w4, L0")
> +__jited("	cbz	w4, L1")
>   __jited("	add	x4, x28, w4, uxtw")
> -__jited("L0:	{{.*}}")
> +__jited("L1:	{{.*}}")
>   __success
>   int arena_arg_jit_args5(void *ctx)
>   {
> diff --git a/tools/testing/selftests/bpf/progs/verifier_arena.c b/tools/testing/selftests/bpf/progs/verifier_arena.c
> index 815f342eb4b0..d76490e059f9 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_arena.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_arena.c
> @@ -445,6 +445,8 @@ int iter_maps1(struct bpf_iter__bpf_map *ctx)
>   
>   	if (!map)
>   		return 0;
> +	/* Associate an arena before testing the generic map-pointer path. */
> +	bpf_arena_reserve_pages(&arena, NULL, 0);
>   	bpf_arena_alloc_pages(map, NULL, map->max_entries, 0, 0);
>   	return 0;
>   }
> @@ -455,6 +457,8 @@ int iter_maps2(struct bpf_iter__bpf_map *ctx)
>   {
>   	struct seq_file *seq = ctx->meta->seq;
>   
> +	/* Associate an arena before testing the generic map-pointer path. */
> +	bpf_arena_reserve_pages(&arena, NULL, 0);
>   	bpf_arena_alloc_pages((void *)seq, NULL, 1, 0, 0);
>   	return 0;
>   }
> @@ -467,6 +471,8 @@ int iter_maps3(struct bpf_iter__bpf_map *ctx)
>   
>   	if (!map)
>   		return 0;
> +	/* Associate an arena before testing the generic map-pointer path. */
> +	bpf_arena_reserve_pages(&arena, NULL, 0);
>   	bpf_arena_alloc_pages(map->inner_map_meta, NULL, map->max_entries, 0, 0);
>   	return 0;
>   }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_arena_large.c b/tools/testing/selftests/bpf/progs/verifier_arena_large.c
> index 6ab8730d4878..f6515e0e9b17 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_arena_large.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_arena_large.c
> @@ -49,8 +49,10 @@ int big_alloc1(void *ctx)
>   
>   	no_page = bpf_arena_alloc_pages(&arena, (void __arena *)ARENA_SIZE,
>   					1, NUMA_NO_NODE, 0);
> -	if (no_page)
> +	/* Only the low 32 bits contribute, so this is equivalent to NULL. */
> +	if (!no_page)
>   		return 3;
> +	bpf_arena_free_pages(&arena, (void __arena *)no_page, 1);
>   	if (*page1 != 1)
>   		return 4;
>   	if (*page2 != 2)


  parent reply	other threads:[~2026-08-26 20:42 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-21 23:34 [PATCH bpf-next v1 00/14] Retire KF_ARENA_ARG kfunc flags Kumar Kartikeya Dwivedi
2026-08-21 23:34 ` [PATCH bpf-next v1 01/14] bpf: Split arena kfunc and struct_ops JIT capabilities Kumar Kartikeya Dwivedi
2026-08-22  0:46   ` bot+bpf-ci
2026-08-24 22:28   ` Eduard Zingerman
2026-08-24 22:37     ` Kumar Kartikeya Dwivedi
2026-08-26 19:52     ` Ihor Solodrai
2026-08-21 23:34 ` [PATCH bpf-next v1 02/14] bpf, riscv: Fix stack-passed arguments for indirect trampolines Kumar Kartikeya Dwivedi
2026-08-24  6:21   ` Pu Lehui
2026-08-21 23:34 ` [PATCH bpf-next v1 03/14] bpf, riscv: JIT arena kfunc argument rebasing Kumar Kartikeya Dwivedi
2026-08-24  6:36   ` Pu Lehui
2026-08-21 23:34 ` [PATCH bpf-next v1 04/14] bpf, riscv: Convert struct_ops arena arguments in the trampoline Kumar Kartikeya Dwivedi
2026-08-21 23:44   ` sashiko-bot
2026-08-24  6:38   ` Pu Lehui
2026-08-21 23:34 ` [PATCH bpf-next v1 05/14] bpf, s390: JIT arena kfunc argument rebasing Kumar Kartikeya Dwivedi
2026-08-21 23:35 ` [PATCH bpf-next v1 06/14] bpf, s390: Convert struct_ops arena arguments Kumar Kartikeya Dwivedi
2026-08-22  0:46   ` bot+bpf-ci
2026-08-21 23:35 ` [PATCH bpf-next v1 07/14] bpf, loongarch: Fix stack arguments for indirect trampolines Kumar Kartikeya Dwivedi
2026-08-22  0:46   ` bot+bpf-ci
2026-08-21 23:35 ` [PATCH bpf-next v1 08/14] bpf, loongarch: JIT arena kfunc argument rebasing Kumar Kartikeya Dwivedi
2026-08-21 23:46   ` sashiko-bot
2026-08-21 23:35 ` [PATCH bpf-next v1 09/14] bpf, loongarch: Convert struct_ops arena arguments in trampolines Kumar Kartikeya Dwivedi
2026-08-21 23:51   ` sashiko-bot
2026-08-21 23:35 ` [PATCH bpf-next v1 10/14] bpf, powerpc: JIT arena kfunc argument rebasing Kumar Kartikeya Dwivedi
2026-08-22  0:46   ` bot+bpf-ci
2026-08-21 23:35 ` [PATCH bpf-next v1 11/14] bpf: Replace arena kfunc argument flags with suffixes Kumar Kartikeya Dwivedi
2026-08-21 23:58   ` sashiko-bot
2026-08-22  0:46   ` bot+bpf-ci
2026-08-24 22:15   ` Eduard Zingerman
2026-08-24 22:52     ` Kumar Kartikeya Dwivedi
2026-08-26 20:42   ` Ihor Solodrai [this message]
2026-08-21 23:35 ` [PATCH bpf-next v1 12/14] resolve_btfids: Drop KF_ARENA_ARG flag support Kumar Kartikeya Dwivedi
2026-08-22  0:46   ` bot+bpf-ci
2026-08-24 22:25   ` Eduard Zingerman
2026-08-24 22:53     ` Kumar Kartikeya Dwivedi
2026-08-26 20:47   ` Ihor Solodrai
2026-08-21 23:35 ` [PATCH bpf-next v1 13/14] selftests/bpf: Exercise arena arguments on every capable JIT Kumar Kartikeya Dwivedi
2026-08-22  0:46   ` bot+bpf-ci
2026-08-26 20:50   ` Ihor Solodrai
2026-08-21 23:35 ` [PATCH bpf-next v1 14/14] docs/bpf: Document split arena argument JIT capabilities Kumar Kartikeya Dwivedi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=49226345-d2e4-4618-8c9d-9dd1d9b0ff4b@linux.dev \
    --to=ihor.solodrai@linux.dev \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=eddyz87@gmail.com \
    --cc=emil@etsalapatis.com \
    --cc=kernel-team@meta.com \
    --cc=kkd@meta.com \
    --cc=memxor@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox