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

[Hengqi Chen <hengqi.chen@gmail.com>]

On 2/16/22 09:19, Andrii Nakryiko wrote:
> On Tue, Feb 15, 2022 at 4:12 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>
>> ---
> 
> LGTM with a trivial styling nits. But this doesn't apply cleanly to
> bpf-next (see [0]). Can you please rebase and resend. Also for
> single-patch submissions we don't require cover letter, to please just
> put all the description into one patch without cover letter.
> 
>   [0] https://github.com/kernel-patches/bpf/pull/2563#issuecomment-1040929960
> 
>>  tools/bpf/bpftool/gen.c | 134 +++++++++++++++++++++++++++++++++-------
>>  1 file changed, 112 insertions(+), 22 deletions(-)
> 
> [...]
> 
>> +
>> +       bpf_object__for_each_map(map, obj) {
>> +               if (!bpf_map__is_internal(map))
>> +                       continue;
>> +               if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
>> +                       continue;
>> +               if (!get_map_ident(map, map_ident, sizeof(map_ident)))
>> +                       continue;
>> +
>> +               sec = find_type_for_map(obj, map_ident);
>> +
> 
> nit: unnecessary empty line between assignment and "error checking"
> 
>> +               if (!sec) {
>> +                       /* best effort, couldn't find the type for this map */
>> +                       continue;
>> +               }
>> +
>> +               sec_var = btf_var_secinfos(sec);
>> +               vlen =  btf_vlen(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);
>> +
>> +                       if (var_size < 0)
>> +                               continue;
>> +
>> +                       /* 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("\t_Static_assert(");
>> +                       printf("sizeof(s->%1$s->%2$s) == %3$lld, ",
>> +                              map_ident, var_ident, var_size);
>> +                       printf("\"unexpected size of '%1$s'\");\n", var_ident);
> 
> nit: I'd keep this as one printf, it makes it a bit easier to follow.
> 
>> +               }
>> +       }
> 
> [...]

Feel free to add:

Acked-by: Hengqi Chen <hengqi.chen@gmail.com>
Tested-by: Hengqi Chen <hengqi.chen@gmail.com>