Re: [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 04/04/2025 19:39, Andrii Nakryiko wrote:
> On Mon, Mar 31, 2025 at 2:12 PM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> From: Mykyta Yatsenko<yatsenko@meta.com>
>>
>> Extend commit e3c9abd0d14b ("selftests/bpf: Implement setting global
>> variables in veristat") to support applying presets to members of
>> the global structs or unions in veristat.
>> For example:
>> ```
>> ./veristat set_global_vars.bpf.o  -G "union1.struct3.var_u8_h = 0xBB"
>> ```
>>
>> Signed-off-by: Mykyta Yatsenko<yatsenko@meta.com>
>> ---
>>   .../selftests/bpf/prog_tests/test_veristat.c  |   5 +
>>   tools/testing/selftests/bpf/progs/prepare.c   |   1 -
>>   .../selftests/bpf/progs/set_global_vars.c     |  40 +++++++
>>   tools/testing/selftests/bpf/veristat.c        | 106 ++++++++++++++++--
>>   4 files changed, 144 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/test_veristat.c b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
>> index a95b42bf744a..47b56c258f3f 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/test_veristat.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
>> @@ -63,6 +63,9 @@ static void test_set_global_vars_succeeds(void)
>>              " -G \"var_eb = EB2\" "\
>>              " -G \"var_ec = EC2\" "\
>>              " -G \"var_b = 1\" "\
>> +           " -G \"struct1.struct2.u.var_u8 = 170\" "\
>> +           " -G \"union1.struct3.var_u8_l = 0xaa\" "\
>> +           " -G \"union1.struct3.var_u8_h = 0xaa\" "\
>>              "-vl2 > %s", fix->veristat, fix->tmpfile);
>>
>>          read(fix->fd, fix->output, fix->sz);
>> @@ -78,6 +81,8 @@ static void test_set_global_vars_succeeds(void)
>>          __CHECK_STR("_w=12 ", "var_eb = EB2");
>>          __CHECK_STR("_w=13 ", "var_ec = EC2");
>>          __CHECK_STR("_w=1 ", "var_b = 1");
>> +       __CHECK_STR("_w=170 ", "struct1.struct2.u.var_u8 = 170");
>> +       __CHECK_STR("_w=0xaaaa ", "union1.var_u16 = 0xaaaa");
>>
>>   out:
>>          teardown_fixture(fix);
>> diff --git a/tools/testing/selftests/bpf/progs/prepare.c b/tools/testing/selftests/bpf/progs/prepare.c
>> index 1f1dd547e4ee..cfc1f48e0d28 100644
>> --- a/tools/testing/selftests/bpf/progs/prepare.c
>> +++ b/tools/testing/selftests/bpf/progs/prepare.c
>> @@ -2,7 +2,6 @@
>>   /* Copyright (c) 2025 Meta */
>>   #include <vmlinux.h>
>>   #include <bpf/bpf_helpers.h>
>> -//#include <bpf/bpf_tracing.h>
>>
>>   char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/set_global_vars.c 
>> b/tools/testing/selftests/bpf/progs/set_global_vars.c index 
>> 9adb5ba4cd4d..187e9791e72e 100644 --- 
>> a/tools/testing/selftests/bpf/progs/set_global_vars.c +++ 
>> b/tools/testing/selftests/bpf/progs/set_global_vars.c @@ -24,6 +24,43 
>> @@ const volatile enum Enumu64 var_eb = EB1; const volatile enum 
>> Enums64 var_ec = EC1; const volatile bool var_b = false; +struct 
>> Struct { + int:16; + __u16 filler; + struct { + const __u16 filler2; 
>> + }; + struct Struct2 { + __u16 filler; + volatile struct { + const 
>> __u32 filler2; + union { + const volatile __u8 var_u8; + const 
>> volatile __s16 filler3; + } u; + }; + } struct2; +}; + +const 
>> volatile __u32 stru = 0; /* same prefix as below */ +const volatile 
>> struct Struct struct1 = {.struct2 = {.u = {.var_u8 = 1}}}; + +union 
>> Union { + __u16 var_u16; + struct Struct3 { + struct { + __u8 
>> var_u8_l; + }; + struct { + struct { + __u8 var_u8_h; + }; + }; + } 
>> struct3; +}; + +const volatile union Union union1 = {.var_u16 = -1}; 
>> + char arr[4] = {0}; SEC("socket")
>> @@ -43,5 +80,8 @@ int test_set_globals(void *ctx)
>>          a = var_eb;
>>          a = var_ec;
>>          a = var_b;
>> +       a = struct1.struct2.u.var_u8;
>> +       a = union1.var_u16;
>> +
>>          return a;
>>   }
>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
>> index a18972ffdeb6..727ef80a1e47 100644
>> --- a/tools/testing/selftests/bpf/veristat.c
>> +++ b/tools/testing/selftests/bpf/veristat.c
>> @@ -1486,7 +1486,89 @@ static bool is_preset_supported(const struct btf_type *t)
>>          return btf_is_int(t) || btf_is_enum(t) || btf_is_enum64(t);
>>   }
>>
>> -static int set_global_var(struct bpf_object *obj, struct btf *btf, const struct btf_type *t,
>> +struct btf_anon_stack {
>> +       const struct btf_type *type;
>> +       __u32 offset;
>> +};
>> +
> unused leftover
>
>> +const int btf_find_member(const struct btf *btf,
>> +                         const struct btf_type *parent_type,
>> +                         __u32 parent_offset,
>> +                         const char *member_name,
>> +                         int *member_tid,
>> +                         __u32 *member_offset)
>> +{
>> +       int i;
>> +
>> +       if (!btf_is_composite(parent_type))
>> +               return -EINVAL;
>> +
>> +       for (i = 0; i < btf_vlen(parent_type); ++i) {
>> +               const struct btf_member *member;
>> +               const struct btf_type *member_type;
>> +               int tid;
>> +
>> +               member = btf_members(parent_type) + i;
>> +               tid =  btf__resolve_type(btf, member->type);
>> +               if (tid < 0)
>> +                       return -EINVAL;
>> +
>> +               member_type = btf__type_by_id(btf, tid);
>> +               if (member->name_off) {
>> +                       const char *name = btf__name_by_offset(btf, member->name_off);
>> +
>> +                       if (strcmp(member_name, name) == 0) {
>> +                               *member_offset = parent_offset + member->offset;
>> +                               *member_tid = tid;
>> +                               return 0;
>> +                       }
>> +               } else if (btf_is_composite(member_type)) {
>> +                       int err;
>> +
>> +                       err = btf_find_member(btf, member_type, parent_offset + member->offset,
>> +                                             member_name, member_tid, member_offset);
>> +                       if (!err)
>> +                               return 0;
>> +               }
>> +       }
>> +
>> +       return -EINVAL;
>> +}
>> +
>> +static int adjust_var_secinfo(struct btf *btf, const struct btf_type *t,
>> +                             struct btf_var_secinfo *sinfo, const char *var)
>> +{
>> +       char expr[256], *saveptr;
>> +       const struct btf_type *base_type, *member_type;
>> +       int err, member_tid;
>> +       char *name;
>> +       __u32 member_offset = 0;
>> +
>> +       base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
>> +       strncpy(expr, var, 255);
>> +       expr[255] = '\0';
> strncpy() isn't a great API, and compilers have problems
> false-reporting non-zero-termination for them. I found that snprintf()
> works better
>
> snprintf(expr, sizeof(expr), "%s", var);
>
> ?
>
>> +       strtok_r(expr, ".", &saveptr);
>> +
>> +       while ((name = strtok_r(NULL, ".", &saveptr))) {
>> +               err = btf_find_member(btf, base_type, 0, name, &member_tid, &member_offset);
>> +               if (err) {
>> +                       fprintf(stderr, "Could not find member %s for variable %s\n", name, var);
>> +                       return err;
>> +               }
>> +               if (btf_kflag(base_type)) {
> hm... doesn't kflag on, say, STRUCT, just mean that there are *some*
> fields that are bitfields? If we don't reference those fields, it
> should be fine, no?
>
> So, instead, I think we should just check that
> btf_member_bitfield_size() for that field is zero, and if not --
> complain.
>
> Can you please also add a test case where we have a struct with
> bitfields, but we set only non-bitfield values and it all should work
> just fine. Thanks.

There is already a test with bitfield struct, this behavior does not 
repro, though
(btf_kflag is not set for structs with bitfields).
I think it's better to move this check out of the loop and only run on 
the final type
  we return in sinfo, either way it makes no sense to do it on structs, 
as you noticed.
I think I'll also move

+               sinfo->size = member_type->size;
+               sinfo->type = member_tid;
out, as we only care for the last type in the chain.

> pw-bot: cr
>
>> +                       fprintf(stderr, "Bitfield presets are not supported %s\n", name);
>> +                       return -EINVAL;
>> +               }
>> +               member_type = btf__type_by_id(btf, member_tid);
>> +               sinfo->offset += member_offset / 8;
>> +               sinfo->size = member_type->size;
>> +               sinfo->type = member_tid;
>> +               base_type = member_type;
>> +       }
>> +       return 0;
>> +}
>> +
>> +static int set_global_var(struct bpf_object *obj, struct btf *btf,
>>                            struct bpf_map *map, struct btf_var_secinfo *sinfo,
>>                            struct var_preset *preset)
>>   {
>> @@ -1495,9 +1577,9 @@ static int set_global_var(struct bpf_object *obj, struct btf *btf, const struct
>>          long long value = preset->ivalue;
>>          size_t size;
>>
>> -       base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
>> +       base_type = btf__type_by_id(btf, btf__resolve_type(btf, sinfo->type));
>>          if (!base_type) {
>> -               fprintf(stderr, "Failed to resolve type %d\n", t->type);
>> +               fprintf(stderr, "Failed to resolve type %d\n", sinfo->type);
>>                  return -EINVAL;
>>          }
>>          if (!is_preset_supported(base_type)) {
>> @@ -1530,7 +1612,7 @@ static int set_global_var(struct bpf_object *obj, struct btf *btf, const struct
>>                  if (value >= max_val || value < -max_val) {
>>                          fprintf(stderr,
>>                                  "Variable %s value %lld is out of range [%lld; %lld]\n",
>> -                               btf__name_by_offset(btf, t->name_off), value,
>> +                               btf__name_by_offset(btf, base_type->name_off), value,
>>                                  is_signed ? -max_val : 0, max_val - 1);
>>                          return -EINVAL;
>>                  }
>> @@ -1583,14 +1665,20 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
>>                  for (j = 0; j < n; ++j, ++sinfo) {
>>                          const struct btf_type *var_type = btf__type_by_id(btf, sinfo->type);
>>                          const char *var_name;
>> +                       int var_len;
>>
>>                          if (!btf_is_var(var_type))
>>                                  continue;
>>
>>                          var_name = btf__name_by_offset(btf, var_type->name_off);
>> +                       var_len = strlen(var_name);
>>
>>                          for (k = 0; k < npresets; ++k) {
>> -                               if (strcmp(var_name, presets[k].name) != 0)
>> +                               struct btf_var_secinfo tmp_sinfo;
>> +
>> +                               if (strncmp(var_name, presets[k].name, var_len) != 0 ||
>> +                                   (presets[k].name[var_len] != '\0' &&
>> +                                    presets[k].name[var_len] != '.'))
>>                                          continue;
>>
>>                                  if (presets[k].applied) {
>> @@ -1598,13 +1686,17 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
>>                                                  var_name);
>>                                          return -EINVAL;
>>                                  }
>> +                               memcpy(&tmp_sinfo, sinfo, sizeof(*sinfo));
> isn't this just:
>
> tmp_sinfo = *sinfo;
>
> why memcpy?
Right, makes sense.
>> +                               err = adjust_var_secinfo(btf, var_type,
>> +                                                        &tmp_sinfo, presets[k].name);
>> +                               if (err)
>> +                                       return err;
>>
>> -                               err = set_global_var(obj, btf, var_type, map, sinfo, presets + k);
>> +                               err = set_global_var(obj, btf, map, &tmp_sinfo, presets + k);
>>                                  if (err)
>>                                          return err;
>>
>>                                  presets[k].applied = true;
>> -                               break;
>>                          }
>>                  }
>>          }
>> --
>> 2.49.0
>>