On Mon, Mar 24, 2025 at 5:35 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
> .../selftests/bpf/prog_tests/test_veristat.c | 5 +
> tools/testing/selftests/bpf/progs/prepare.c | 1 -
> .../selftests/bpf/progs/set_global_vars.c | 39 +++++
> tools/testing/selftests/bpf/veristat.c | 141 +++++++++++++++++-
> 4 files changed, 178 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..0259d290d5f2 100644
> --- a/tools/testing/selftests/bpf/progs/set_global_vars.c
> +++ b/tools/testing/selftests/bpf/progs/set_global_vars.c
> @@ -24,6 +24,42 @@ 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 {
> + __u16 filler2;
> + };
> + struct Struct2 {
> + __u16 filler;
> + volatile struct {
> + __u32 filler2;
> + union {
> + const volatile __u8 var_u8;
> + const volatile __s16 filler3;
> + } u;
> + };
> + } struct2;
> +};
> +const volatile __u32 struc = 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 +79,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..4fb52767ea73 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -23,6 +23,7 @@
> #include <float.h>
> #include <math.h>
> #include <limits.h>
> +#include <linux/err.h>
this is kernel-internal header (not UAPI), so we won't have it in
Github; let's avoid adding this
>
> #ifndef ARRAY_SIZE
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> @@ -1486,7 +1487,124 @@ 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;
> +};
> +
> +const struct btf_member *btf_find_member(const struct btf *btf,
> + const struct btf_type *parent_type,
> + const char *member_name,
> + __u32 *anon_offset)
> +{
> + struct btf_anon_stack *anon_stack;
> + const struct btf_member *retval = NULL;
> + __u32 cur_offset = 0;
> + const char *name;
> + int top = 0, i;
> +
> + if (!btf_is_struct(parent_type) && !btf_is_union(parent_type))
we have btf_is_composite() which does exactly this
> + return ERR_PTR(-EINVAL);
> +
> + anon_stack = malloc(sizeof(*anon_stack));
> + if (!anon_stack)
> + return ERR_PTR(-ENOMEM);
> +
> + anon_stack[top].type = parent_type;
> + anon_stack[top++].offset = 0;
> +
> + do {
> + parent_type = anon_stack[--top].type;
> + cur_offset = anon_stack[top].offset;
> +
> + for (i = 0; i < btf_vlen(parent_type); ++i) {
> + const struct btf_member *member;
> + const struct btf_type *member_type;
> + int member_tid;
> +
> + member = btf_members(parent_type) + i;
> + member_tid = btf__resolve_type(btf, member->type);
> + if (member_tid < 0) {
> + retval = ERR_PTR(-EINVAL);
> + goto out;
> + }
> + member_type = btf__type_by_id(btf, member_tid);
> + if (member->name_off) {
> + name = btf__name_by_offset(btf, member->name_off);
> + if (name && strcmp(member_name, name) == 0) {
let's assume valid BTF and not do these unnecessary name != NULL
checks, we don't do it in many other places anyways
> + *anon_offset = cur_offset;
> + retval = member;
> + goto out;
> + }
> + } else if (btf_is_struct(member_type) || btf_is_union(member_type)) {
> + struct btf_anon_stack *tmp;
> + /* Anonymous union/struct: push to stack */
> + tmp = realloc(anon_stack, (top + 1) * sizeof(*anon_stack));
> + if (!tmp) {
> + retval = ERR_PTR(-ENOMEM);
> + goto out;
> + }
> + anon_stack = tmp;
> + anon_stack[top].type = member_type;
> + anon_stack[top++].offset = cur_offset + member->offset;
> + }
> + }
> + } while (top > 0);
> +out:
> + free(anon_stack);
> + return retval;
> +}
> +
why all this dynamic mem allocation for stack? this is user space
code, we have recursion, let's use the benefits of user space here
pw-bot: cr
> +static int adjust_var_secinfo_tok(char **name_tok, const struct btf *btf,
> + const struct btf_type *t, struct btf_var_secinfo *sinfo)
> +{
> + char *name = strtok_r(NULL, ".", name_tok);
> + const struct btf_type *member_type;
> + const struct btf_member *member;
> + int member_tid;
> + __u32 anon_offset = 0;
> +
> + if (!name)
> + return 0;
> +
> + member = btf_find_member(btf, t, name, &anon_offset);
> + if (IS_ERR(member)) {
why using ERR_PTR approach here if you don't really propagate error code?
I'd say instead of returning btf_member, I'd return containing BTF
type ID and member index within it (plus the offset you are returning)
This way you can take advantage of btf_member_bit_offset() and
btf_member_bitfield_size() helpers provided by libbpf in btf.h
and given you have multiple outputs, it's probably easier to return
int error code (or zero on success), and then everything else as out
parameters by pointer (instead of all the ERR_PTR business)
> + fprintf(stderr, "Could not find member %s\n", name);
> + return -EINVAL;
> + }
> +
> + member_tid = btf__resolve_type(btf, member->type);
> + member_type = btf__type_by_id(btf, member_tid);
> +
> + if (btf_kflag(t)) {
> + fprintf(stderr, "Bitfield presets are not supported %s\n", name);
> + return -EINVAL;
> + }
> + sinfo->offset += (member->offset + anon_offset) / 8;
> + sinfo->size = member_type->size;
> + sinfo->type = member_tid;
> +
> + return adjust_var_secinfo_tok(name_tok, btf, member_type, sinfo);
tbh, not a big fan of this recursion on tokenizer itself... Why can't
there be an outer loop to get tokens one at a time, and then recursive
btf_find_member() logic inside the loop to find offset and type
information?
Ultimately you need to find one offset and one type, corresponding to
the last member in the `a.b.c.d.e.f` chain, right? There is no
backtracking here, overall (the only backtracking will be hidden
inside btf_find_member due to anonymous fields), so the process is a
simple loop. There is no need to employ recursion, imo (unless I'm
missing some nuance).
Let's keep it simple(r).
> +}
> +
> +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;
> + int err;
> +
> + base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
> + strncpy(expr, var, 256);
> + strtok_r(expr, ".", &saveptr);
> + err = adjust_var_secinfo_tok(&saveptr, btf, base_type, sinfo);
> + if (err)
> + return err;
> +
> + 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 +1613,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 +1648,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;
> }
> @@ -1590,7 +1708,12 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
> var_name = btf__name_by_offset(btf, var_type->name_off);
>
> for (k = 0; k < npresets; ++k) {
> - if (strcmp(var_name, presets[k].name) != 0)
> + struct btf_var_secinfo tmp_sinfo;
> + int var_len = strlen(var_name);
do this once outside of the loop, why recalculating on each iteration?
> +
> + 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 +1721,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));
> + 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.48.1
>
