Re: [PATCH bpf-next v4] bpftool: bpf skeletons assert type sizes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 23, 2022 at 2:02 PM Delyan Kratunov <delyank@xxxxxx> 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@xxxxxx>
> Acked-by: Hengqi Chen <hengqi.chen@xxxxxxxxx>
> Tested-by: Hengqi Chen <hengqi.chen@xxxxxxxxx>
> ---
> v3 -> v4:
>  - rebase
>  - style fixes
>

I added a few tweaks (see below) and applied to bpf-next. Thanks!

> v2 -> v3:
>  - group all static asserts in one function at the end of the file
>  - only use macros in C++ mode
>
> v1 -> v2:
>  - drop the stdint approach in favor of static asserts right after the structs
>
>  tools/bpf/bpftool/gen.c | 133 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 111 insertions(+), 22 deletions(-)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index f8c1413523a3..a42545bcb12d 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -209,15 +209,38 @@ static int codegen_datasec_def(struct bpf_object *obj,
>         return 0;
>  }
>
> +static const struct btf_type *find_type_for_map(struct bpf_object *obj,
> +                                               const char *map_ident)

this doesn't need entire bpf_object, it's enough to pass struct btf
directly, I changed it to take just btf

> +{
> +       struct btf *btf = bpf_object__btf(obj);
> +       int n = btf__type_cnt(btf), i;
> +       char sec_ident[256];
> +

[...]

> +/* Emit type size asserts for all top-level fields in memory-mapped internal maps.
> + */
> +static void codegen_asserts(struct bpf_object *obj, const char *obj_name)
> +{
> +       struct btf *btf = bpf_object__btf(obj);
> +       struct bpf_map *map;
> +       struct btf_var_secinfo *sec_var;
> +       int i, vlen;
> +       const struct btf_type *sec;
> +       char map_ident[256], var_ident[256];
> +
> +       codegen("\
> +               \n\
> +                                                                           \n\
> +               #ifdef __cplusplus                                          \n\
> +               #define _Static_assert static_assert                        \n\
> +               #endif                                                      \n\

I moved this _Static_assert business inside the <skel>__type_asserts()
function. I also thought that if in the future we want to add some
other asserts, then having a more generic "<skel>__assert()" name
would be more appropriate, so I renamed it to just "<skel>__assert".

> +                                                                           \n\
> +               __attribute__((unused)) static void                         \n\
> +               %1$s__type_asserts(struct %1$s *s)                          \n\
> +               {                                                           \n\
> +               ", obj_name);
> +

[...]

> +                       var_ident[0] = '\0';
> +                       strncat(var_ident, var_name, sizeof(var_ident) - 1);
> +                       sanitize_identifier(var_ident);
> +
> +                       printf("\t_Static_assert(sizeof(s->%1$s->%2$s) == %3$lld, \"unexpected size of '%2$s'\");\n",
> +                              map_ident, var_ident, var_size);

__s64 isn't %lld on each supported architecture. I just used long and
%ld instead of __s64 to avoid compilation warnings.

> +               }
> +       }
> +       codegen("\
> +               \n\
> +               }                                                           \n\
> +                                                                           \n\
> +               #ifdef __cplusplus                                          \n\
> +               #undef _Static_assert                                       \n\
> +               #endif                                                      \n\
> +               ");
> +}
> +
> +

[...]



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux