bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/3] bpf: Allow union argument in trampoline based programs
@ 2025-09-16 15:52 Leon Hwang
  2025-09-16 15:52 ` [PATCH bpf-next v2 1/3] " Leon Hwang
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Leon Hwang @ 2025-09-16 15:52 UTC (permalink / raw)
  To: bpf
  Cc: ast, andrii, daniel, martin.lau, eddyz87, song, yonghong.song,
	yatsenko, puranjay, davidzalman.101, cheick.traore, chen.dylane,
	mika.westerberg, ameryhung, menglong8.dong, leon.hwang,
	kernel-patches-bot

While tracing 'release_pages' with bpfsnoop[0], the verifier reports:

The function release_pages arg0 type UNION is unsupported.

However, it should be acceptable to trace functions that have 'union'
arguments.

This patch set enables such support in the verifier by allowing 'union'
as a valid argument type.

Changes:
v1 -> v2:
* Add 16B 'union' argument support in x86_64 trampoline.
* Update selftests using bpf_testmod.
* Add test case about 16-bytes 'union' argument.
* Address comments from Alexei:
  * Study the patch set about 'struct' argument support.
  * Update selftests to cover more cases.
v1: https://lore.kernel.org/bpf/20250905133226.84675-1-leon.hwang@linux.dev/

Links:
[0] https://github.com/bpfsnoop/bpfsnoop

Leon Hwang (3):
  bpf: Allow union argument in trampoline based programs
  bpf, x64: Add union argument support in trampoline
  selftests/bpf: Add union argument tests using fexit programs

 arch/x86/net/bpf_jit_comp.c                   |  2 +-
 include/linux/bpf.h                           |  3 ++
 include/linux/btf.h                           |  5 +++
 kernel/bpf/btf.c                              |  8 +++--
 .../selftests/bpf/prog_tests/tracing_struct.c | 29 ++++++++++++++++
 .../selftests/bpf/progs/tracing_struct.c      | 33 +++++++++++++++++++
 .../selftests/bpf/test_kmods/bpf_testmod.c    | 31 +++++++++++++++++
 7 files changed, 107 insertions(+), 4 deletions(-)

--
2.50.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH bpf-next v2 1/3] bpf: Allow union argument in trampoline based programs
  2025-09-16 15:52 [PATCH bpf-next v2 0/3] bpf: Allow union argument in trampoline based programs Leon Hwang
@ 2025-09-16 15:52 ` Leon Hwang
  2025-09-16 21:35   ` Amery Hung
  2025-09-16 15:52 ` [PATCH bpf-next v2 2/3] bpf, x64: Add union argument support in trampoline Leon Hwang
  2025-09-16 15:52 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs Leon Hwang
  2 siblings, 1 reply; 12+ messages in thread
From: Leon Hwang @ 2025-09-16 15:52 UTC (permalink / raw)
  To: bpf
  Cc: ast, andrii, daniel, martin.lau, eddyz87, song, yonghong.song,
	yatsenko, puranjay, davidzalman.101, cheick.traore, chen.dylane,
	mika.westerberg, ameryhung, menglong8.dong, leon.hwang,
	kernel-patches-bot

Currently, functions with 'union' arguments cannot be traced with
fentry/fexit:

bpftrace -e 'fentry:release_pages { exit(); }' -v
AST node count: 6
Attaching 1 probe...
ERROR: Error loading BPF program for fentry_vmlinux_release_pages_1.
Kernel error log:
The function release_pages arg0 type UNION is unsupported.
processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

ERROR: Loading BPF object(s) failed.

The type of the 'release_pages' argument is defined as:

typedef union {
	struct page **pages;
	struct folio **folios;
	struct encoded_page **encoded_pages;
} release_pages_arg __attribute__ ((__transparent_union__));

This patch relaxes the restriction by allowing function arguments of type
'union' to be traced in verifier.

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 include/linux/bpf.h | 3 +++
 include/linux/btf.h | 5 +++++
 kernel/bpf/btf.c    | 8 +++++---
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 41f776071ff51..010ecbb798c60 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1119,6 +1119,9 @@ struct bpf_prog_offload {
 /* The argument is signed. */
 #define BTF_FMODEL_SIGNED_ARG		BIT(1)
 
+/* The argument is a union. */
+#define BTF_FMODEL_UNION_ARG		BIT(2)
+
 struct btf_func_model {
 	u8 ret_size;
 	u8 ret_flags;
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 9eda6b113f9b4..255f8c6bd2438 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -404,6 +404,11 @@ static inline bool btf_type_is_struct(const struct btf_type *t)
 	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
 }
 
+static inline bool __btf_type_is_union(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info) == BTF_KIND_UNION;
+}
+
 static inline bool __btf_type_is_struct(const struct btf_type *t)
 {
 	return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 64739308902f7..2a85c51412bea 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6762,7 +6762,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	/* skip modifiers */
 	while (btf_type_is_modifier(t))
 		t = btf_type_by_id(btf, t->type);
-	if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
+	if (btf_type_is_small_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
 		/* accessing a scalar */
 		return true;
 	if (!btf_type_is_ptr(t)) {
@@ -7334,7 +7334,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
 	if (btf_type_is_ptr(t))
 		/* kernel size of pointer. Not BPF's size of pointer*/
 		return sizeof(void *);
-	if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
+	if (btf_type_is_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
 		return t->size;
 	return -EINVAL;
 }
@@ -7347,6 +7347,8 @@ static u8 __get_type_fmodel_flags(const struct btf_type *t)
 		flags |= BTF_FMODEL_STRUCT_ARG;
 	if (btf_type_is_signed_int(t))
 		flags |= BTF_FMODEL_SIGNED_ARG;
+	if (__btf_type_is_union(t))
+		flags |= BTF_FMODEL_UNION_ARG;
 
 	return flags;
 }
@@ -7384,7 +7386,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 		return -EINVAL;
 	}
 	ret = __get_type_size(btf, func->type, &t);
-	if (ret < 0 || __btf_type_is_struct(t)) {
+	if (ret < 0 || btf_type_is_struct(t)) {
 		bpf_log(log,
 			"The function %s return type %s is unsupported.\n",
 			tname, btf_type_str(t));
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH bpf-next v2 2/3] bpf, x64: Add union argument support in trampoline
  2025-09-16 15:52 [PATCH bpf-next v2 0/3] bpf: Allow union argument in trampoline based programs Leon Hwang
  2025-09-16 15:52 ` [PATCH bpf-next v2 1/3] " Leon Hwang
@ 2025-09-16 15:52 ` Leon Hwang
  2025-09-16 15:52 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs Leon Hwang
  2 siblings, 0 replies; 12+ messages in thread
From: Leon Hwang @ 2025-09-16 15:52 UTC (permalink / raw)
  To: bpf
  Cc: ast, andrii, daniel, martin.lau, eddyz87, song, yonghong.song,
	yatsenko, puranjay, davidzalman.101, cheick.traore, chen.dylane,
	mika.westerberg, ameryhung, menglong8.dong, leon.hwang,
	kernel-patches-bot

As verifier allows functions with 'union' argument can be traced, add
'union' arguments support in trampoline if the argument's size is in
range '(8, 16]'.

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 arch/x86/net/bpf_jit_comp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 8d34a9400a5e4..f30d3455ecf07 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3164,7 +3164,7 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *rw_im
 
 	/* extra registers for struct arguments */
 	for (i = 0; i < m->nr_args; i++) {
-		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
+		if (m->arg_flags[i] & (BTF_FMODEL_STRUCT_ARG | BTF_FMODEL_UNION_ARG))
 			nr_regs += (m->arg_size[i] + 7) / 8 - 1;
 	}
 
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs
  2025-09-16 15:52 [PATCH bpf-next v2 0/3] bpf: Allow union argument in trampoline based programs Leon Hwang
  2025-09-16 15:52 ` [PATCH bpf-next v2 1/3] " Leon Hwang
  2025-09-16 15:52 ` [PATCH bpf-next v2 2/3] bpf, x64: Add union argument support in trampoline Leon Hwang
@ 2025-09-16 15:52 ` Leon Hwang
  2025-09-18 16:09   ` Tao Chen
  2 siblings, 1 reply; 12+ messages in thread
From: Leon Hwang @ 2025-09-16 15:52 UTC (permalink / raw)
  To: bpf
  Cc: ast, andrii, daniel, martin.lau, eddyz87, song, yonghong.song,
	yatsenko, puranjay, davidzalman.101, cheick.traore, chen.dylane,
	mika.westerberg, ameryhung, menglong8.dong, leon.hwang,
	kernel-patches-bot

By referencing
commit 1642a3945e223 ("selftests/bpf: Add struct argument tests with fentry/fexit programs."),
test the following cases for union argument support:

* 8B union argument.
* 16B union argument.

cd tools/testing/selftests/bpf
./test_progs -t tracing_struct/union_args
472/3   tracing_struct/union_args:OK
472     tracing_struct:OK
Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 .../selftests/bpf/prog_tests/tracing_struct.c | 29 ++++++++++++++++
 .../selftests/bpf/progs/tracing_struct.c      | 33 +++++++++++++++++++
 .../selftests/bpf/test_kmods/bpf_testmod.c    | 31 +++++++++++++++++
 3 files changed, 93 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
index 19e68d4b35327..6f8c0bfb04155 100644
--- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
@@ -112,10 +112,39 @@ static void test_struct_many_args(void)
 	tracing_struct_many_args__destroy(skel);
 }
 
+static void test_union_args(void)
+{
+	struct tracing_struct *skel;
+	int err;
+
+	skel = tracing_struct__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "tracing_struct__open_and_load"))
+		return;
+
+	err = tracing_struct__attach(skel);
+	if (!ASSERT_OK(err, "tracing_struct__attach"))
+		goto out;
+
+	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
+
+	ASSERT_EQ(skel->bss->ut1_a_a, 1, "ut1:a.arg.a");
+	ASSERT_EQ(skel->bss->ut1_b, 4, "ut1:b");
+	ASSERT_EQ(skel->bss->ut1_c, 5, "ut1:c");
+
+	ASSERT_EQ(skel->bss->ut2_a, 6, "ut2:a");
+	ASSERT_EQ(skel->bss->ut2_b_a, 2, "ut2:b.arg.a");
+	ASSERT_EQ(skel->bss->ut2_b_b, 3, "ut2:b.arg.b");
+
+out:
+	tracing_struct__destroy(skel);
+}
+
 void test_tracing_struct(void)
 {
 	if (test__start_subtest("struct_args"))
 		test_struct_args();
 	if (test__start_subtest("struct_many_args"))
 		test_struct_many_args();
+	if (test__start_subtest("union_args"))
+		test_union_args();
 }
diff --git a/tools/testing/selftests/bpf/progs/tracing_struct.c b/tools/testing/selftests/bpf/progs/tracing_struct.c
index c435a3a8328ab..d460732e20239 100644
--- a/tools/testing/selftests/bpf/progs/tracing_struct.c
+++ b/tools/testing/selftests/bpf/progs/tracing_struct.c
@@ -18,6 +18,18 @@ struct bpf_testmod_struct_arg_3 {
 	int b[];
 };
 
+union bpf_testmod_union_arg_1 {
+	char a;
+	short b;
+	struct bpf_testmod_struct_arg_1 arg;
+};
+
+union bpf_testmod_union_arg_2 {
+	int a;
+	long b;
+	struct bpf_testmod_struct_arg_2 arg;
+};
+
 long t1_a_a, t1_a_b, t1_b, t1_c, t1_ret, t1_nregs;
 __u64 t1_reg0, t1_reg1, t1_reg2, t1_reg3;
 long t2_a, t2_b_a, t2_b_b, t2_c, t2_ret;
@@ -26,6 +38,9 @@ long t4_a_a, t4_b, t4_c, t4_d, t4_e_a, t4_e_b, t4_ret;
 long t5_ret;
 int t6;
 
+long ut1_a_a, ut1_b, ut1_c;
+long ut2_a, ut2_b_a, ut2_b_b;
+
 SEC("fentry/bpf_testmod_test_struct_arg_1")
 int BPF_PROG2(test_struct_arg_1, struct bpf_testmod_struct_arg_2, a, int, b, int, c)
 {
@@ -130,4 +145,22 @@ int BPF_PROG2(test_struct_arg_11, struct bpf_testmod_struct_arg_3 *, a)
 	return 0;
 }
 
+SEC("fexit/bpf_testmod_test_union_arg_1")
+int BPF_PROG2(test_union_arg_1, union bpf_testmod_union_arg_1, a, int, b, int, c)
+{
+	ut1_a_a = a.arg.a;
+	ut1_b = b;
+	ut1_c = c;
+	return 0;
+}
+
+SEC("fexit/bpf_testmod_test_union_arg_2")
+int BPF_PROG2(test_union_arg_2, int, a, union bpf_testmod_union_arg_2, b)
+{
+	ut2_a = a;
+	ut2_b_a = b.arg.a;
+	ut2_b_b = b.arg.b;
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
index 2beb9b2fcbd87..9cd28de05960c 100644
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
@@ -62,6 +62,18 @@ struct bpf_testmod_struct_arg_5 {
 	long d;
 };
 
+union bpf_testmod_union_arg_1 {
+	char a;
+	short b;
+	struct bpf_testmod_struct_arg_1 arg;
+};
+
+union bpf_testmod_union_arg_2 {
+	int a;
+	long b;
+	struct bpf_testmod_struct_arg_2 arg;
+};
+
 __bpf_hook_start();
 
 noinline int
@@ -128,6 +140,20 @@ bpf_testmod_test_struct_arg_9(u64 a, void *b, short c, int d, void *e, char f,
 	return bpf_testmod_test_struct_arg_result;
 }
 
+noinline int
+bpf_testmod_test_union_arg_1(union bpf_testmod_union_arg_1 a, int b, int c)
+{
+	bpf_testmod_test_struct_arg_result = a.arg.a + b + c;
+	return bpf_testmod_test_struct_arg_result;
+}
+
+noinline int
+bpf_testmod_test_union_arg_2(int a, union bpf_testmod_union_arg_2 b)
+{
+	bpf_testmod_test_struct_arg_result = a + b.arg.a + b.arg.b;
+	return bpf_testmod_test_struct_arg_result;
+}
+
 noinline int
 bpf_testmod_test_arg_ptr_to_struct(struct bpf_testmod_struct_arg_1 *a) {
 	bpf_testmod_test_struct_arg_result = a->a;
@@ -398,6 +424,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 	struct bpf_testmod_struct_arg_3 *struct_arg3;
 	struct bpf_testmod_struct_arg_4 struct_arg4 = {21, 22};
 	struct bpf_testmod_struct_arg_5 struct_arg5 = {23, 24, 25, 26};
+	union bpf_testmod_union_arg_1 union_arg1 = { .arg = {1} };
+	union bpf_testmod_union_arg_2 union_arg2 = { .arg = {2, 3} };
 	int i = 1;
 
 	while (bpf_testmod_return_ptr(i))
@@ -415,6 +443,9 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 	(void)bpf_testmod_test_struct_arg_9(16, (void *)17, 18, 19, (void *)20,
 					    21, 22, struct_arg5, 27);
 
+	(void)bpf_testmod_test_union_arg_1(union_arg1, 4, 5);
+	(void)bpf_testmod_test_union_arg_2(6, union_arg2);
+
 	(void)bpf_testmod_test_arg_ptr_to_struct(&struct_arg1_2);
 
 	(void)trace_bpf_testmod_test_raw_tp_null_tp(NULL);
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Allow union argument in trampoline based programs
  2025-09-16 15:52 ` [PATCH bpf-next v2 1/3] " Leon Hwang
@ 2025-09-16 21:35   ` Amery Hung
  2025-09-17 12:40     ` Leon Hwang
  0 siblings, 1 reply; 12+ messages in thread
From: Amery Hung @ 2025-09-16 21:35 UTC (permalink / raw)
  To: Leon Hwang
  Cc: bpf, ast, andrii, daniel, martin.lau, eddyz87, song,
	yonghong.song, yatsenko, puranjay, davidzalman.101, cheick.traore,
	chen.dylane, mika.westerberg, menglong8.dong, kernel-patches-bot

On Tue, Sep 16, 2025 at 8:52 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>
> Currently, functions with 'union' arguments cannot be traced with
> fentry/fexit:
>
> bpftrace -e 'fentry:release_pages { exit(); }' -v
> AST node count: 6
> Attaching 1 probe...
> ERROR: Error loading BPF program for fentry_vmlinux_release_pages_1.
> Kernel error log:
> The function release_pages arg0 type UNION is unsupported.
> processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>
> ERROR: Loading BPF object(s) failed.
>
> The type of the 'release_pages' argument is defined as:
>
> typedef union {
>         struct page **pages;
>         struct folio **folios;
>         struct encoded_page **encoded_pages;
> } release_pages_arg __attribute__ ((__transparent_union__));
>
> This patch relaxes the restriction by allowing function arguments of type
> 'union' to be traced in verifier.
>
> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> ---
>  include/linux/bpf.h | 3 +++
>  include/linux/btf.h | 5 +++++
>  kernel/bpf/btf.c    | 8 +++++---
>  3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 41f776071ff51..010ecbb798c60 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1119,6 +1119,9 @@ struct bpf_prog_offload {
>  /* The argument is signed. */
>  #define BTF_FMODEL_SIGNED_ARG          BIT(1)
>
> +/* The argument is a union. */
> +#define BTF_FMODEL_UNION_ARG           BIT(2)
> +

[...]

>  struct btf_func_model {
>         u8 ret_size;
>         u8 ret_flags;
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 9eda6b113f9b4..255f8c6bd2438 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -404,6 +404,11 @@ static inline bool btf_type_is_struct(const struct btf_type *t)
>         return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
>  }
>
> +static inline bool __btf_type_is_union(const struct btf_type *t)
> +{
> +       return BTF_INFO_KIND(t->info) == BTF_KIND_UNION;
> +}
> +
>  static inline bool __btf_type_is_struct(const struct btf_type *t)
>  {
>         return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 64739308902f7..2a85c51412bea 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6762,7 +6762,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>         /* skip modifiers */
>         while (btf_type_is_modifier(t))
>                 t = btf_type_by_id(btf, t->type);
> -       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
> +       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>                 /* accessing a scalar */
>                 return true;
>         if (!btf_type_is_ptr(t)) {
> @@ -7334,7 +7334,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
>         if (btf_type_is_ptr(t))
>                 /* kernel size of pointer. Not BPF's size of pointer*/
>                 return sizeof(void *);
> -       if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
> +       if (btf_type_is_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>                 return t->size;
>         return -EINVAL;
>  }
> @@ -7347,6 +7347,8 @@ static u8 __get_type_fmodel_flags(const struct btf_type *t)
>                 flags |= BTF_FMODEL_STRUCT_ARG;

Might be nit-picking but the handling of union arguments is identical
to struct, so maybe we don't need to introduce a new flag
BTF_FMODEL_UNION_ARG just for this. Changing __btf_type_is_struct() to
btf_type_is_struct() here should also work.

Otherwise, the set looks good to me.

Reviewed-by: Amery Hung <ameryhung@gmail.com>

>         if (btf_type_is_signed_int(t))
>                 flags |= BTF_FMODEL_SIGNED_ARG;
> +       if (__btf_type_is_union(t))
> +               flags |= BTF_FMODEL_UNION_ARG;
>
>         return flags;
>  }
> @@ -7384,7 +7386,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
>                 return -EINVAL;
>         }
>         ret = __get_type_size(btf, func->type, &t);
> -       if (ret < 0 || __btf_type_is_struct(t)) {
> +       if (ret < 0 || btf_type_is_struct(t)) {
>                 bpf_log(log,
>                         "The function %s return type %s is unsupported.\n",
>                         tname, btf_type_str(t));
> --
> 2.50.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Allow union argument in trampoline based programs
  2025-09-16 21:35   ` Amery Hung
@ 2025-09-17 12:40     ` Leon Hwang
  2025-09-17 18:13       ` Alexei Starovoitov
  0 siblings, 1 reply; 12+ messages in thread
From: Leon Hwang @ 2025-09-17 12:40 UTC (permalink / raw)
  To: Amery Hung
  Cc: bpf, ast, andrii, daniel, martin.lau, eddyz87, song,
	yonghong.song, yatsenko, puranjay, davidzalman.101, cheick.traore,
	chen.dylane, mika.westerberg, menglong8.dong, kernel-patches-bot

On Wed Sep 17, 2025 at 5:35 AM +08, Amery Hung wrote:
> On Tue, Sep 16, 2025 at 8:52 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> Currently, functions with 'union' arguments cannot be traced with
>> fentry/fexit:
>>
>> bpftrace -e 'fentry:release_pages { exit(); }' -v
>> AST node count: 6
>> Attaching 1 probe...
>> ERROR: Error loading BPF program for fentry_vmlinux_release_pages_1.
>> Kernel error log:
>> The function release_pages arg0 type UNION is unsupported.
>> processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>
>> ERROR: Loading BPF object(s) failed.
>>
>> The type of the 'release_pages' argument is defined as:
>>
>> typedef union {
>>         struct page **pages;
>>         struct folio **folios;
>>         struct encoded_page **encoded_pages;
>> } release_pages_arg __attribute__ ((__transparent_union__));
>>
>> This patch relaxes the restriction by allowing function arguments of type
>> 'union' to be traced in verifier.
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  include/linux/bpf.h | 3 +++
>>  include/linux/btf.h | 5 +++++
>>  kernel/bpf/btf.c    | 8 +++++---
>>  3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 41f776071ff51..010ecbb798c60 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1119,6 +1119,9 @@ struct bpf_prog_offload {
>>  /* The argument is signed. */
>>  #define BTF_FMODEL_SIGNED_ARG          BIT(1)
>>
>> +/* The argument is a union. */
>> +#define BTF_FMODEL_UNION_ARG           BIT(2)
>> +
>
> [...]
>
>>  struct btf_func_model {
>>         u8 ret_size;
>>         u8 ret_flags;
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index 9eda6b113f9b4..255f8c6bd2438 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -404,6 +404,11 @@ static inline bool btf_type_is_struct(const struct btf_type *t)
>>         return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
>>  }
>>
>> +static inline bool __btf_type_is_union(const struct btf_type *t)
>> +{
>> +       return BTF_INFO_KIND(t->info) == BTF_KIND_UNION;
>> +}
>> +
>>  static inline bool __btf_type_is_struct(const struct btf_type *t)
>>  {
>>         return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 64739308902f7..2a85c51412bea 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -6762,7 +6762,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>         /* skip modifiers */
>>         while (btf_type_is_modifier(t))
>>                 t = btf_type_by_id(btf, t->type);
>> -       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>> +       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>>                 /* accessing a scalar */
>>                 return true;
>>         if (!btf_type_is_ptr(t)) {
>> @@ -7334,7 +7334,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
>>         if (btf_type_is_ptr(t))
>>                 /* kernel size of pointer. Not BPF's size of pointer*/
>>                 return sizeof(void *);
>> -       if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>> +       if (btf_type_is_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>>                 return t->size;
>>         return -EINVAL;
>>  }
>> @@ -7347,6 +7347,8 @@ static u8 __get_type_fmodel_flags(const struct btf_type *t)
>>                 flags |= BTF_FMODEL_STRUCT_ARG;
>
> Might be nit-picking but the handling of union arguments is identical
> to struct, so maybe we don't need to introduce a new flag
> BTF_FMODEL_UNION_ARG just for this. Changing __btf_type_is_struct() to
> btf_type_is_struct() here should also work.

Correct. It should work with such changing.

However, it would be more readable to introduce the new flag as the flag
indicates the argument is a 'union' instead of a 'struct'.

>
> Otherwise, the set looks good to me.
>
> Reviewed-by: Amery Hung <ameryhung@gmail.com>

Thank you for your review.

Thanks,
Leon

[...]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Allow union argument in trampoline based programs
  2025-09-17 12:40     ` Leon Hwang
@ 2025-09-17 18:13       ` Alexei Starovoitov
  2025-09-18  1:47         ` Leon Hwang
  0 siblings, 1 reply; 12+ messages in thread
From: Alexei Starovoitov @ 2025-09-17 18:13 UTC (permalink / raw)
  To: Leon Hwang
  Cc: Amery Hung, bpf, Alexei Starovoitov, Andrii Nakryiko,
	Daniel Borkmann, Martin KaFai Lau, Eduard, Song Liu,
	Yonghong Song, Mykyta Yatsenko, Puranjay Mohan, davidzalman.101,
	cheick.traore, Tao Chen, mika.westerberg, Menglong Dong,
	kernel-patches-bot

On Wed, Sep 17, 2025 at 5:40 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>
> On Wed Sep 17, 2025 at 5:35 AM +08, Amery Hung wrote:
> > On Tue, Sep 16, 2025 at 8:52 AM Leon Hwang <leon.hwang@linux.dev> wrote:
> >>
> >> Currently, functions with 'union' arguments cannot be traced with
> >> fentry/fexit:
> >>
> >> bpftrace -e 'fentry:release_pages { exit(); }' -v
> >> AST node count: 6
> >> Attaching 1 probe...
> >> ERROR: Error loading BPF program for fentry_vmlinux_release_pages_1.
> >> Kernel error log:
> >> The function release_pages arg0 type UNION is unsupported.
> >> processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> >>
> >> ERROR: Loading BPF object(s) failed.
> >>
> >> The type of the 'release_pages' argument is defined as:
> >>
> >> typedef union {
> >>         struct page **pages;
> >>         struct folio **folios;
> >>         struct encoded_page **encoded_pages;
> >> } release_pages_arg __attribute__ ((__transparent_union__));
> >>
> >> This patch relaxes the restriction by allowing function arguments of type
> >> 'union' to be traced in verifier.
> >>
> >> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> >> ---
> >>  include/linux/bpf.h | 3 +++
> >>  include/linux/btf.h | 5 +++++
> >>  kernel/bpf/btf.c    | 8 +++++---
> >>  3 files changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index 41f776071ff51..010ecbb798c60 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -1119,6 +1119,9 @@ struct bpf_prog_offload {
> >>  /* The argument is signed. */
> >>  #define BTF_FMODEL_SIGNED_ARG          BIT(1)
> >>
> >> +/* The argument is a union. */
> >> +#define BTF_FMODEL_UNION_ARG           BIT(2)
> >> +
> >
> > [...]
> >
> >>  struct btf_func_model {
> >>         u8 ret_size;
> >>         u8 ret_flags;
> >> diff --git a/include/linux/btf.h b/include/linux/btf.h
> >> index 9eda6b113f9b4..255f8c6bd2438 100644
> >> --- a/include/linux/btf.h
> >> +++ b/include/linux/btf.h
> >> @@ -404,6 +404,11 @@ static inline bool btf_type_is_struct(const struct btf_type *t)
> >>         return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
> >>  }
> >>
> >> +static inline bool __btf_type_is_union(const struct btf_type *t)
> >> +{
> >> +       return BTF_INFO_KIND(t->info) == BTF_KIND_UNION;
> >> +}
> >> +
> >>  static inline bool __btf_type_is_struct(const struct btf_type *t)
> >>  {
> >>         return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
> >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> >> index 64739308902f7..2a85c51412bea 100644
> >> --- a/kernel/bpf/btf.c
> >> +++ b/kernel/bpf/btf.c
> >> @@ -6762,7 +6762,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >>         /* skip modifiers */
> >>         while (btf_type_is_modifier(t))
> >>                 t = btf_type_by_id(btf, t->type);
> >> -       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
> >> +       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
> >>                 /* accessing a scalar */
> >>                 return true;
> >>         if (!btf_type_is_ptr(t)) {
> >> @@ -7334,7 +7334,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
> >>         if (btf_type_is_ptr(t))
> >>                 /* kernel size of pointer. Not BPF's size of pointer*/
> >>                 return sizeof(void *);
> >> -       if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
> >> +       if (btf_type_is_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
> >>                 return t->size;
> >>         return -EINVAL;
> >>  }
> >> @@ -7347,6 +7347,8 @@ static u8 __get_type_fmodel_flags(const struct btf_type *t)
> >>                 flags |= BTF_FMODEL_STRUCT_ARG;
> >
> > Might be nit-picking but the handling of union arguments is identical
> > to struct, so maybe we don't need to introduce a new flag
> > BTF_FMODEL_UNION_ARG just for this. Changing __btf_type_is_struct() to
> > btf_type_is_struct() here should also work.
>
> Correct. It should work with such changing.
>
> However, it would be more readable to introduce the new flag as the flag
> indicates the argument is a 'union' instead of a 'struct'.

Why? How is that more readable?
Does it make a difference in calling convention?
If so, then yes they should be treated differently by JITs
that process func model, but if current JITs that support small structs
as an argument treat small union exact same way, then extra flag
is redundant and an existing flag should be renamed,
or a comment added
-/* The argument is a structure. */
+/* The argument is a structure or a union. */
 #define BTF_FMODEL_STRUCT_ARG          BIT(0)


pw-bot: cr

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Allow union argument in trampoline based programs
  2025-09-17 18:13       ` Alexei Starovoitov
@ 2025-09-18  1:47         ` Leon Hwang
  0 siblings, 0 replies; 12+ messages in thread
From: Leon Hwang @ 2025-09-18  1:47 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Amery Hung, bpf, Alexei Starovoitov, Andrii Nakryiko,
	Daniel Borkmann, Martin KaFai Lau, Eduard, Song Liu,
	Yonghong Song, Mykyta Yatsenko, Puranjay Mohan, davidzalman.101,
	cheick.traore, Tao Chen, mika.westerberg, Menglong Dong,
	kernel-patches-bot



On 18/9/25 02:13, Alexei Starovoitov wrote:
> On Wed, Sep 17, 2025 at 5:40 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> On Wed Sep 17, 2025 at 5:35 AM +08, Amery Hung wrote:
>>> On Tue, Sep 16, 2025 at 8:52 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>>>
>>>> Currently, functions with 'union' arguments cannot be traced with
>>>> fentry/fexit:
>>>>
>>>> bpftrace -e 'fentry:release_pages { exit(); }' -v
>>>> AST node count: 6
>>>> Attaching 1 probe...
>>>> ERROR: Error loading BPF program for fentry_vmlinux_release_pages_1.
>>>> Kernel error log:
>>>> The function release_pages arg0 type UNION is unsupported.
>>>> processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>>>
>>>> ERROR: Loading BPF object(s) failed.
>>>>
>>>> The type of the 'release_pages' argument is defined as:
>>>>
>>>> typedef union {
>>>>         struct page **pages;
>>>>         struct folio **folios;
>>>>         struct encoded_page **encoded_pages;
>>>> } release_pages_arg __attribute__ ((__transparent_union__));
>>>>
>>>> This patch relaxes the restriction by allowing function arguments of type
>>>> 'union' to be traced in verifier.
>>>>
>>>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>>>> ---
>>>>  include/linux/bpf.h | 3 +++
>>>>  include/linux/btf.h | 5 +++++
>>>>  kernel/bpf/btf.c    | 8 +++++---
>>>>  3 files changed, 13 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index 41f776071ff51..010ecbb798c60 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -1119,6 +1119,9 @@ struct bpf_prog_offload {
>>>>  /* The argument is signed. */
>>>>  #define BTF_FMODEL_SIGNED_ARG          BIT(1)
>>>>
>>>> +/* The argument is a union. */
>>>> +#define BTF_FMODEL_UNION_ARG           BIT(2)
>>>> +
>>>
>>> [...]
>>>
>>>>  struct btf_func_model {
>>>>         u8 ret_size;
>>>>         u8 ret_flags;
>>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>>> index 9eda6b113f9b4..255f8c6bd2438 100644
>>>> --- a/include/linux/btf.h
>>>> +++ b/include/linux/btf.h
>>>> @@ -404,6 +404,11 @@ static inline bool btf_type_is_struct(const struct btf_type *t)
>>>>         return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
>>>>  }
>>>>
>>>> +static inline bool __btf_type_is_union(const struct btf_type *t)
>>>> +{
>>>> +       return BTF_INFO_KIND(t->info) == BTF_KIND_UNION;
>>>> +}
>>>> +
>>>>  static inline bool __btf_type_is_struct(const struct btf_type *t)
>>>>  {
>>>>         return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
>>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>>> index 64739308902f7..2a85c51412bea 100644
>>>> --- a/kernel/bpf/btf.c
>>>> +++ b/kernel/bpf/btf.c
>>>> @@ -6762,7 +6762,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>>>         /* skip modifiers */
>>>>         while (btf_type_is_modifier(t))
>>>>                 t = btf_type_by_id(btf, t->type);
>>>> -       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>>>> +       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>>>>                 /* accessing a scalar */
>>>>                 return true;
>>>>         if (!btf_type_is_ptr(t)) {
>>>> @@ -7334,7 +7334,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
>>>>         if (btf_type_is_ptr(t))
>>>>                 /* kernel size of pointer. Not BPF's size of pointer*/
>>>>                 return sizeof(void *);
>>>> -       if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>>>> +       if (btf_type_is_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>>>>                 return t->size;
>>>>         return -EINVAL;
>>>>  }
>>>> @@ -7347,6 +7347,8 @@ static u8 __get_type_fmodel_flags(const struct btf_type *t)
>>>>                 flags |= BTF_FMODEL_STRUCT_ARG;
>>>
>>> Might be nit-picking but the handling of union arguments is identical
>>> to struct, so maybe we don't need to introduce a new flag
>>> BTF_FMODEL_UNION_ARG just for this. Changing __btf_type_is_struct() to
>>> btf_type_is_struct() here should also work.
>>
>> Correct. It should work with such changing.
>>
>> However, it would be more readable to introduce the new flag as the flag
>> indicates the argument is a 'union' instead of a 'struct'.
>
> Why? How is that more readable?
> Does it make a difference in calling convention?
> If so, then yes they should be treated differently by JITs
> that process func model, but if current JITs that support small structs
> as an argument treat small union exact same way, then extra flag
> is redundant and an existing flag should be renamed,
> or a comment added
> -/* The argument is a structure. */
> +/* The argument is a structure or a union. */
>  #define BTF_FMODEL_STRUCT_ARG          BIT(0)
>

Adding a comment sounds like a better approach than introducing a new flag:

1. Change __btf_type_is_struct() to btf_type_is_struct().
2. Drop BTF_FMODEL_UNION_ARG.
3. Drop __btf_type_is_union().
4. Drop patch #2.

I’ll send the next revision later.

Thanks,
Leon

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs
  2025-09-16 15:52 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs Leon Hwang
@ 2025-09-18 16:09   ` Tao Chen
  2025-09-19  1:47     ` Leon Hwang
  0 siblings, 1 reply; 12+ messages in thread
From: Tao Chen @ 2025-09-18 16:09 UTC (permalink / raw)
  To: Leon Hwang, bpf
  Cc: ast, andrii, daniel, martin.lau, eddyz87, song, yonghong.song,
	yatsenko, puranjay, davidzalman.101, cheick.traore,
	mika.westerberg, ameryhung, menglong8.dong, kernel-patches-bot

在 2025/9/16 23:52, Leon Hwang 写道:
> By referencing
> commit 1642a3945e223 ("selftests/bpf: Add struct argument tests with fentry/fexit programs."),
> test the following cases for union argument support:
> 

Can we use ‘commit 1642a3945e22’ with 12 chars, maybe it's minor nit 
anyways or not.

> * 8B union argument.
> * 16B union argument.
> 
> cd tools/testing/selftests/bpf
> ./test_progs -t tracing_struct/union_args
> 472/3   tracing_struct/union_args:OK
> 472     tracing_struct:OK
> Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
> 
> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
> ---
>   .../selftests/bpf/prog_tests/tracing_struct.c | 29 ++++++++++++++++
>   .../selftests/bpf/progs/tracing_struct.c      | 33 +++++++++++++++++++
>   .../selftests/bpf/test_kmods/bpf_testmod.c    | 31 +++++++++++++++++
>   3 files changed, 93 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> index 19e68d4b35327..6f8c0bfb04155 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> @@ -112,10 +112,39 @@ static void test_struct_many_args(void)
>   	tracing_struct_many_args__destroy(skel);
>   }
>   
> +static void test_union_args(void)
> +{
> +	struct tracing_struct *skel;
> +	int err;
> +
> +	skel = tracing_struct__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "tracing_struct__open_and_load"))
> +		return;
> +
> +	err = tracing_struct__attach(skel);
> +	if (!ASSERT_OK(err, "tracing_struct__attach"))
> +		goto out;
> +
> +	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
> +
> +	ASSERT_EQ(skel->bss->ut1_a_a, 1, "ut1:a.arg.a");
> +	ASSERT_EQ(skel->bss->ut1_b, 4, "ut1:b");
> +	ASSERT_EQ(skel->bss->ut1_c, 5, "ut1:c");
> +
> +	ASSERT_EQ(skel->bss->ut2_a, 6, "ut2:a");
> +	ASSERT_EQ(skel->bss->ut2_b_a, 2, "ut2:b.arg.a");
> +	ASSERT_EQ(skel->bss->ut2_b_b, 3, "ut2:b.arg.b");
> +
> +out:
> +	tracing_struct__destroy(skel);
> +}
> +
>   void test_tracing_struct(void)
>   {
>   	if (test__start_subtest("struct_args"))
>   		test_struct_args();
>   	if (test__start_subtest("struct_many_args"))
>   		test_struct_many_args();
> +	if (test__start_subtest("union_args"))
> +		test_union_args();
>   }
> diff --git a/tools/testing/selftests/bpf/progs/tracing_struct.c b/tools/testing/selftests/bpf/progs/tracing_struct.c
> index c435a3a8328ab..d460732e20239 100644
> --- a/tools/testing/selftests/bpf/progs/tracing_struct.c
> +++ b/tools/testing/selftests/bpf/progs/tracing_struct.c
> @@ -18,6 +18,18 @@ struct bpf_testmod_struct_arg_3 {
>   	int b[];
>   };
>   
> +union bpf_testmod_union_arg_1 {
> +	char a;
> +	short b;
> +	struct bpf_testmod_struct_arg_1 arg;
> +};
> +
> +union bpf_testmod_union_arg_2 {
> +	int a;
> +	long b;
> +	struct bpf_testmod_struct_arg_2 arg;
> +};
> +
>   long t1_a_a, t1_a_b, t1_b, t1_c, t1_ret, t1_nregs;
>   __u64 t1_reg0, t1_reg1, t1_reg2, t1_reg3;
>   long t2_a, t2_b_a, t2_b_b, t2_c, t2_ret;
> @@ -26,6 +38,9 @@ long t4_a_a, t4_b, t4_c, t4_d, t4_e_a, t4_e_b, t4_ret;
>   long t5_ret;
>   int t6;
>   
> +long ut1_a_a, ut1_b, ut1_c;
> +long ut2_a, ut2_b_a, ut2_b_b;
> +
>   SEC("fentry/bpf_testmod_test_struct_arg_1")
>   int BPF_PROG2(test_struct_arg_1, struct bpf_testmod_struct_arg_2, a, int, b, int, c)
>   {
> @@ -130,4 +145,22 @@ int BPF_PROG2(test_struct_arg_11, struct bpf_testmod_struct_arg_3 *, a)
>   	return 0;
>   }
>   
> +SEC("fexit/bpf_testmod_test_union_arg_1")
> +int BPF_PROG2(test_union_arg_1, union bpf_testmod_union_arg_1, a, int, b, int, c)
> +{
> +	ut1_a_a = a.arg.a;
> +	ut1_b = b;
> +	ut1_c = c;
> +	return 0;
> +}
> +
> +SEC("fexit/bpf_testmod_test_union_arg_2")
> +int BPF_PROG2(test_union_arg_2, int, a, union bpf_testmod_union_arg_2, b)
> +{
> +	ut2_a = a;
> +	ut2_b_a = b.arg.a;
> +	ut2_b_b = b.arg.b;
> +	return 0;
> +}
> +
>   char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> index 2beb9b2fcbd87..9cd28de05960c 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> @@ -62,6 +62,18 @@ struct bpf_testmod_struct_arg_5 {
>   	long d;
>   };
>   
> +union bpf_testmod_union_arg_1 {
> +	char a;
> +	short b;
> +	struct bpf_testmod_struct_arg_1 arg;
> +};
> +
> +union bpf_testmod_union_arg_2 {
> +	int a;
> +	long b;
> +	struct bpf_testmod_struct_arg_2 arg;
> +};
> +
>   __bpf_hook_start();
>   
>   noinline int
> @@ -128,6 +140,20 @@ bpf_testmod_test_struct_arg_9(u64 a, void *b, short c, int d, void *e, char f,
>   	return bpf_testmod_test_struct_arg_result;
>   }
>   
> +noinline int
> +bpf_testmod_test_union_arg_1(union bpf_testmod_union_arg_1 a, int b, int c)
> +{
> +	bpf_testmod_test_struct_arg_result = a.arg.a + b + c;
> +	return bpf_testmod_test_struct_arg_result;
> +}
> +
> +noinline int
> +bpf_testmod_test_union_arg_2(int a, union bpf_testmod_union_arg_2 b)
> +{
> +	bpf_testmod_test_struct_arg_result = a + b.arg.a + b.arg.b;
> +	return bpf_testmod_test_struct_arg_result;
> +}
> +
>   noinline int
>   bpf_testmod_test_arg_ptr_to_struct(struct bpf_testmod_struct_arg_1 *a) {
>   	bpf_testmod_test_struct_arg_result = a->a;
> @@ -398,6 +424,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
>   	struct bpf_testmod_struct_arg_3 *struct_arg3;
>   	struct bpf_testmod_struct_arg_4 struct_arg4 = {21, 22};
>   	struct bpf_testmod_struct_arg_5 struct_arg5 = {23, 24, 25, 26};
> +	union bpf_testmod_union_arg_1 union_arg1 = { .arg = {1} };
> +	union bpf_testmod_union_arg_2 union_arg2 = { .arg = {2, 3} };
>   	int i = 1;
>   
>   	while (bpf_testmod_return_ptr(i))
> @@ -415,6 +443,9 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
>   	(void)bpf_testmod_test_struct_arg_9(16, (void *)17, 18, 19, (void *)20,
>   					    21, 22, struct_arg5, 27);
>   
> +	(void)bpf_testmod_test_union_arg_1(union_arg1, 4, 5);
> +	(void)bpf_testmod_test_union_arg_2(6, union_arg2);
> +
>   	(void)bpf_testmod_test_arg_ptr_to_struct(&struct_arg1_2);
>   
>   	(void)trace_bpf_testmod_test_raw_tp_null_tp(NULL);


-- 
Best Regards
Tao Chen

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs
  2025-09-18 16:09   ` Tao Chen
@ 2025-09-19  1:47     ` Leon Hwang
  2025-09-19  1:53       ` Alexei Starovoitov
  0 siblings, 1 reply; 12+ messages in thread
From: Leon Hwang @ 2025-09-19  1:47 UTC (permalink / raw)
  To: Tao Chen, bpf
  Cc: ast, andrii, daniel, martin.lau, eddyz87, song, yonghong.song,
	yatsenko, puranjay, davidzalman.101, cheick.traore,
	mika.westerberg, ameryhung, menglong8.dong, kernel-patches-bot



On 19/9/25 00:09, Tao Chen wrote:
> 在 2025/9/16 23:52, Leon Hwang 写道:
>> By referencing
>> commit 1642a3945e223 ("selftests/bpf: Add struct argument tests with
>> fentry/fexit programs."),
>> test the following cases for union argument support:
>>
> 
> Can we use ‘commit 1642a3945e22’ with 12 chars, maybe it's minor nit
> anyways or not.
> 
Thank you for pointing this out.

I’ll update my script to generate the commit information in the proper
format.

Thanks,
Leon


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs
  2025-09-19  1:47     ` Leon Hwang
@ 2025-09-19  1:53       ` Alexei Starovoitov
  2025-09-19  2:01         ` Leon Hwang
  0 siblings, 1 reply; 12+ messages in thread
From: Alexei Starovoitov @ 2025-09-19  1:53 UTC (permalink / raw)
  To: Leon Hwang
  Cc: Tao Chen, bpf, Alexei Starovoitov, Andrii Nakryiko,
	Daniel Borkmann, Martin KaFai Lau, Eduard, Song Liu,
	Yonghong Song, Mykyta Yatsenko, Puranjay Mohan, davidzalman.101,
	cheick.traore, mika.westerberg, Amery Hung, Menglong Dong,
	kernel-patches-bot

On Thu, Sep 18, 2025 at 6:47 PM Leon Hwang <leon.hwang@linux.dev> wrote:
>
>
>
> On 19/9/25 00:09, Tao Chen wrote:
> > 在 2025/9/16 23:52, Leon Hwang 写道:
> >> By referencing
> >> commit 1642a3945e223 ("selftests/bpf: Add struct argument tests with
> >> fentry/fexit programs."),
> >> test the following cases for union argument support:
> >>
> >
> > Can we use ‘commit 1642a3945e22’ with 12 chars, maybe it's minor nit
> > anyways or not.
> >
> Thank you for pointing this out.
>
> I’ll update my script to generate the commit information in the proper
> format.

Since you're going to respin please rewrite the whole commit log
in both patches.
There is no need to talk about patch 1642a3945e223 at all.
It doesn't matter what you used to get inspiration from.
Also don't say things like:

cd tools/testing/selftests/bpf
./test_progs -t tracing_struct/union_args
472/3   tracing_struct/union_args:OK
472     tracing_struct:OK
Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED

of course they suppose to pass. Above is not useful in the commit log.

Instead describe what the test is for and what it's doing.

Similar in patch 1 trim bpftrace output.
Only mention the relevant parts. The command line and the error.
Things like:

processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0
peak_states 0 mark_read 0

are not useful.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs
  2025-09-19  1:53       ` Alexei Starovoitov
@ 2025-09-19  2:01         ` Leon Hwang
  0 siblings, 0 replies; 12+ messages in thread
From: Leon Hwang @ 2025-09-19  2:01 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Tao Chen, bpf, Alexei Starovoitov, Andrii Nakryiko,
	Daniel Borkmann, Martin KaFai Lau, Eduard, Song Liu,
	Yonghong Song, Mykyta Yatsenko, Puranjay Mohan, davidzalman.101,
	cheick.traore, mika.westerberg, Amery Hung, Menglong Dong,
	kernel-patches-bot



On 19/9/25 09:53, Alexei Starovoitov wrote:
> On Thu, Sep 18, 2025 at 6:47 PM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>>
>>
>> On 19/9/25 00:09, Tao Chen wrote:
>>> 在 2025/9/16 23:52, Leon Hwang 写道:
>>>> By referencing
>>>> commit 1642a3945e223 ("selftests/bpf: Add struct argument tests with
>>>> fentry/fexit programs."),
>>>> test the following cases for union argument support:
>>>>
>>>
>>> Can we use ‘commit 1642a3945e22’ with 12 chars, maybe it's minor nit
>>> anyways or not.
>>>
>> Thank you for pointing this out.
>>
>> I’ll update my script to generate the commit information in the proper
>> format.
> 
> Since you're going to respin please rewrite the whole commit log
> in both patches.
> There is no need to talk about patch 1642a3945e223 at all.
> It doesn't matter what you used to get inspiration from.
> Also don't say things like:
> 
> cd tools/testing/selftests/bpf
> ./test_progs -t tracing_struct/union_args
> 472/3   tracing_struct/union_args:OK
> 472     tracing_struct:OK
> Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
> 
> of course they suppose to pass. Above is not useful in the commit log.
> 
> Instead describe what the test is for and what it's doing.
> > Similar in patch 1 trim bpftrace output.
> Only mention the relevant parts. The command line and the error.
> Things like:
> 
> processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0
> peak_states 0 mark_read 0
> 
> are not useful.

Got it.

I’ll respin with updated commit logs:

* Patch #1: trim bpftrace output to only include the relevant command
  line and error.
* Patch #2: drop references to commit 1642a3945e223 and test output, and
  instead describe what the test is for and what it does.

Thanks,
Leon


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-09-19  2:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-16 15:52 [PATCH bpf-next v2 0/3] bpf: Allow union argument in trampoline based programs Leon Hwang
2025-09-16 15:52 ` [PATCH bpf-next v2 1/3] " Leon Hwang
2025-09-16 21:35   ` Amery Hung
2025-09-17 12:40     ` Leon Hwang
2025-09-17 18:13       ` Alexei Starovoitov
2025-09-18  1:47         ` Leon Hwang
2025-09-16 15:52 ` [PATCH bpf-next v2 2/3] bpf, x64: Add union argument support in trampoline Leon Hwang
2025-09-16 15:52 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add union argument tests using fexit programs Leon Hwang
2025-09-18 16:09   ` Tao Chen
2025-09-19  1:47     ` Leon Hwang
2025-09-19  1:53       ` Alexei Starovoitov
2025-09-19  2:01         ` Leon Hwang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).