| Message ID | dcb8cfcd9946a937b8d4a93b9c42eaf3aad54038.1644453291.git.delyank@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Avoid typedef size mismatches in skeletons | expand |
On Wed, Feb 9, 2022 at 4:37 PM Delyan Kratunov <delyank@fb.com> wrote: > > Multiple test cases already fail if you add a type whose size is > different between userspace and bpf. That said, let's also add an > explicit test that ensures mis-sized reads/writes do not actually > happen. This test case fails before this patch series and passes after: > > test_skeleton:FAIL:writes and reads match size unexpected writes > and reads match size: actual 3735928559 != expected 8030895855 > test_skeleton:FAIL:skeleton uses underlying type unexpected > skeleton uses underlying type: actual 8 != expected 4 > > Signed-off-by: Delyan Kratunov <delyank@fb.com> > --- > tools/testing/selftests/bpf/prog_tests/skeleton.c | 6 ++++++ > tools/testing/selftests/bpf/progs/test_skeleton.c | 8 ++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c b/tools/testing/selftests/bpf/prog_tests/skeleton.c > index 9894e1b39211..bc07da929566 100644 > --- a/tools/testing/selftests/bpf/prog_tests/skeleton.c > +++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c > @@ -97,6 +97,9 @@ void test_skeleton(void) > > skel->data_read_mostly->read_mostly_var = 123; > > + /* validate apparent 64-bit value is actually 32-bit */ > + skel->data->intest64 = (typeof(skel->data->intest64)) 0xdeadbeefdeadbeefULL; > + > err = test_skeleton__attach(skel); > if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err)) > goto cleanup; > @@ -126,6 +129,9 @@ void test_skeleton(void) > ASSERT_OK_PTR(elf_bytes, "elf_bytes"); > ASSERT_GE(elf_bytes_sz, 0, "elf_bytes_sz"); > > + ASSERT_EQ(skel->data->outtest64, skel->data->intest64, "writes and reads match size"); > + ASSERT_EQ(sizeof(skel->data->intest64), sizeof(u32), "skeleton uses underlying type"); > + > cleanup: > test_skeleton__destroy(skel); > } > diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c > index 1b1187d2967b..fd1f4910cf42 100644 > --- a/tools/testing/selftests/bpf/progs/test_skeleton.c > +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c > @@ -16,6 +16,13 @@ struct s { > int in1 = -1; > long long in2 = -1; > > +/* declare the int64_t type to actually be 32-bit to ensure the skeleton > + * uses actual sizes and doesn't just copy the type name > + */ > +typedef __s32 int64_t; > +int64_t intest64 = -1; > +int64_t outtest64 = -1; This will be so confusing... But when you drop __s32 special handling you can just use __s32 directly, right? > + > /* .bss section */ > char in3 = '\0'; > long long in4 __attribute__((aligned(64))) = 0; > @@ -62,6 +69,7 @@ int handler(const void *ctx) > out4 = in4; > out5 = in5; > out6 = in.in6; > + outtest64 = intest64; > > bpf_syscall = CONFIG_BPF_SYSCALL; > kern_ver = LINUX_KERNEL_VERSION; > -- > 2.34.1
On Thu, 2022-02-10 at 14:44 -0800, Andrii Nakryiko wrote: > > +/* declare the int64_t type to actually be 32-bit to ensure the skeleton > > + * uses actual sizes and doesn't just copy the type name > > + */ > > +typedef __s32 int64_t; > > +int64_t intest64 = -1; > > +int64_t outtest64 = -1; > > This will be so confusing... But when you drop __s32 special handling > you can just use __s32 directly, right? Actually, no. We need the type to have the textual representation of a 64-bit type (int64_t) but a different underlying size. The test is ensuring that the skeleton is using the underlying type and not the apparent type. You need a layer of typedefs to introduce that confusion and intXX_t is ideal because it's not normally transitively available in .bpf.c programs. `typedef signed char int64_t;` would also work, if you prefer that.
diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c b/tools/testing/selftests/bpf/prog_tests/skeleton.c index 9894e1b39211..bc07da929566 100644 --- a/tools/testing/selftests/bpf/prog_tests/skeleton.c +++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c @@ -97,6 +97,9 @@ void test_skeleton(void) skel->data_read_mostly->read_mostly_var = 123; + /* validate apparent 64-bit value is actually 32-bit */ + skel->data->intest64 = (typeof(skel->data->intest64)) 0xdeadbeefdeadbeefULL; + err = test_skeleton__attach(skel); if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err)) goto cleanup; @@ -126,6 +129,9 @@ void test_skeleton(void) ASSERT_OK_PTR(elf_bytes, "elf_bytes"); ASSERT_GE(elf_bytes_sz, 0, "elf_bytes_sz"); + ASSERT_EQ(skel->data->outtest64, skel->data->intest64, "writes and reads match size"); + ASSERT_EQ(sizeof(skel->data->intest64), sizeof(u32), "skeleton uses underlying type"); + cleanup: test_skeleton__destroy(skel); } diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c index 1b1187d2967b..fd1f4910cf42 100644 --- a/tools/testing/selftests/bpf/progs/test_skeleton.c +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c @@ -16,6 +16,13 @@ struct s { int in1 = -1; long long in2 = -1; +/* declare the int64_t type to actually be 32-bit to ensure the skeleton + * uses actual sizes and doesn't just copy the type name + */ +typedef __s32 int64_t; +int64_t intest64 = -1; +int64_t outtest64 = -1; + /* .bss section */ char in3 = '\0'; long long in4 __attribute__((aligned(64))) = 0; @@ -62,6 +69,7 @@ int handler(const void *ctx) out4 = in4; out5 = in5; out6 = in.in6; + outtest64 = intest64; bpf_syscall = CONFIG_BPF_SYSCALL; kern_ver = LINUX_KERNEL_VERSION;
Multiple test cases already fail if you add a type whose size is different between userspace and bpf. That said, let's also add an explicit test that ensures mis-sized reads/writes do not actually happen. This test case fails before this patch series and passes after: test_skeleton:FAIL:writes and reads match size unexpected writes and reads match size: actual 3735928559 != expected 8030895855 test_skeleton:FAIL:skeleton uses underlying type unexpected skeleton uses underlying type: actual 8 != expected 4 Signed-off-by: Delyan Kratunov <delyank@fb.com> --- tools/testing/selftests/bpf/prog_tests/skeleton.c | 6 ++++++ tools/testing/selftests/bpf/progs/test_skeleton.c | 8 ++++++++ 2 files changed, 14 insertions(+) -- 2.34.1