Re: [PATCH bpf-next v2 1/1] bpftool: bpf skeletons assert type sizes

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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