* [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat
@ 2025-03-31 21:12 Mykyta Yatsenko
2025-04-04 18:39 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Mykyta Yatsenko @ 2025-03-31 21:12 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87; +Cc: Mykyta Yatsenko
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;
+};
+
+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';
+ 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)) {
+ 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));
+ 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat
2025-03-31 21:12 [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat Mykyta Yatsenko
@ 2025-04-04 18:39 ` Andrii Nakryiko
2025-04-07 16:35 ` Mykyta Yatsenko
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2025-04-04 18:39 UTC (permalink / raw)
To: Mykyta Yatsenko
Cc: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87,
Mykyta Yatsenko
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.
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?
> + 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat
2025-04-04 18:39 ` Andrii Nakryiko
@ 2025-04-07 16:35 ` Mykyta Yatsenko
2025-04-07 17:26 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Mykyta Yatsenko @ 2025-04-07 16:35 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87,
Mykyta Yatsenko
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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat
2025-04-07 16:35 ` Mykyta Yatsenko
@ 2025-04-07 17:26 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2025-04-07 17:26 UTC (permalink / raw)
To: Mykyta Yatsenko
Cc: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87,
Mykyta Yatsenko
On Mon, Apr 7, 2025 at 9:35 AM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
> 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(-)
> >>
[...]
> >> + 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.
Discussed offline. Checking kflag will trigger false positives and
will reject fields that are "colocated" with bitfields within the same
struct/union. The proper way to check this would be
btf_member_bitfield_size(), as suggested above.
> 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;
> >> +}
> >> +
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat
@ 2025-03-24 12:34 Mykyta Yatsenko
2025-03-27 19:39 ` Eduard Zingerman
2025-03-28 17:03 ` Andrii Nakryiko
0 siblings, 2 replies; 7+ messages in thread
From: Mykyta Yatsenko @ 2025-03-24 12:34 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87; +Cc: Mykyta Yatsenko
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 | 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>
#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))
+ 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) {
+ *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;
+}
+
+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)) {
+ 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);
+}
+
+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);
+
+ 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat
2025-03-24 12:34 Mykyta Yatsenko
@ 2025-03-27 19:39 ` Eduard Zingerman
2025-03-28 17:03 ` Andrii Nakryiko
1 sibling, 0 replies; 7+ messages in thread
From: Eduard Zingerman @ 2025-03-27 19:39 UTC (permalink / raw)
To: Mykyta Yatsenko, bpf, ast, andrii, daniel, kafai, kernel-team
Cc: Mykyta Yatsenko
On Mon, 2025-03-24 at 12:34 +0000, Mykyta Yatsenko 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>
> ---
Thank you for addressing comments from my previous review.
Sorry for the delay, didn't look at patches while travelling.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
[...]
> +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)) {
> + 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;
Nit: I'd push this assignment down to `btf_find_member` and avoid
`&anon_offset` parameter. It would be easier to read this way,
as all offset manipulations would be in one place.
> +
> + return adjust_var_secinfo_tok(name_tok, btf, member_type, sinfo);
> +}
> +
> +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);
Nit: strncpy does not null terminate if strlen(src) == N.
> + 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)
> {
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat
2025-03-24 12:34 Mykyta Yatsenko
2025-03-27 19:39 ` Eduard Zingerman
@ 2025-03-28 17:03 ` Andrii Nakryiko
1 sibling, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2025-03-28 17:03 UTC (permalink / raw)
To: Mykyta Yatsenko
Cc: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87,
Mykyta Yatsenko
On Mon, Mar 24, 2025 at 5:35 AM 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 | 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-04-07 17:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-31 21:12 [PATCH bpf-next v2] selftests/bpf: support struct/union presets in veristat Mykyta Yatsenko
2025-04-04 18:39 ` Andrii Nakryiko
2025-04-07 16:35 ` Mykyta Yatsenko
2025-04-07 17:26 ` Andrii Nakryiko
-- strict thread matches above, loose matches on Subject: below --
2025-03-24 12:34 Mykyta Yatsenko
2025-03-27 19:39 ` Eduard Zingerman
2025-03-28 17:03 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox