| Message ID | 6c673f48d35fd06bc3490b00d4e6527b7e180d59.1644884357.git.delyank@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Avoid size mismatches in skeletons | expand |
On Mon, Feb 14, 2022 at 4:27 PM Delyan Kratunov <delyank@fb.com> wrote: > > When emitting type declarations in skeletons, bpftool will now also emit > static assertions on the size of the data/bss/rodata/etc fields. This > ensures that in situations where userspace and kernel types have the same > name but differ in size we do not silently produce incorrect results but > instead break the build. > > This was reported in [1] and as expected the repro in [2] fails to build > on the new size assert after this change. > > [1]: Closes: https://github.com/libbpf/libbpf/issues/433 > [2]: https://github.com/fuweid/iovisor-bcc-pr-3777 > > Signed-off-by: Delyan Kratunov <delyank@fb.com> > --- > tools/bpf/bpftool/gen.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > index 6f2e20be0c62..e7f11899437a 100644 > --- a/tools/bpf/bpftool/gen.c > +++ b/tools/bpf/bpftool/gen.c > @@ -205,6 +205,29 @@ static int codegen_datasec_def(struct bpf_object *obj, > off = sec_var->offset + sec_var->size; > } > printf(" } *%s;\n", sec_ident); > + > + /* Walk through the section again to emit size asserts */ > + sec_var = btf_var_secinfos(sec); > + for (i = 0; i < vlen; i++, sec_var++) { > + const struct btf_type *var = btf__type_by_id(btf, sec_var->type); > + const char *var_name = btf__name_by_offset(btf, var->name_off); > + __u32 var_type_id = var->type; > + __s64 var_size = btf__resolve_size(btf, var_type_id); > + > + /* static variables are not exposed through BPF skeleton */ > + if (btf_var(var)->linkage == BTF_VAR_STATIC) > + continue; > + > + var_ident[0] = '\0'; > + strncat(var_ident, var_name, sizeof(var_ident) - 1); > + sanitize_identifier(var_ident); > + > + printf("\tBPF_STATIC_ASSERT("); > + printf("sizeof(((struct %s__%s*)0)->%s) == %lld, ", > + obj_name, sec_ident, var_ident, var_size); > + printf("\"unexpected size of field %s\");\n", var_ident); > + } > + So doing it right after each section really pollutes the layout of the skeleton's struct and hurts readability a lot. How about adding all those _Static_asserts in <skeleton__elf_bytes() function, after the huge binary dump, to get it out of sight? I think if we are doing asserts, we might as well validate that not just sizes, but also each variable's offset within the section is right. Those huge struct casts are also pretty verbose. What if we do something like this (assuming we are in a separate function, but we can easily just do that in __elf_bytes(). Let's use test_skeleton as skeleton name struct test_skeleton *s = (void *)0; _Static_assert(sizeof(s->data->in1) == 4, "invalid size of in1"); _Static_assert(offsetof(typeof(*skel->data), in1) == 0, "invalid offset of in1"); ... _Static_assert(sizeof(s->data_read_mostly->read_mostly_var) == 4, "invalid size of read_mostly_var"); _Static_assert(offsetof(typeof(*skel->data_read_mostly), read_mostly_var) == 0, "invalid offset of read_mostly_var"); (void)s; /* avoid unused variable warning */ WDYT? > return 0; > } > > @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv) > \n\ > #include <bpf/skel_internal.h> \n\ > \n\ > + #ifdef __cplusplus \n\ > + #define BPF_STATIC_ASSERT static_assert \n\ > + #else \n\ > + #define BPF_STATIC_ASSERT _Static_assert \n\ > + #endif \n\ Maybe just: #ifdef __cplusplus #define _Static_assert static_assert #endif ? Or that doesn't work? BPF_STATIC_ASSERT sounds very BPF-y, while this should stay within the skeleton. Also any such macro has to be #undef in this file, otherwise it will "leak" into the user's code (as this is just a header file included in user's .c files). > + \n\ > struct %1$s { \n\ > struct bpf_loader_ctx ctx; \n\ > ", > @@ -774,6 +803,12 @@ static int do_skeleton(int argc, char **argv) > #include <stdlib.h> \n\ > #include <bpf/libbpf.h> \n\ > \n\ > + #ifdef __cplusplus \n\ > + #define BPF_STATIC_ASSERT static_assert \n\ > + #else \n\ > + #define BPF_STATIC_ASSERT _Static_assert \n\ > + #endif \n\ > + \n\ > struct %1$s { \n\ > struct bpf_object_skeleton *skeleton; \n\ > struct bpf_object *obj; \n\ > -- > 2.34.1
On Mon, 2022-02-14 at 21:11 -0800, Andrii Nakryiko wrote: > So doing it right after each section really pollutes the layout of the > skeleton's struct and hurts readability a lot. > > How about adding all those _Static_asserts in <skeleton__elf_bytes() > function, after the huge binary dump, to get it out of sight? I can just add a `void __attribute__((unused)) skeleton__assert_sizes()` at the end? Or a `struct skeleton__type_asserts`? It feels weird to just put them in elf_bytes, they don't belong there. > I think > if we are doing asserts, we might as well validate that not just > sizes, but also each variable's offset within the section is right. Sure, can do. > _Static_assert(sizeof(s->data->in1) == 4, "invalid size of in1"); > _Static_assert(offsetof(typeof(*skel->data), in1) == 0, "invalid > offset of in1"); > ... > _Static_assert(sizeof(s->data_read_mostly->read_mostly_var) == 4, > "invalid size of read_mostly_var"); > _Static_assert(offsetof(typeof(*skel->data_read_mostly), > read_mostly_var) == 0, "invalid offset of read_mostly_var"); > > (void)s; /* avoid unused variable warning */ > > WDYT? That's fine by me, I have no objections. I'll see if a function or a struct is more readable. I suspect `SIZE_ASSERT(data, in1, 4); OFFSET_ASSERT(data, in1, 0);` is probably most readable but I hate that I'd have to include the macros inline (to emit the skeleton type name). > > return 0; > > } > > > > @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv) > > \n\ > > #include <bpf/skel_internal.h> \n\ > > \n\ > > + #ifdef __cplusplus \n\ > > + #define BPF_STATIC_ASSERT static_assert \n\ > > + #else \n\ > > + #define BPF_STATIC_ASSERT _Static_assert \n\ > > + #endif \n\ > > Maybe just: > > #ifdef __cplusplus > #define _Static_assert static_assert > #endif > > ? Or that doesn't work? It does work, it's just less explicit. I'd be happy to remove the macro expansion on the C path though, it would make diagnostics shorter. > Also any such macro has to be #undef in this file, otherwise it will > "leak" into the user's code (as this is just a header file included in > user's .c files). My bad, just thought of that too. -- To summarize, structurally I'll do this: 1. Put them all in one place. (tbd what type) 2. Put them at the end of the file. 3. Add offsets. 4. Fix up the macro usage.
On Tue, Feb 15, 2022 at 9:27 AM Delyan Kratunov <delyank@fb.com> wrote: > > On Mon, 2022-02-14 at 21:11 -0800, Andrii Nakryiko wrote: > > So doing it right after each section really pollutes the layout of the > > skeleton's struct and hurts readability a lot. > > > > How about adding all those _Static_asserts in <skeleton__elf_bytes() > > function, after the huge binary dump, to get it out of sight? > > I can just add a `void __attribute__((unused)) skeleton__assert_sizes()` at the > end? Or a `struct skeleton__type_asserts`? It feels weird to just put them in > elf_bytes, they don't belong there. SGTM. > > > I think > > if we are doing asserts, we might as well validate that not just > > sizes, but also each variable's offset within the section is right. > > Sure, can do. Alexei pointed out that it's very unlikely that we'll mess up offsets (we have actual offset from BTF and then we control alignment in skeleton's struct, so should never get out of sync), so let's skip offset assertion for now. > > > > _Static_assert(sizeof(s->data->in1) == 4, "invalid size of in1"); > > _Static_assert(offsetof(typeof(*skel->data), in1) == 0, "invalid > > offset of in1"); > > ... > > _Static_assert(sizeof(s->data_read_mostly->read_mostly_var) == 4, > > "invalid size of read_mostly_var"); > > _Static_assert(offsetof(typeof(*skel->data_read_mostly), > > read_mostly_var) == 0, "invalid offset of read_mostly_var"); > > > > (void)s; /* avoid unused variable warning */ > > > > WDYT? > > That's fine by me, I have no objections. I'll see if a function or a struct is > more readable. > > I suspect `SIZE_ASSERT(data, in1, 4); OFFSET_ASSERT(data, in1, 0);` is probably > most readable but I hate that I'd have to include the macros inline (to emit the > skeleton type name). No one should read those asserts, so putting them somewhere after elf_bytes function and writing out _Static_assert() directly is probably best for when one of those asserts fires. It will result in simpler compiler error (rather than unscrambling a chain of macro invocations). So yeah, I'd stick to a bit more verbose _Static_assert. > > > > return 0; > > > } > > > > > > @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv) > > > \n\ > > > #include <bpf/skel_internal.h> \n\ > > > \n\ > > > + #ifdef __cplusplus \n\ > > > + #define BPF_STATIC_ASSERT static_assert \n\ > > > + #else \n\ > > > + #define BPF_STATIC_ASSERT _Static_assert \n\ > > > + #endif \n\ > > > > Maybe just: > > > > #ifdef __cplusplus > > #define _Static_assert static_assert > > #endif > > > > ? Or that doesn't work? > > It does work, it's just less explicit. I'd be happy to remove the macro > expansion on the C path though, it would make diagnostics shorter. Yep, it was my thinking that we should "optimize" for pure C case. > > > > Also any such macro has to be #undef in this file, otherwise it will > > "leak" into the user's code (as this is just a header file included in > > user's .c files). > > My bad, just thought of that too. > > -- > > To summarize, structurally I'll do this: > > 1. Put them all in one place. (tbd what type) > 2. Put them at the end of the file. > 3. Add offsets. > 4. Fix up the macro usage. >
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c index 6f2e20be0c62..e7f11899437a 100644 --- a/tools/bpf/bpftool/gen.c +++ b/tools/bpf/bpftool/gen.c @@ -205,6 +205,29 @@ static int codegen_datasec_def(struct bpf_object *obj, off = sec_var->offset + sec_var->size; } printf(" } *%s;\n", sec_ident); + + /* Walk through the section again to emit size asserts */ + sec_var = btf_var_secinfos(sec); + for (i = 0; i < vlen; i++, sec_var++) { + const struct btf_type *var = btf__type_by_id(btf, sec_var->type); + const char *var_name = btf__name_by_offset(btf, var->name_off); + __u32 var_type_id = var->type; + __s64 var_size = btf__resolve_size(btf, var_type_id); + + /* static variables are not exposed through BPF skeleton */ + if (btf_var(var)->linkage == BTF_VAR_STATIC) + continue; + + var_ident[0] = '\0'; + strncat(var_ident, var_name, sizeof(var_ident) - 1); + sanitize_identifier(var_ident); + + printf("\tBPF_STATIC_ASSERT("); + printf("sizeof(((struct %s__%s*)0)->%s) == %lld, ", + obj_name, sec_ident, var_ident, var_size); + printf("\"unexpected size of field %s\");\n", var_ident); + } + return 0; } @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv) \n\ #include <bpf/skel_internal.h> \n\ \n\ + #ifdef __cplusplus \n\ + #define BPF_STATIC_ASSERT static_assert \n\ + #else \n\ + #define BPF_STATIC_ASSERT _Static_assert \n\ + #endif \n\ + \n\ struct %1$s { \n\ struct bpf_loader_ctx ctx; \n\ ", @@ -774,6 +803,12 @@ static int do_skeleton(int argc, char **argv) #include <stdlib.h> \n\ #include <bpf/libbpf.h> \n\ \n\ + #ifdef __cplusplus \n\ + #define BPF_STATIC_ASSERT static_assert \n\ + #else \n\ + #define BPF_STATIC_ASSERT _Static_assert \n\ + #endif \n\ + \n\ struct %1$s { \n\ struct bpf_object_skeleton *skeleton; \n\ struct bpf_object *obj; \n\
When emitting type declarations in skeletons, bpftool will now also emit static assertions on the size of the data/bss/rodata/etc fields. This ensures that in situations where userspace and kernel types have the same name but differ in size we do not silently produce incorrect results but instead break the build. This was reported in [1] and as expected the repro in [2] fails to build on the new size assert after this change. [1]: Closes: https://github.com/libbpf/libbpf/issues/433 [2]: https://github.com/fuweid/iovisor-bcc-pr-3777 Signed-off-by: Delyan Kratunov <delyank@fb.com> --- tools/bpf/bpftool/gen.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) -- 2.34.1