When dealing with large types (>8 bytes), ARM64 trampolines need to take
extra care about the arguments alignment to respect the calling
convention set by AAPCS64.
Add two tests ensuring that the BPF trampoline arranges arguments with
the relevant layout. The two new tests involve almost the same
arguments, except that the second one requires a more specific alignment
to be set by the trampoline when preparing arguments before calling the
the target function.
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
.../selftests/bpf/prog_tests/tracing_struct.c | 23 ++++++++
tools/testing/selftests/bpf/progs/tracing_struct.c | 10 +++-
.../selftests/bpf/progs/tracing_struct_many_args.c | 67 ++++++++++++++++++++++
.../testing/selftests/bpf/test_kmods/bpf_testmod.c | 50 ++++++++++++++++
4 files changed, 149 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
index 19e68d4b353278bf8e2917e62f62c89d14d7fe80..ecb5d38539f8b2fc275f93713ce2a7aad908b929 100644
--- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
@@ -108,6 +108,29 @@ static void test_struct_many_args(void)
ASSERT_EQ(skel->bss->t9_i, 27, "t9:i");
ASSERT_EQ(skel->bss->t9_ret, 258, "t9 ret");
+ ASSERT_EQ(skel->bss->t10_a_a, 27, "t10:a.a");
+ ASSERT_EQ(skel->bss->t10_a_b, 28, "t10:a.b");
+ ASSERT_EQ(skel->bss->t10_b_a, 29, "t10:b.a");
+ ASSERT_EQ(skel->bss->t10_b_b, 30, "t10:b.b");
+ ASSERT_EQ(skel->bss->t10_c_a, 31, "t10:c.a");
+ ASSERT_EQ(skel->bss->t10_c_b, 32, "t10:c.b");
+ ASSERT_EQ(skel->bss->t10_d_a, 33, "t10:d.a");
+ ASSERT_EQ(skel->bss->t10_d_b, 34, "t10:d.b");
+ ASSERT_EQ(skel->bss->t10_e, 35, "t10:e");
+ ASSERT_EQ(skel->bss->t10_f_a, 36, "t10:f.a");
+ ASSERT_EQ(skel->bss->t10_f_b, 37, "t10:f.b");
+
+ ASSERT_EQ(skel->bss->t10_ret, 352, "t10 ret");
+
+ ASSERT_EQ(skel->bss->t11_a, 38, "t11:a");
+ ASSERT_EQ(skel->bss->t11_b, 39, "t11:b");
+ ASSERT_EQ(skel->bss->t11_c, 40, "t11:c");
+ ASSERT_EQ(skel->bss->t11_d, 41, "t11:d");
+ ASSERT_EQ(skel->bss->t11_e, 42, "t11:e");
+ ASSERT_EQ(skel->bss->t11_f, 43, "t11:f");
+
+ ASSERT_EQ(skel->bss->t11_ret, 243, "t11 ret");
+
destroy_skel:
tracing_struct_many_args__destroy(skel);
}
diff --git a/tools/testing/selftests/bpf/progs/tracing_struct.c b/tools/testing/selftests/bpf/progs/tracing_struct.c
index c435a3a8328ab1580c63967a7f0ab779aa7ca4d4..feabf618a2e011b0d08eeaa6cc09fba1922ecb3f 100644
--- a/tools/testing/selftests/bpf/progs/tracing_struct.c
+++ b/tools/testing/selftests/bpf/progs/tracing_struct.c
@@ -18,6 +18,15 @@ struct bpf_testmod_struct_arg_3 {
int b[];
};
+struct bpf_testmod_struct_arg_4 {
+ __u64 a;
+ __u64 b;
+};
+
+struct bpf_testmod_struct_arg_5 {
+ __int128 a;
+};
+
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;
@@ -25,7 +34,6 @@ long t3_a, t3_b, t3_c_a, t3_c_b, t3_ret;
long t4_a_a, t4_b, t4_c, t4_d, t4_e_a, t4_e_b, t4_ret;
long t5_ret;
int t6;
-
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)
{
diff --git a/tools/testing/selftests/bpf/progs/tracing_struct_many_args.c b/tools/testing/selftests/bpf/progs/tracing_struct_many_args.c
index 4742012ace06af949d7f15a21131aaef7ab006e4..6f086b3d32d5f89e426aa4b79daa24bdb42861db 100644
--- a/tools/testing/selftests/bpf/progs/tracing_struct_many_args.c
+++ b/tools/testing/selftests/bpf/progs/tracing_struct_many_args.c
@@ -18,6 +18,14 @@ struct bpf_testmod_struct_arg_5 {
long t7_a, t7_b, t7_c, t7_d, t7_e, t7_f_a, t7_f_b, t7_ret;
long t8_a, t8_b, t8_c, t8_d, t8_e, t8_f_a, t8_f_b, t8_g, t8_ret;
long t9_a, t9_b, t9_c, t9_d, t9_e, t9_f, t9_g, t9_h_a, t9_h_b, t9_h_c, t9_h_d, t9_i, t9_ret;
+__u64 t10_a_a, t10_a_b, t10_b_a, t10_b_b, t10_c_a, t10_c_b, t10_d_a, t10_d_b,
+ t10_f_a, t10_f_b;
+short t10_e;
+int t10_ret;
+__int128 t11_a, t11_b, t11_c, t11_d, t11_f;
+short t11_e;
+int t11_ret;
+
SEC("fentry/bpf_testmod_test_struct_arg_7")
int BPF_PROG2(test_struct_many_args_1, __u64, a, void *, b, short, c, int, d,
@@ -92,4 +100,63 @@ int BPF_PROG2(test_struct_many_args_6, __u64, a, void *, b, short, c, int, d, vo
return 0;
}
+SEC("fentry/bpf_testmod_test_struct_arg_10")
+int BPF_PROG2(test_struct_many_args_7, struct bpf_testmod_struct_arg_4, a,
+ struct bpf_testmod_struct_arg_4, b,
+ struct bpf_testmod_struct_arg_4, c,
+ struct bpf_testmod_struct_arg_4, d, short, e,
+ struct bpf_testmod_struct_arg_4, f)
+{
+ t10_a_a = a.a;
+ t10_a_b = a.b;
+ t10_b_a = b.a;
+ t10_b_b = b.b;
+ t10_c_a = c.a;
+ t10_c_b = c.b;
+ t10_d_a = d.a;
+ t10_d_b = d.b;
+ t10_e = e;
+ t10_f_a = f.a;
+ t10_f_b = f.b;
+ return 0;
+}
+
+SEC("fexit/bpf_testmod_test_struct_arg_10")
+int BPF_PROG2(test_struct_many_args_8, struct bpf_testmod_struct_arg_4, a,
+ struct bpf_testmod_struct_arg_4, b,
+ struct bpf_testmod_struct_arg_4, c,
+ struct bpf_testmod_struct_arg_4, d, short, e,
+ struct bpf_testmod_struct_arg_4, f, int, ret)
+{
+ t10_ret = ret;
+ return 0;
+}
+
+SEC("fentry/bpf_testmod_test_struct_arg_11")
+int BPF_PROG2(test_struct_many_args_9, struct bpf_testmod_struct_arg_5, a,
+ struct bpf_testmod_struct_arg_5, b,
+ struct bpf_testmod_struct_arg_5, c,
+ struct bpf_testmod_struct_arg_5, d, int, e,
+ struct bpf_testmod_struct_arg_5, f)
+{
+ t11_a = a.a;
+ t11_b = b.a;
+ t11_c = c.a;
+ t11_d = d.a;
+ t11_e = e;
+ t11_f = f.a;
+ return 0;
+}
+
+SEC("fexit/bpf_testmod_test_struct_arg_11")
+int BPF_PROG2(test_struct_many_args_10, struct bpf_testmod_struct_arg_5, a,
+ struct bpf_testmod_struct_arg_5, b,
+ struct bpf_testmod_struct_arg_5, c,
+ struct bpf_testmod_struct_arg_5, d, int, e,
+ struct bpf_testmod_struct_arg_5, f, int, ret)
+{
+ t11_ret = ret;
+ 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 f38eaf0d35efa712ec288f06983c86b02c0d3e0e..96c80da725c978f2e48df8602dd63155971a7bf6 100644
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
@@ -62,6 +62,15 @@ struct bpf_testmod_struct_arg_5 {
long d;
};
+struct bpf_testmod_struct_arg_6 {
+ u64 a;
+ u64 b;
+};
+
+struct bpf_testmod_struct_arg_7 {
+ __int128 a;
+};
+
__bpf_hook_start();
noinline int
@@ -128,6 +137,29 @@ 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_struct_arg_10(struct bpf_testmod_struct_arg_6 a,
+ struct bpf_testmod_struct_arg_6 b,
+ struct bpf_testmod_struct_arg_6 c,
+ struct bpf_testmod_struct_arg_6 d,
+ short e,
+ struct bpf_testmod_struct_arg_6 f)
+{
+ bpf_testmod_test_struct_arg_result =
+ a.a + a.b + b.a + b.b + c.a + c.b + d.a + d.b + e + f.a + f.b;
+ return bpf_testmod_test_struct_arg_result;
+}
+
+noinline int bpf_testmod_test_struct_arg_11(struct bpf_testmod_struct_arg_7 a,
+ struct bpf_testmod_struct_arg_7 b,
+ struct bpf_testmod_struct_arg_7 c,
+ struct bpf_testmod_struct_arg_7 d,
+ short e,
+ struct bpf_testmod_struct_arg_7 f)
+{
+ bpf_testmod_test_struct_arg_result = a.a + b.a + c.a + d.a + e + f.a;
+ 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;
@@ -394,6 +426,16 @@ 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};
+ struct bpf_testmod_struct_arg_6 struct_arg6_a = {27, 28};
+ struct bpf_testmod_struct_arg_6 struct_arg6_b = {29, 30};
+ struct bpf_testmod_struct_arg_6 struct_arg6_c = {31, 32};
+ struct bpf_testmod_struct_arg_6 struct_arg6_d = {33, 34};
+ struct bpf_testmod_struct_arg_6 struct_arg6_f = {36, 37};
+ struct bpf_testmod_struct_arg_7 struct_arg7_a = {38};
+ struct bpf_testmod_struct_arg_7 struct_arg7_b = {39};
+ struct bpf_testmod_struct_arg_7 struct_arg7_c = {40};
+ struct bpf_testmod_struct_arg_7 struct_arg7_d = {41};
+ struct bpf_testmod_struct_arg_7 struct_arg7_f = {43};
int i = 1;
while (bpf_testmod_return_ptr(i))
@@ -411,6 +453,14 @@ 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_struct_arg_10(struct_arg6_a, struct_arg6_b,
+ struct_arg6_c, struct_arg6_d, 35,
+ struct_arg6_f);
+
+ (void)bpf_testmod_test_struct_arg_11(struct_arg7_a, struct_arg7_b,
+ struct_arg7_c, struct_arg7_d, 42,
+ struct_arg7_f);
+
(void)bpf_testmod_test_arg_ptr_to_struct(&struct_arg1_2);
(void)trace_bpf_testmod_test_raw_tp_null(NULL);
--
2.49.0
On Fri, 2025-04-11 at 22:32 +0200, Alexis Lothoré (eBPF Foundation) wrote:
> When dealing with large types (>8 bytes), ARM64 trampolines need to take
> extra care about the arguments alignment to respect the calling
> convention set by AAPCS64.
>
> Add two tests ensuring that the BPF trampoline arranges arguments with
> the relevant layout. The two new tests involve almost the same
> arguments, except that the second one requires a more specific alignment
> to be set by the trampoline when preparing arguments before calling the
> the target function.
>
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
> ---
[...]
> +SEC("fentry/bpf_testmod_test_struct_arg_11")
> +int BPF_PROG2(test_struct_many_args_9, struct bpf_testmod_struct_arg_5, a,
> + struct bpf_testmod_struct_arg_5, b,
> + struct bpf_testmod_struct_arg_5, c,
> + struct bpf_testmod_struct_arg_5, d, int, e,
> + struct bpf_testmod_struct_arg_5, f)
Hello Alexis,
I'm trying to double check the error you've seen for x86.
I see that tracing_struct/struct_many_args fails with assertion:
"test_struct_many_args:FAIL:t11:f unexpected t11:f: actual 35 != expected 43".
Could you please help me understand this test?
The function listened to is defined as accepting 'struct bpf_testmod_struct_arg_7',
at the same time this function uses 'struct bpf_testmod_struct_arg_5'.
Nevertheless, the assertion persists even with correct types.
> +{
> + t11_a = a.a;
> + t11_b = b.a;
> + t11_c = c.a;
> + t11_d = d.a;
> + t11_e = e;
> + t11_f = f.a;
> + return 0;
> +}
[...]
Hello Eduard,
On Mon Apr 28, 2025 at 9:01 AM CEST, Eduard Zingerman wrote:
> On Fri, 2025-04-11 at 22:32 +0200, Alexis Lothoré (eBPF Foundation) wrote:
>> When dealing with large types (>8 bytes), ARM64 trampolines need to take
>> extra care about the arguments alignment to respect the calling
>> convention set by AAPCS64.
>>
>> Add two tests ensuring that the BPF trampoline arranges arguments with
>> the relevant layout. The two new tests involve almost the same
>> arguments, except that the second one requires a more specific alignment
>> to be set by the trampoline when preparing arguments before calling the
>> the target function.
>>
>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
>> ---
>
> [...]
>
>> +SEC("fentry/bpf_testmod_test_struct_arg_11")
>> +int BPF_PROG2(test_struct_many_args_9, struct bpf_testmod_struct_arg_5, a,
>> + struct bpf_testmod_struct_arg_5, b,
>> + struct bpf_testmod_struct_arg_5, c,
>> + struct bpf_testmod_struct_arg_5, d, int, e,
>> + struct bpf_testmod_struct_arg_5, f)
>
> Hello Alexis,
>
> I'm trying to double check the error you've seen for x86.
Thanks for taking a look at this.
> I see that tracing_struct/struct_many_args fails with assertion:
> "test_struct_many_args:FAIL:t11:f unexpected t11:f: actual 35 != expected 43".
> Could you please help me understand this test?
Sure. When we attach fentry/fexit programs to a kernel function, a small
trampoline is generated to handle the attached programs execution. This
trampoline has to:
1. properly read and aggregate the functions arguments into a bpf tracing
context. Those arguments comes from registers, and possibly from the stack
if not all function arguments fit in registers
2. if the trampoline is in charge of calling the target function (that's
the case for example when we have both a fentry and a fexit programs
attached), it must prepare the arguments for the target function, by
filling again the registers, and possibly pushing the additional arguments
on the stack at the relevant offset.
This test is about validating the first point: ensuring that the trampoline
has correctly read the initial arguments from the target function and
forwarded them to the fentry program. So the actual test triggers the
targeted function, it triggers the attached fentry program which record the
values passed by the trampoline in t11_a, t11_b, etc, and then the
test checks back that those "spied" values are the one that it has actually
passed when executing the target function.
> The function listened to is defined as accepting 'struct bpf_testmod_struct_arg_7',
> at the same time this function uses 'struct bpf_testmod_struct_arg_5'.
That's not an accidental mistake, those are in fact the same definition.
bpf_testmod_struct_arg_7 is the kernel side definition in bpf_testmod.c:
struct bpf_testmod_struct_arg_7 {
__int128 a;
};
and struct bpf_testmode_struct_arg_5 is the one defined in the bpf test
program:
struct bpf_testmod_struct_arg_5 {
__int128 a;
};
I agree while being correct, this is confusing :/ I have kind of blindly
applied the logic I have observed in here for declaring test structs, and
so declared it twice (in kernel part and in bpf part), just incrementing
the index of the last defined structure. But I guess it could benefit from
some index syncing, or better, some refactoring to properly share those
declarations. I'll think about it for a new rev.
> Nevertheless, the assertion persists even with correct types.
So I digged a bit further to better share my observations here. This is the
function stack when entering the trampoline after having triggered the
target function execution:
(gdb) x/64b $rbp+0x18
0xffffc9000015fd60: 41 0 0 0 0 0 0 0
0xffffc9000015fd68: 0 0 0 0 0 0 0 0
0xffffc9000015fd70: 42 0 0 0 0 0 0 0
0xffffc9000015fd78: 35 0 0 0 0 0 0 0
0xffffc9000015fd80: 43 0 0 0 0 0 0 0
0xffffc9000015fd88: 0 0 0 0 0 0 0 0
We see the arguments that did not fit in registers, so d, e and f.
This is the ebpf context generated by the trampoline for the fentry
program, from the content of the stack above + the registers:
(gdb) x/128b $rbp-60
0xffffc9000015fce8: 38 0 0 0 0 0 0 0
0xffffc9000015fcf0: 0 0 0 0 0 0 0 0
0xffffc9000015fcf8: 39 0 0 0 0 0 0 0
0xffffc9000015fd00: 0 0 0 0 0 0 0 0
0xffffc9000015fd08: 40 0 0 0 0 0 0 0
0xffffc9000015fd10: 0 0 0 0 0 0 0 0
0xffffc9000015fd18: 41 0 0 0 0 0 0 0
0xffffc9000015fd20: 0 0 0 0 0 0 0 0
0xffffc9000015fd28: 42 0 0 0 0 0 0 0
0xffffc9000015fd30: 35 0 0 0 0 0 0 0
0xffffc9000015fd38: 43 0 0 0 0 0 0 0
0xffffc9000015fd40: 37 0 0 0 0 0 0 0
So IIUC, this is wrong because the "e" variable in the bpf program being
an int (and about to receive value 42), it occupies only 1 "tracing context
8-byte slot", so the value 43 (representing the content for variable f),
should be right after it, at 0xffffc9000015fd30. What we have instead is a
hole, very likely because we copied silently the alignment from the
original function call (and I guess this 35 value is a remnant from the
previous test, which uses values from 27 to 37)
Regardless of this issue, based on discussion from last week, I think I'll
go for the implementation suggested by Alexei: handling the nominal cases,
and detecting and blocking the non trivial cases (eg: structs passed on
stack). It sounds reasonable as there seems to be no exisiting kernel
function currently able to trigger those very specific cases, so it could
be added later if this changes.
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Alexis Lothoré <alexis.lothore@bootlin.com> writes:
[...]
>> The function listened to is defined as accepting 'struct bpf_testmod_struct_arg_7',
>> at the same time this function uses 'struct bpf_testmod_struct_arg_5'.
>
> That's not an accidental mistake, those are in fact the same definition.
> bpf_testmod_struct_arg_7 is the kernel side definition in bpf_testmod.c:
>
> struct bpf_testmod_struct_arg_7 {
> __int128 a;
> };
>
> and struct bpf_testmode_struct_arg_5 is the one defined in the bpf test
> program:
>
> struct bpf_testmod_struct_arg_5 {
> __int128 a;
> };
Apologies, but I'm still confused:
- I apply this series on top of:
224ee86639f5 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf after rc4")
- line 12 of tracing_struct_many_args.c has the following definition:
struct bpf_testmod_struct_arg_5 {
char a;
short b;
int c;
long d;
};
- line 135 of the same file has the following definition:
SEC("fentry/bpf_testmod_test_struct_arg_11")
int BPF_PROG2(test_struct_many_args_9, struct bpf_testmod_struct_arg_5, a,
struct bpf_testmod_struct_arg_5, b,
struct bpf_testmod_struct_arg_5, c,
struct bpf_testmod_struct_arg_5, d, int, e,
struct bpf_testmod_struct_arg_5, f)
- line 70 of tools/testing/selftests/bpf/test_kmods/bpf_testmod.c:
struct bpf_testmod_struct_arg_7 {
__int128 a;
};
- line 152 of the same file:
noinline int bpf_testmod_test_struct_arg_11(struct bpf_testmod_struct_arg_7 a,
struct bpf_testmod_struct_arg_7 b,
struct bpf_testmod_struct_arg_7 c,
struct bpf_testmod_struct_arg_7 d,
short e,
struct bpf_testmod_struct_arg_7 f)
Do I use a wrong base to apply the series?
[...]
>> Nevertheless, the assertion persists even with correct types.
>
> So I digged a bit further to better share my observations here. This is the
> function stack when entering the trampoline after having triggered the
> target function execution:
>
> (gdb) x/64b $rbp+0x18
> 0xffffc9000015fd60: 41 0 0 0 0 0 0 0
> 0xffffc9000015fd68: 0 0 0 0 0 0 0 0
> 0xffffc9000015fd70: 42 0 0 0 0 0 0 0
> 0xffffc9000015fd78: 35 0 0 0 0 0 0 0
> 0xffffc9000015fd80: 43 0 0 0 0 0 0 0
> 0xffffc9000015fd88: 0 0 0 0 0 0 0 0
>
> We see the arguments that did not fit in registers, so d, e and f.
>
> This is the ebpf context generated by the trampoline for the fentry
> program, from the content of the stack above + the registers:
>
> (gdb) x/128b $rbp-60
> 0xffffc9000015fce8: 38 0 0 0 0 0 0 0
> 0xffffc9000015fcf0: 0 0 0 0 0 0 0 0
> 0xffffc9000015fcf8: 39 0 0 0 0 0 0 0
> 0xffffc9000015fd00: 0 0 0 0 0 0 0 0
> 0xffffc9000015fd08: 40 0 0 0 0 0 0 0
> 0xffffc9000015fd10: 0 0 0 0 0 0 0 0
> 0xffffc9000015fd18: 41 0 0 0 0 0 0 0
> 0xffffc9000015fd20: 0 0 0 0 0 0 0 0
> 0xffffc9000015fd28: 42 0 0 0 0 0 0 0
> 0xffffc9000015fd30: 35 0 0 0 0 0 0 0
> 0xffffc9000015fd38: 43 0 0 0 0 0 0 0
> 0xffffc9000015fd40: 37 0 0 0 0 0 0 0
>
> So IIUC, this is wrong because the "e" variable in the bpf program being
> an int (and about to receive value 42), it occupies only 1 "tracing context
> 8-byte slot", so the value 43 (representing the content for variable f),
> should be right after it, at 0xffffc9000015fd30. What we have instead is a
> hole, very likely because we copied silently the alignment from the
> original function call (and I guess this 35 value is a remnant from the
> previous test, which uses values from 27 to 37)
Interesting, thank you for the print outs.
> Regardless of this issue, based on discussion from last week, I think I'll
> go for the implementation suggested by Alexei: handling the nominal cases,
> and detecting and blocking the non trivial cases (eg: structs passed on
> stack). It sounds reasonable as there seems to be no exisiting kernel
> function currently able to trigger those very specific cases, so it could
> be added later if this changes.
Yes, this makes sense.
On Mon Apr 28, 2025 at 6:52 PM CEST, Eduard Zingerman wrote:
> Alexis Lothoré <alexis.lothore@bootlin.com> writes:
>
> [...]
>
>>> The function listened to is defined as accepting 'struct bpf_testmod_struct_arg_7',
>>> at the same time this function uses 'struct bpf_testmod_struct_arg_5'.
>>
>> That's not an accidental mistake, those are in fact the same definition.
>> bpf_testmod_struct_arg_7 is the kernel side definition in bpf_testmod.c:
>>
>> struct bpf_testmod_struct_arg_7 {
>> __int128 a;
>> };
>>
>> and struct bpf_testmode_struct_arg_5 is the one defined in the bpf test
>> program:
>>
>> struct bpf_testmod_struct_arg_5 {
>> __int128 a;
>> };
>
> Apologies, but I'm still confused:
> - I apply this series on top of:
> 224ee86639f5 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf after rc4")
>
> - line 12 of tracing_struct_many_args.c has the following definition:
>
> struct bpf_testmod_struct_arg_5 {
> char a;
> short b;
> int c;
> long d;
> };
>
> - line 135 of the same file has the following definition:
>
> SEC("fentry/bpf_testmod_test_struct_arg_11")
> int BPF_PROG2(test_struct_many_args_9, struct bpf_testmod_struct_arg_5, a,
> struct bpf_testmod_struct_arg_5, b,
> struct bpf_testmod_struct_arg_5, c,
> struct bpf_testmod_struct_arg_5, d, int, e,
> struct bpf_testmod_struct_arg_5, f)
>
> - line 70 of tools/testing/selftests/bpf/test_kmods/bpf_testmod.c:
>
> struct bpf_testmod_struct_arg_7 {
> __int128 a;
> };
>
> - line 152 of the same file:
>
> noinline int bpf_testmod_test_struct_arg_11(struct bpf_testmod_struct_arg_7 a,
> struct bpf_testmod_struct_arg_7 b,
> struct bpf_testmod_struct_arg_7 c,
> struct bpf_testmod_struct_arg_7 d,
> short e,
> struct bpf_testmod_struct_arg_7 f)
>
> Do I use a wrong base to apply the series?
Argh, no, no, you are right, I checked again and I made some confusions
between progs/tracing_struct.c and progs/tracing_struct_many_args.c. I
initially did most of the work in tracing_struct.c, and eventually moved
the code to tracing_struct_many_args.c before sending my series, but I
apparently forgot to move bpf_testmod_struct_arg_5 declaration in
tracing_struct_many_args.c (and so, to rename it, since this name is
already used in there). As a consequence the bpf program is actually using
the wrong struct layout. So thanks for insisting and spotting this issue !
I fixed my mess locally in order to re-run the gdb analysis mentioned in my
previous mail, and the bug seems to be the same (unexpected t11:f: actual
35 != expected 43), with the same layout issue on the bpf context passed on
the stack ("lucky" mistake ?). However, thinking more about this, I feel
like there is still something that I have missed:
0xffffc900001dbce8: 38 0 0 0 0 0 0 0
0xffffc900001dbcf0: 0 0 0 0 0 0 0 0
0xffffc900001dbcf8: 39 0 0 0 0 0 0 0
0xffffc900001dbd00: 0 0 0 0 0 0 0 0
0xffffc900001dbd08: 40 0 0 0 0 0 0 0
0xffffc900001dbd10: 0 0 0 0 0 0 0 0
0xffffc900001dbd18: 41 0 0 0 0 0 0 0
0xffffc900001dbd20: 0 0 0 0 0 0 0 0
0xffffffffc04016a6: 42 0 0 0 0 0 0 0
0xffffc900001dbd30: 35 0 0 0 0 0 0 0
0xffffc900001dbd38: 43 0 0 0 0 0 0 0
0xffffc900001dbd40: 37 0 0 0 0 0 0 0
If things really behaved correctly, f would not have the correct value but
would still be handled as a 16 bytes value, so the test would not fail with
"actual 35 != 43", but something like "actual
27254487904906932132179118915584 != 43" (43 << 64 | 35) I guess. I still
need to sort this out.
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon Apr 28, 2025 at 10:41 PM CEST, Alexis Lothoré wrote: > On Mon Apr 28, 2025 at 6:52 PM CEST, Eduard Zingerman wrote: >> Alexis Lothoré <alexis.lothore@bootlin.com> writes: > If things really behaved correctly, f would not have the correct value but > would still be handled as a 16 bytes value, so the test would not fail with > "actual 35 != 43", but something like "actual > 27254487904906932132179118915584 != 43" (43 << 64 | 35) I guess. I still > need to sort this out. And so indeed, the broken value is a big one: (gdb) p skel->bss->t11_f $4 = 793209995169510719523 (gdb) p/x skel->bss->t11_f $5 = 0x2b0000000000000023 (gdb) But we see the 35 (0x23) value in the error log because the formatters used in ASSERT_EQ truncate the actual value. Alexis -- Alexis Lothoré, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
© 2016 - 2026 Red Hat, Inc.