| Message ID | b904faaff6e8a04809e722d33e062ad47e97c84e.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: > > As reported in [0] and [1], kernel and userspace can sometimes disagree > on the definition of a typedef (in particular, the size). > This leads to trouble when userspace maps the memory of a bpf program > and reads/writes to it assuming a different memory layout. > > This commit now uses the libbpf sized ints logic when emitting the > skeleton. This resolves int types to int32_t-like equivalents and > ensures that typedefs are not just emitted verbatim. > > The drive-by selftest changes fix format specifier issues > due to the definitions of [us]64 and (u)int64_t differing in how > many longs they use (long long int vs long int on x86_64). > > [0]: https://github.com/iovisor/bcc/pull/3777 > [1]: Closes: https://github.com/libbpf/libbpf/issues/433 > > Signed-off-by: Delyan Kratunov <delyank@fb.com> > --- > tools/bpf/bpftool/gen.c | 3 +++ > .../testing/selftests/bpf/prog_tests/skeleton.c | 16 ++++++++-------- > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > index eacfc6a2060d..18c3f755ad88 100644 > --- a/tools/bpf/bpftool/gen.c > +++ b/tools/bpf/bpftool/gen.c > @@ -146,6 +146,7 @@ static int codegen_datasec_def(struct bpf_object *obj, > .field_name = var_ident, > .indent_level = 2, > .strip_mods = strip_mods, > + .sizedints = true, > ); > int need_off = sec_var->offset, align_off, align; > __u32 var_type_id = var->type; > @@ -751,6 +752,7 @@ static int do_skeleton(int argc, char **argv) > #ifndef %2$s \n\ > #define %2$s \n\ > \n\ > + #include <inttypes.h> \n\ if Alexei's patch set will go in first (very likely), you'll need to rebase and make sure that you don't include either inttypes.h or stdint.h for kernel mode, as those headers don't exist there > #include <stdlib.h> \n\ > #include <bpf/bpf.h> \n\ > #include <bpf/skel_internal.h> \n\ > @@ -770,6 +772,7 @@ static int do_skeleton(int argc, char **argv) > #define %2$s \n\ > \n\ > #include <errno.h> \n\ > + #include <inttypes.h> \n\ seems like inttypes.h just includes stdint.h, I'd just include stdint.h directly > #include <stdlib.h> \n\ > #include <bpf/libbpf.h> \n\ > \n\ > diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c b/tools/testing/selftests/bpf/prog_tests/skeleton.c > index 180afd632f4c..9894e1b39211 100644 > --- a/tools/testing/selftests/bpf/prog_tests/skeleton.c > +++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c > @@ -43,13 +43,13 @@ void test_skeleton(void) > /* validate values are pre-initialized correctly */ > CHECK(data->in1 != -1, "in1", "got %d != exp %d\n", data->in1, -1); > CHECK(data->out1 != -1, "out1", "got %d != exp %d\n", data->out1, -1); > - CHECK(data->in2 != -1, "in2", "got %lld != exp %lld\n", data->in2, -1LL); > - CHECK(data->out2 != -1, "out2", "got %lld != exp %lld\n", data->out2, -1LL); > + CHECK(data->in2 != -1, "in2", "got %"PRId64" != exp %lld\n", data->in2, -1LL); > + CHECK(data->out2 != -1, "out2", "got %"PRId64" != exp %lld\n", data->out2, -1LL); we don't use PRIxxx ugliness anywhere in selftests or libbpf code base, it would be easier to just convert this to ASSERT_EQ() > > CHECK(bss->in3 != 0, "in3", "got %d != exp %d\n", bss->in3, 0); > CHECK(bss->out3 != 0, "out3", "got %d != exp %d\n", bss->out3, 0); > - CHECK(bss->in4 != 0, "in4", "got %lld != exp %lld\n", bss->in4, 0LL); > - CHECK(bss->out4 != 0, "out4", "got %lld != exp %lld\n", bss->out4, 0LL); > + CHECK(bss->in4 != 0, "in4", "got %"PRId64" != exp %lld\n", bss->in4, 0LL); > + CHECK(bss->out4 != 0, "out4", "got %"PRId64" != exp %lld\n", bss->out4, 0LL); [...]
On Thu, 2022-02-10 at 14:42 -0800, Andrii Nakryiko wrote: > if Alexei's patch set will go in first (very likely), you'll need to > rebase and make sure that you don't include either inttypes.h or > stdint.h for kernel mode, as those headers don't exist there Ack. > seems like inttypes.h just includes stdint.h, I'd just include stdint.h directly You need inttypes.h for the PRIxxx format specifiers and I figured users of skels are not unlikely to want to print things. Happy to move it to just stdint.h though. > we don't use PRIxxx ugliness anywhere in selftests or libbpf code > base, it would be easier to just convert this to ASSERT_EQ() > Sure, can do. >
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c index eacfc6a2060d..18c3f755ad88 100644 --- a/tools/bpf/bpftool/gen.c +++ b/tools/bpf/bpftool/gen.c @@ -146,6 +146,7 @@ static int codegen_datasec_def(struct bpf_object *obj, .field_name = var_ident, .indent_level = 2, .strip_mods = strip_mods, + .sizedints = true, ); int need_off = sec_var->offset, align_off, align; __u32 var_type_id = var->type; @@ -751,6 +752,7 @@ static int do_skeleton(int argc, char **argv) #ifndef %2$s \n\ #define %2$s \n\ \n\ + #include <inttypes.h> \n\ #include <stdlib.h> \n\ #include <bpf/bpf.h> \n\ #include <bpf/skel_internal.h> \n\ @@ -770,6 +772,7 @@ static int do_skeleton(int argc, char **argv) #define %2$s \n\ \n\ #include <errno.h> \n\ + #include <inttypes.h> \n\ #include <stdlib.h> \n\ #include <bpf/libbpf.h> \n\ \n\ diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c b/tools/testing/selftests/bpf/prog_tests/skeleton.c index 180afd632f4c..9894e1b39211 100644 --- a/tools/testing/selftests/bpf/prog_tests/skeleton.c +++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c @@ -43,13 +43,13 @@ void test_skeleton(void) /* validate values are pre-initialized correctly */ CHECK(data->in1 != -1, "in1", "got %d != exp %d\n", data->in1, -1); CHECK(data->out1 != -1, "out1", "got %d != exp %d\n", data->out1, -1); - CHECK(data->in2 != -1, "in2", "got %lld != exp %lld\n", data->in2, -1LL); - CHECK(data->out2 != -1, "out2", "got %lld != exp %lld\n", data->out2, -1LL); + CHECK(data->in2 != -1, "in2", "got %"PRId64" != exp %lld\n", data->in2, -1LL); + CHECK(data->out2 != -1, "out2", "got %"PRId64" != exp %lld\n", data->out2, -1LL); CHECK(bss->in3 != 0, "in3", "got %d != exp %d\n", bss->in3, 0); CHECK(bss->out3 != 0, "out3", "got %d != exp %d\n", bss->out3, 0); - CHECK(bss->in4 != 0, "in4", "got %lld != exp %lld\n", bss->in4, 0LL); - CHECK(bss->out4 != 0, "out4", "got %lld != exp %lld\n", bss->out4, 0LL); + CHECK(bss->in4 != 0, "in4", "got %"PRId64" != exp %lld\n", bss->in4, 0LL); + CHECK(bss->out4 != 0, "out4", "got %"PRId64" != exp %lld\n", bss->out4, 0LL); CHECK(rodata->in.in6 != 0, "in6", "got %d != exp %d\n", rodata->in.in6, 0); CHECK(bss->out6 != 0, "out6", "got %d != exp %d\n", bss->out6, 0); @@ -77,9 +77,9 @@ void test_skeleton(void) /* validate pre-setup values are still there */ CHECK(data->in1 != 10, "in1", "got %d != exp %d\n", data->in1, 10); - CHECK(data->in2 != 11, "in2", "got %lld != exp %lld\n", data->in2, 11LL); + CHECK(data->in2 != 11, "in2", "got %"PRId64" != exp %lld\n", data->in2, 11LL); CHECK(bss->in3 != 12, "in3", "got %d != exp %d\n", bss->in3, 12); - CHECK(bss->in4 != 13, "in4", "got %lld != exp %lld\n", bss->in4, 13LL); + CHECK(bss->in4 != 13, "in4", "got %"PRId64" != exp %lld\n", bss->in4, 13LL); CHECK(rodata->in.in6 != 14, "in6", "got %d != exp %d\n", rodata->in.in6, 14); ASSERT_EQ(rodata_dyn->in_dynarr_sz, 4, "in_dynarr_sz"); @@ -105,9 +105,9 @@ void test_skeleton(void) usleep(1); CHECK(data->out1 != 1, "res1", "got %d != exp %d\n", data->out1, 1); - CHECK(data->out2 != 2, "res2", "got %lld != exp %d\n", data->out2, 2); + CHECK(data->out2 != 2, "res2", "got %"PRId64" != exp %d\n", data->out2, 2); CHECK(bss->out3 != 3, "res3", "got %d != exp %d\n", (int)bss->out3, 3); - CHECK(bss->out4 != 4, "res4", "got %lld != exp %d\n", bss->out4, 4); + CHECK(bss->out4 != 4, "res4", "got %"PRId64" != exp %d\n", bss->out4, 4); CHECK(bss->out5.a != 5, "res5", "got %d != exp %d\n", bss->out5.a, 5); CHECK(bss->out5.b != 6, "res6", "got %lld != exp %d\n", bss->out5.b, 6); CHECK(bss->out6 != 14, "res7", "got %d != exp %d\n", bss->out6, 14);
As reported in [0] and [1], kernel and userspace can sometimes disagree on the definition of a typedef (in particular, the size). This leads to trouble when userspace maps the memory of a bpf program and reads/writes to it assuming a different memory layout. This commit now uses the libbpf sized ints logic when emitting the skeleton. This resolves int types to int32_t-like equivalents and ensures that typedefs are not just emitted verbatim. The drive-by selftest changes fix format specifier issues due to the definitions of [us]64 and (u)int64_t differing in how many longs they use (long long int vs long int on x86_64). [0]: https://github.com/iovisor/bcc/pull/3777 [1]: Closes: https://github.com/libbpf/libbpf/issues/433 Signed-off-by: Delyan Kratunov <delyank@fb.com> --- tools/bpf/bpftool/gen.c | 3 +++ .../testing/selftests/bpf/prog_tests/skeleton.c | 16 ++++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) -- 2.34.1