BPF List
 help / color / mirror / Atom feed
* [PATCH bpf v2 0/3] bpf: Fix trampoline handling of 128-bit values
@ 2026-07-10 18:22 Yonghong Song
  2026-07-10 18:22 ` [PATCH bpf v2 1/3] bpf: Reject >8 byte return values on return-reading trampoline paths Yonghong Song
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Yonghong Song @ 2026-07-10 18:22 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
	Eduard Zingerman, kernel-team

The BPF trampoline preserves only 8 bytes of a target function's return
value (R0), and its register save area under-allocates space for 128-bit
arguments for x86_64. These two problems lead to memory corruption or
incorrect values observed by BPF programs and the real caller.

This series fixes both issues and adds two selftests, otherwise, each of
them will fail if without the corresponding fix.

Changelogs:
  v1 -> v2:
    - v1: https://lore.kernel.org/bpf/20260710144404.2579671-1-yonghong.song@linux.dev/
    - Also handle __int128 arguments for x86_64.

Yonghong Song (3):
  bpf: Reject >8 byte return values on return-reading trampoline paths
  bpf, x86: Fix trampoline stack size for 128-bit arguments
  selftests/bpf: Add tests for >8 byte return value and 128-bit
    arguments

 arch/x86/net/bpf_jit_comp.c                   |  7 ++--
 kernel/bpf/bpf_struct_ops.c                   | 12 +++++++
 kernel/bpf/verifier.c                         | 25 +++++++++++++
 .../bpf/prog_tests/tracing_failure.c          | 12 +++++++
 .../selftests/bpf/prog_tests/tracing_struct.c | 36 +++++++++++++++++++
 .../selftests/bpf/progs/tracing_failure.c     |  6 ++++
 .../bpf/progs/tracing_struct_int128.c         | 18 ++++++++++
 .../selftests/bpf/test_kmods/bpf_testmod.c    | 21 +++++++++++
 8 files changed, 132 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct_int128.c

-- 
2.53.0-Meta


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

* [PATCH bpf v2 1/3] bpf: Reject >8 byte return values on return-reading trampoline paths
  2026-07-10 18:22 [PATCH bpf v2 0/3] bpf: Fix trampoline handling of 128-bit values Yonghong Song
@ 2026-07-10 18:22 ` Yonghong Song
  2026-07-10 18:22 ` [PATCH bpf v2 2/3] bpf, x86: Fix trampoline stack size for 128-bit arguments Yonghong Song
  2026-07-10 18:22 ` [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and " Yonghong Song
  2 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2026-07-10 18:22 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
	Eduard Zingerman, kernel-team, Leon Hwang

btf_distill_func_proto() builds the function model used for the
fentry/fexit/fmod_ret/fsession trampolines and struct_ops. It has
accepted a 16-byte __int128 return value since the trampoline was
introduced: __get_type_size() returns the integer's type size, and the
return-type check only rejected ret < 0.

But the BPF trampoline preserves only 8 bytes of the return value (RAX on
x86, i.e. R0). For an attach type that reads the target's return value the
second half (RDX / R3) is neither saved nor restored, so a program
attached to a function returning a 16-byte value corrupts the value seen
by the real caller and itself observes only half of it. struct_ops
trampolines have the same limitation.

This affects the attach types that read the target's return value: fexit,
fmod_ret and fsession (plus the _multi variants of fexit and fsession),
and struct_ops. fentry/fentry_multi run before the target returns and are
unaffected.

Reject a >8 byte return value for these attach types in
bpf_check_attach_target() and bpf_check_attach_btf_id_multi(), and for
struct_ops in bpf_struct_ops_desc_init().

Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
Cc: Leon Hwang <leon.hwang@linux.dev>
Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/bpf_struct_ops.c | 12 ++++++++++++
 kernel/bpf/verifier.c       | 25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 51b16e5f5534..4e7a48c02be5 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -445,6 +445,18 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
 			goto errout;
 		}
 
+		/*
+		 * A >8 byte return value is passed back in a register pair,
+		 * which the struct_ops trampoline does not preserve (only
+		 * 8 bytes of the return value are saved and restored).
+		 */
+		if (st_ops->func_models[i].ret_size > 8) {
+			pr_warn("func ptr %s in struct %s has a >8 byte return value, which is not supported\n",
+				mname, st_ops->name);
+			err = -EOPNOTSUPP;
+			goto errout;
+		}
+
 		stub_func_addr = *(void **)(st_ops->cfi_stubs + moff);
 		err = prepare_arg_info(btf, st_ops->name, mname,
 				       func_proto, stub_func_addr,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6515d4d3c003..26d281b77a6e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -18873,6 +18873,20 @@ static int btf_id_allow_sleepable(u32 btf_id, unsigned long addr, const struct b
 	return -EINVAL;
 }
 
+static bool attach_uses_trampoline_retval(enum bpf_attach_type type)
+{
+	switch (type) {
+	case BPF_MODIFY_RETURN:
+	case BPF_TRACE_FEXIT:
+	case BPF_TRACE_FEXIT_MULTI:
+	case BPF_TRACE_FSESSION:
+	case BPF_TRACE_FSESSION_MULTI:
+		return true;
+	default:
+		return false;
+	}
+}
+
 int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    const struct bpf_prog *prog,
 			    const struct bpf_prog *tgt_prog,
@@ -19137,6 +19151,14 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 		if (ret < 0)
 			return ret;
 
+		if (tgt_info->fmodel.ret_size > 8 &&
+		    attach_uses_trampoline_retval(prog->expected_attach_type)) {
+			bpf_log(log,
+				"Attach to function %s with a >8 byte return value is not supported for this attach type\n",
+				tname);
+			return -EOPNOTSUPP;
+		}
+
 		/*
 		 * *.multi programs don't need an address during program
 		 * verification, we just take the module ref if needed.
@@ -19413,6 +19435,9 @@ int bpf_check_attach_btf_id_multi(struct btf *btf, struct bpf_prog *prog, u32 bt
 	err = btf_distill_func_proto(NULL, btf, t, tname, &tgt_info->fmodel);
 	if (err < 0)
 		return err;
+	if (tgt_info->fmodel.ret_size > 8 &&
+	    attach_uses_trampoline_retval(prog->expected_attach_type))
+		return -EOPNOTSUPP;
 	if (btf_is_module(btf)) {
 		/* The bpf program already holds reference to module. */
 		if (WARN_ON_ONCE(!prog->aux->mod))
-- 
2.53.0-Meta


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

* [PATCH bpf v2 2/3] bpf, x86: Fix trampoline stack size for 128-bit arguments
  2026-07-10 18:22 [PATCH bpf v2 0/3] bpf: Fix trampoline handling of 128-bit values Yonghong Song
  2026-07-10 18:22 ` [PATCH bpf v2 1/3] bpf: Reject >8 byte return values on return-reading trampoline paths Yonghong Song
@ 2026-07-10 18:22 ` Yonghong Song
  2026-07-10 18:22 ` [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and " Yonghong Song
  2 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2026-07-10 18:22 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
	Eduard Zingerman, kernel-team

btf_distill_func_proto() accepts a function argument up to 16 bytes, so a
128-bit scalar such as __int128 reaches the x86 trampoline with
arg_size == 16. But the current implementation assumes an __int128
argument only needs one register, so the register save area is
under-allocated and save_args() overwrites adjacent stack slots.

Compute the register count from arg_size for all arguments to fix it.

Fixes: a9c5ad31fbdc ("bpf: x86: Support in-register struct arguments in trampoline programs")
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 arch/x86/net/bpf_jit_comp.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index de7515ea1bea..0b8bd30f7c87 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3369,11 +3369,8 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *rw_im
 	WARN_ON_ONCE((flags & BPF_TRAMP_F_INDIRECT) &&
 		     (flags & ~(BPF_TRAMP_F_INDIRECT | BPF_TRAMP_F_RET_FENTRY_RET)));
 
-	/* extra registers for struct arguments */
-	for (i = 0; i < m->nr_args; i++) {
-		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
-			nr_regs += (m->arg_size[i] + 7) / 8 - 1;
-	}
+	for (i = 0; i < m->nr_args; i++)
+		nr_regs += (m->arg_size[i] + 7) / 8 - 1;
 
 	/* x86-64 supports up to MAX_BPF_FUNC_ARGS arguments. 1-6
 	 * are passed through regs, the remains are through stack.
-- 
2.53.0-Meta


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

* [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and 128-bit arguments
  2026-07-10 18:22 [PATCH bpf v2 0/3] bpf: Fix trampoline handling of 128-bit values Yonghong Song
  2026-07-10 18:22 ` [PATCH bpf v2 1/3] bpf: Reject >8 byte return values on return-reading trampoline paths Yonghong Song
  2026-07-10 18:22 ` [PATCH bpf v2 2/3] bpf, x86: Fix trampoline stack size for 128-bit arguments Yonghong Song
@ 2026-07-10 18:22 ` Yonghong Song
  2026-07-10 18:38   ` sashiko-bot
  2 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2026-07-10 18:22 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
	Eduard Zingerman, kernel-team

The BPF trampoline preserves only 8 bytes of the target's return value
(R0), so attaching an fexit/fmod_ret/fsession program to a function that
returns a >8 byte value is now rejected by the verifier. Add a bpf_testmod
function returning __int128 and an fexit program that targets it. The
program is expected to fail to load with the "with a >8 byte return value
is not supported for this attach type" message.

A 128-bit __int128 argument is passed in a register pair and occupies two
trampoline context slots. Add a bpf_testmod function taking an __int128
argument between two scalar arguments and an fexit program that reads the
surrounding argument and the return value, verifying that the trampoline
reserves enough stack for the 128-bit argument.

__int128 is only available on 64-bit targets (where the compiler defines
__SIZEOF_INT128__). The argument test additionally depends on the calling
convention: x86_64 and arm64 pass an __int128 in a register pair as the
trampoline expects, while other architectures pass it differently (e.g.
s390x passes larger arguments by reference), so that subtest runs only on
x86_64 and arm64 and is skipped elsewhere.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/tracing_failure.c          | 12 +++++++
 .../selftests/bpf/prog_tests/tracing_struct.c | 36 +++++++++++++++++++
 .../selftests/bpf/progs/tracing_failure.c     |  6 ++++
 .../bpf/progs/tracing_struct_int128.c         | 18 ++++++++++
 .../selftests/bpf/test_kmods/bpf_testmod.c    | 21 +++++++++++
 5 files changed, 93 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct_int128.c

diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_failure.c b/tools/testing/selftests/bpf/prog_tests/tracing_failure.c
index f9f9e1cb87bf..345dd21ad621 100644
--- a/tools/testing/selftests/bpf/prog_tests/tracing_failure.c
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_failure.c
@@ -76,6 +76,16 @@ static void test_fexit_noreturns(void)
 			       "Attaching fexit/fsession/fmod_ret to __noreturn function 'do_exit' is rejected.");
 }
 
+static void test_fexit_int128_ret(void)
+{
+#ifdef __SIZEOF_INT128__
+	test_tracing_fail_prog("fexit_int128_ret",
+			       "with a >8 byte return value is not supported for this attach type");
+#else
+	test__skip();
+#endif
+}
+
 void test_tracing_failure(void)
 {
 	if (test__start_subtest("bpf_spin_lock"))
@@ -86,4 +96,6 @@ void test_tracing_failure(void)
 		test_tracing_deny();
 	if (test__start_subtest("fexit_noreturns"))
 		test_fexit_noreturns();
+	if (test__start_subtest("fexit_int128_ret"))
+		test_fexit_int128_ret();
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
index 6f8c0bfb0415..d32198ee02df 100644
--- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
@@ -4,6 +4,7 @@
 #include <test_progs.h>
 #include "tracing_struct.skel.h"
 #include "tracing_struct_many_args.skel.h"
+#include "tracing_struct_int128.skel.h"
 
 static void test_struct_args(void)
 {
@@ -112,6 +113,39 @@ static void test_struct_many_args(void)
 	tracing_struct_many_args__destroy(skel);
 }
 
+static void test_int128_args(void)
+{
+	/*
+	 * __int128 arguments are passed in a register pair on x86_64 and
+	 * arm64, which the trampoline packs into two context slots. Other
+	 * architectures pass a __int128 differently (e.g. s390x passes larger
+	 * arguments by reference), so only exercise this on x86_64 and arm64.
+	 */
+#if defined(__x86_64__) || defined(__aarch64__)
+	struct tracing_struct_int128 *skel;
+	int err;
+
+	skel = tracing_struct_int128__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "tracing_struct_int128__open_and_load"))
+		return;
+
+	err = tracing_struct_int128__attach(skel);
+	if (!ASSERT_OK(err, "tracing_struct_int128__attach"))
+		goto destroy_skel;
+
+	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
+
+	ASSERT_EQ(skel->bss->t_a, 1, "t:a");
+	ASSERT_EQ(skel->bss->t_c, 3, "t:c");
+	ASSERT_EQ(skel->bss->t_ret, 6, "t ret");
+
+destroy_skel:
+	tracing_struct_int128__destroy(skel);
+#else
+	test__skip();
+#endif
+}
+
 static void test_union_args(void)
 {
 	struct tracing_struct *skel;
@@ -145,6 +179,8 @@ void test_tracing_struct(void)
 		test_struct_args();
 	if (test__start_subtest("struct_many_args"))
 		test_struct_many_args();
+	if (test__start_subtest("int128_args"))
+		test_int128_args();
 	if (test__start_subtest("union_args"))
 		test_union_args();
 }
diff --git a/tools/testing/selftests/bpf/progs/tracing_failure.c b/tools/testing/selftests/bpf/progs/tracing_failure.c
index 65e485c4468c..f7a095767679 100644
--- a/tools/testing/selftests/bpf/progs/tracing_failure.c
+++ b/tools/testing/selftests/bpf/progs/tracing_failure.c
@@ -30,3 +30,9 @@ int BPF_PROG(fexit_noreturns)
 {
 	return 0;
 }
+
+SEC("?fexit/bpf_testmod_test_int128_ret")
+int BPF_PROG(fexit_int128_ret)
+{
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/tracing_struct_int128.c b/tools/testing/selftests/bpf/progs/tracing_struct_int128.c
new file mode 100644
index 000000000000..5f20dd4fdd9d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tracing_struct_int128.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+
+long t_a, t_c, t_ret;
+
+SEC("fexit/bpf_testmod_test_int128_arg")
+int test_int128_arg_fexit(unsigned long long *ctx)
+{
+	t_a = (int)ctx[0];
+	t_c = (long)ctx[3];
+	t_ret = (long)ctx[4];
+	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 30f1cd23093c..a0ad810b5ddc 100644
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
@@ -161,6 +161,22 @@ bpf_testmod_test_arg_ptr_to_struct(struct bpf_testmod_struct_arg_1 *a) {
 	return bpf_testmod_test_struct_arg_result;
 }
 
+#ifdef __SIZEOF_INT128__
+noinline __int128
+bpf_testmod_test_int128_ret(int a)
+{
+	bpf_testmod_test_struct_arg_result = a;
+	return (__int128)a;
+}
+
+noinline long
+bpf_testmod_test_int128_arg(int a, __int128 b, long c)
+{
+	bpf_testmod_test_struct_arg_result = a + (long)b + c;
+	return bpf_testmod_test_struct_arg_result;
+}
+#endif
+
 __weak noinline void bpf_testmod_looooooooooooooooooooooooooooooong_name(void)
 {
 }
@@ -514,6 +530,11 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 
 	(void)bpf_testmod_test_arg_ptr_to_struct(&struct_arg1_2);
 
+#ifdef __SIZEOF_INT128__
+	(void)bpf_testmod_test_int128_ret(i);
+	(void)bpf_testmod_test_int128_arg(1, (__int128)2, 3);
+#endif
+
 	(void)trace_bpf_testmod_test_raw_tp_null_tp(NULL);
 
 	bpf_testmod_test_struct_ops3();
-- 
2.53.0-Meta


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

* Re: [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and 128-bit arguments
  2026-07-10 18:22 ` [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and " Yonghong Song
@ 2026-07-10 18:38   ` sashiko-bot
  2026-07-10 19:13     ` Yonghong Song
  0 siblings, 1 reply; 6+ messages in thread
From: sashiko-bot @ 2026-07-10 18:38 UTC (permalink / raw)
  To: Yonghong Song; +Cc: bpf

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added `test_int128_args` selftest will predictably fail on `aarch64` due to an unhandled ABI alignment requirement in the ARM64 BPF trampoline.
--

--- Patch [3]: [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and 128-bit arguments ---
commit c4e66c7551048f829ca6d77d8b1b2e6467e4525e
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add tests for >8 byte return value and 128-bit arguments

The BPF trampoline preserves only 8 bytes of the target's return value
(R0), so attaching an fexit/fmod_ret/fsession program to a function that
returns a >8 byte value is now rejected by the verifier. Add a bpf_testmod
function returning __int128 and an fexit program that targets it. The
program is expected to fail to load with the "with a >8 byte return value
is not supported for this attach type" message.

> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> index 6f8c0bfb04155..d32198ee02dff 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
[ ... ]
> @@ -112,6 +113,39 @@ static void test_struct_many_args(void)
>  	tracing_struct_many_args__destroy(skel);
>  }
>  
> +static void test_int128_args(void)
> +{
> +	/*
> +	 * __int128 arguments are passed in a register pair on x86_64 and
> +	 * arm64, which the trampoline packs into two context slots. Other
> +	 * architectures pass a __int128 differently (e.g. s390x passes larger
> +	 * arguments by reference), so only exercise this on x86_64 and arm64.
> +	 */
> +#if defined(__x86_64__) || defined(__aarch64__)
> +	struct tracing_struct_int128 *skel;
> +	int err;
> +
> +	skel = tracing_struct_int128__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "tracing_struct_int128__open_and_load"))
> +		return;
> +
> +	err = tracing_struct_int128__attach(skel);
> +	if (!ASSERT_OK(err, "tracing_struct_int128__attach"))
> +		goto destroy_skel;
> +
> +	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
> +
> +	ASSERT_EQ(skel->bss->t_a, 1, "t:a");
> +	ASSERT_EQ(skel->bss->t_c, 3, "t:c");
        ^^^^

[Severity: Medium]
Will this test predictably fail on AArch64?

The test calls bpf_testmod_test_int128_arg(1, 2, 3). According to the AAPCS64
calling convention, a 16-byte aligned argument (__int128) must be passed in an
even-odd register pair. Thus, the integer 'a' goes to w0, x1 is skipped for
alignment padding, the __int128 'b' goes to x2 and x3, and the long 'c' goes
to x4.

However, the pre-existing logic in the ARM64 BPF trampoline fails to account
for this padding. It solely calculates register usage based on argument sizes:

arch/arm64/net/bpf_jit_comp.c:calc_arg_aux() {
	...
	for (i = 0, nregs = 0; i < m->nr_args; i++) {
		slots = (m->arg_size[i] + 7) / 8;
		if (nregs + slots <= 8) /* passed through register ? */
			nregs += slots;
	...
}

Because it incorrectly assumes the arguments use exactly 4 contiguous
registers (x0 through x3), it only saves those.

As a result, when the test's BPF program reads ctx[3] expecting 'c', it
actually reads x3 (the upper 64 bits of 'b', which is 0).

Would it be better to fix the ARM64 trampoline padding handling first, or
exclude AArch64 from this test for now to prevent CI failures?

> +	ASSERT_EQ(skel->bss->t_ret, 6, "t ret");
> +
> +destroy_skel:
> +	tracing_struct_int128__destroy(skel);
> +#else
> +	test__skip();
> +#endif
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710182204.1085329-1-yonghong.song@linux.dev?part=3

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

* Re: [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and 128-bit arguments
  2026-07-10 18:38   ` sashiko-bot
@ 2026-07-10 19:13     ` Yonghong Song
  0 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2026-07-10 19:13 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: bpf



On 7/10/26 11:38 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The newly added `test_int128_args` selftest will predictably fail on `aarch64` due to an unhandled ABI alignment requirement in the ARM64 BPF trampoline.
> --
>
> --- Patch [3]: [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and 128-bit arguments ---
> commit c4e66c7551048f829ca6d77d8b1b2e6467e4525e
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> selftests/bpf: Add tests for >8 byte return value and 128-bit arguments
>
> The BPF trampoline preserves only 8 bytes of the target's return value
> (R0), so attaching an fexit/fmod_ret/fsession program to a function that
> returns a >8 byte value is now rejected by the verifier. Add a bpf_testmod
> function returning __int128 and an fexit program that targets it. The
> program is expected to fail to load with the "with a >8 byte return value
> is not supported for this attach type" message.
>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
>> index 6f8c0bfb04155..d32198ee02dff 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> [ ... ]
>> @@ -112,6 +113,39 @@ static void test_struct_many_args(void)
>>   	tracing_struct_many_args__destroy(skel);
>>   }
>>   
>> +static void test_int128_args(void)
>> +{
>> +	/*
>> +	 * __int128 arguments are passed in a register pair on x86_64 and
>> +	 * arm64, which the trampoline packs into two context slots. Other
>> +	 * architectures pass a __int128 differently (e.g. s390x passes larger
>> +	 * arguments by reference), so only exercise this on x86_64 and arm64.
>> +	 */
>> +#if defined(__x86_64__) || defined(__aarch64__)
>> +	struct tracing_struct_int128 *skel;
>> +	int err;
>> +
>> +	skel = tracing_struct_int128__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "tracing_struct_int128__open_and_load"))
>> +		return;
>> +
>> +	err = tracing_struct_int128__attach(skel);
>> +	if (!ASSERT_OK(err, "tracing_struct_int128__attach"))
>> +		goto destroy_skel;
>> +
>> +	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
>> +
>> +	ASSERT_EQ(skel->bss->t_a, 1, "t:a");
>> +	ASSERT_EQ(skel->bss->t_c, 3, "t:c");
>          ^^^^
>
> [Severity: Medium]
> Will this test predictably fail on AArch64?
>
> The test calls bpf_testmod_test_int128_arg(1, 2, 3). According to the AAPCS64
> calling convention, a 16-byte aligned argument (__int128) must be passed in an
> even-odd register pair. Thus, the integer 'a' goes to w0, x1 is skipped for
> alignment padding, the __int128 'b' goes to x2 and x3, and the long 'c' goes
> to x4.
>
> However, the pre-existing logic in the ARM64 BPF trampoline fails to account
> for this padding. It solely calculates register usage based on argument sizes:
>
> arch/arm64/net/bpf_jit_comp.c:calc_arg_aux() {
> 	...
> 	for (i = 0, nregs = 0; i < m->nr_args; i++) {
> 		slots = (m->arg_size[i] + 7) / 8;
> 		if (nregs + slots <= 8) /* passed through register ? */
> 			nregs += slots;
> 	...
> }
>
> Because it incorrectly assumes the arguments use exactly 4 contiguous
> registers (x0 through x3), it only saves those.
>
> As a result, when the test's BPF program reads ctx[3] expecting 'c', it
> actually reads x3 (the upper 64 bits of 'b', which is 0).
>
> Would it be better to fix the ARM64 trampoline padding handling first, or
> exclude AArch64 from this test for now to prevent CI failures?

Another arch difference between x86_64 and arm64. x84_64 allows __int128 at
any position in an array. But arm64 allows __int128 must be even position.

I will fix it.

>
>> +	ASSERT_EQ(skel->bss->t_ret, 6, "t ret");
>> +
>> +destroy_skel:
>> +	tracing_struct_int128__destroy(skel);
>> +#else
>> +	test__skip();
>> +#endif
>> +}


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

end of thread, other threads:[~2026-07-10 19:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-10 18:22 [PATCH bpf v2 0/3] bpf: Fix trampoline handling of 128-bit values Yonghong Song
2026-07-10 18:22 ` [PATCH bpf v2 1/3] bpf: Reject >8 byte return values on return-reading trampoline paths Yonghong Song
2026-07-10 18:22 ` [PATCH bpf v2 2/3] bpf, x86: Fix trampoline stack size for 128-bit arguments Yonghong Song
2026-07-10 18:22 ` [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and " Yonghong Song
2026-07-10 18:38   ` sashiko-bot
2026-07-10 19:13     ` Yonghong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox