* [PATCH bpf-next] selftests/bpf: support struct/union presets in veristat
@ 2025-03-20 22:45 Mykyta Yatsenko
2025-03-22 1:09 ` Eduard Zingerman
0 siblings, 1 reply; 3+ messages in thread
From: Mykyta Yatsenko @ 2025-03-20 22:45 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 | 38 +++++
tools/testing/selftests/bpf/veristat.c | 148 +++++++++++++++++-
4 files changed, 184 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..f2b354decbb8 100644
--- a/tools/testing/selftests/bpf/progs/set_global_vars.c
+++ b/tools/testing/selftests/bpf/progs/set_global_vars.c
@@ -24,6 +24,41 @@ const volatile enum Enumu64 var_eb = EB1;
const volatile enum Enums64 var_ec = EC1;
const volatile bool var_b = false;
+struct Struct {
+ __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 +78,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..babc97b799a2 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,131 @@ 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 *t;
+ __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].t = parent_type;
+ anon_stack[top++].offset = 0;
+
+ do {
+ parent_type = anon_stack[--top].t;
+ cur_offset = anon_stack[top].offset;
+
+ for (i = 0; i < btf_vlen(parent_type); ++i) {
+ const struct btf_member *member;
+ const struct btf_type *t;
+ int tid;
+
+ member = btf_members(parent_type) + i;
+ tid = btf__resolve_type(btf, member->type);
+ if (tid < 0) {
+ retval = ERR_PTR(-EINVAL);
+ goto out;
+ }
+ t = btf__type_by_id(btf, tid);
+ if (member->name_off) {
+ name = btf__name_by_offset(btf, member->name_off);
+ if (name && strcmp(member_name, name) == 0) {
+ if (anon_offset)
+ *anon_offset = cur_offset;
+ retval = member;
+ goto out;
+ }
+ } else if (t) {
+ struct btf_anon_stack *tmp;
+
+ tmp = realloc(anon_stack, (top + 1) * sizeof(*anon_stack));
+ if (!tmp) {
+ retval = ERR_PTR(-ENOMEM);
+ goto out;
+ }
+ anon_stack = tmp;
+ /* Anonymous union/struct: push to stack */
+ anon_stack[top].t = t;
+ 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;
+
+ if (!btf_is_union(t) && !btf_is_struct(t))
+ return -EINVAL;
+
+ member = btf_find_member(btf, t, name, &anon_offset);
+ if (IS_ERR(member))
+ return -EINVAL;
+
+ member_tid = btf__resolve_type(btf, member->type);
+ member_type = btf__type_by_id(btf, member_tid);
+
+ if (btf_kflag(t)) {
+ sinfo->offset += (BTF_MEMBER_BIT_OFFSET(member->offset) + anon_offset) / 8;
+ sinfo->size = BTF_MEMBER_BITFIELD_SIZE(member->offset) / 8;
+ } else {
+ 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));
+ if (!btf_is_union(base_type) && !btf_is_struct(base_type))
+ return 0;
+
+ strcpy(expr, var);
+ 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 +1620,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 +1655,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 +1715,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 +1728,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] 3+ messages in thread* Re: [PATCH bpf-next] selftests/bpf: support struct/union presets in veristat
2025-03-20 22:45 [PATCH bpf-next] selftests/bpf: support struct/union presets in veristat Mykyta Yatsenko
@ 2025-03-22 1:09 ` Eduard Zingerman
2025-03-24 12:01 ` Mykyta Yatsenko
0 siblings, 1 reply; 3+ messages in thread
From: Eduard Zingerman @ 2025-03-22 1:09 UTC (permalink / raw)
To: Mykyta Yatsenko, bpf, ast, andrii, daniel, kafai, kernel-team
Cc: Mykyta Yatsenko
On Thu, 2025-03-20 at 22:45 +0000, Mykyta Yatsenko wrote:
[...]
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index a18972ffdeb6..babc97b799a2 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,131 @@ 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 *t;
> + __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].t = parent_type;
> + anon_stack[top++].offset = 0;
> +
> + do {
> + parent_type = anon_stack[--top].t;
> + cur_offset = anon_stack[top].offset;
> +
> + for (i = 0; i < btf_vlen(parent_type); ++i) {
> + const struct btf_member *member;
> + const struct btf_type *t;
> + int tid;
> +
> + member = btf_members(parent_type) + i;
> + tid = btf__resolve_type(btf, member->type);
Nit: these are called member_tid and member_type in the function below.
> + if (tid < 0) {
> + retval = ERR_PTR(-EINVAL);
> + goto out;
> + }
> + t = btf__type_by_id(btf, tid);
> + if (member->name_off) {
> + name = btf__name_by_offset(btf, member->name_off);
> + if (name && strcmp(member_name, name) == 0) {
> + if (anon_offset)
Nit: anon_offset is always non-null.
> + *anon_offset = cur_offset;
> + retval = member;
> + goto out;
> + }
> + } else if (t) {
Nit: result of `btf__resolve_type()` is not checked against NULL in
most places in veristat.c. When bpf object file is opened by
libbpf the BTF is setup by function btf.c:btf_new(), which
does some sanity including checks for ids of member types.
See btf.c:btf_sanity_check().
> + struct btf_anon_stack *tmp;
> +
> + tmp = realloc(anon_stack, (top + 1) * sizeof(*anon_stack));
> + if (!tmp) {
> + retval = ERR_PTR(-ENOMEM);
> + goto out;
> + }
> + anon_stack = tmp;
> + /* Anonymous union/struct: push to stack */
> + anon_stack[top].t = t;
> + anon_stack[top++].offset = cur_offset + member->offset;
I think it is necessary to check that `t` is struct or union,
otherwise something like 'struct foo { int :64; int bar; }'
will cause trouble.
> + }
> + }
> + } 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;
> +
> + if (!btf_is_union(t) && !btf_is_struct(t))
> + return -EINVAL;
> +
> + member = btf_find_member(btf, t, name, &anon_offset);
> + if (IS_ERR(member))
> + return -EINVAL;
> +
> + member_tid = btf__resolve_type(btf, member->type);
> + member_type = btf__type_by_id(btf, member_tid);
> +
> + if (btf_kflag(t)) {
> + sinfo->offset += (BTF_MEMBER_BIT_OFFSET(member->offset) + anon_offset) / 8;
> + sinfo->size = BTF_MEMBER_BITFIELD_SIZE(member->offset) / 8;
Bitfields are not handled by `set_global_var`, as ->size is in bytes.
Maybe just error out here saying that setting bitfields is not supported?
Alternatively, there is a utility function btf_member_bit_offset(),
maybe declare a similar btf_member_bit_size() and remove the
btf_kflag(t) condition here? Just to make it a bit easier to understand.
> + } else {
> + 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));
> + if (!btf_is_union(base_type) && !btf_is_struct(base_type))
> + return 0;
What would happen if preset "foo.bar" would be specified for variable
"foo" being e.g. of type "int"? It seems the ".bar" part would be just
ignored.
> +
> + strcpy(expr, var);
Nit: strncpy ?
> + 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 +1620,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 +1655,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 +1715,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] != '.'))
var_name comes from BTF and presets[k].name comes from command line, right?
Meaning that there might be a case when strlen(presets[k].name) < strlen(var_name)
and access presets[k].name[var_len] would be out of bounds. Wdyt?
> continue;
>
> if (presets[k].applied) {
> @@ -1598,13 +1728,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;
This is removed to handle cases with presets "foo.bar" and "foo.buz", right?
Maybe extend the test case a bit?
> }
> }
> }
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH bpf-next] selftests/bpf: support struct/union presets in veristat
2025-03-22 1:09 ` Eduard Zingerman
@ 2025-03-24 12:01 ` Mykyta Yatsenko
0 siblings, 0 replies; 3+ messages in thread
From: Mykyta Yatsenko @ 2025-03-24 12:01 UTC (permalink / raw)
To: Eduard Zingerman, bpf, ast, andrii, daniel, kafai, kernel-team
Cc: Mykyta Yatsenko
On 22/03/2025 01:09, Eduard Zingerman wrote:
> On Thu, 2025-03-20 at 22:45 +0000, Mykyta Yatsenko wrote:
>
> [...]
>
>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
>> index a18972ffdeb6..babc97b799a2 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,131 @@ 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 *t;
>> + __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].t = parent_type;
>> + anon_stack[top++].offset = 0;
>> +
>> + do {
>> + parent_type = anon_stack[--top].t;
>> + cur_offset = anon_stack[top].offset;
>> +
>> + for (i = 0; i < btf_vlen(parent_type); ++i) {
>> + const struct btf_member *member;
>> + const struct btf_type *t;
>> + int tid;
>> +
>> + member = btf_members(parent_type) + i;
>> + tid = btf__resolve_type(btf, member->type);
> Nit: these are called member_tid and member_type in the function below.
>
>> + if (tid < 0) {
>> + retval = ERR_PTR(-EINVAL);
>> + goto out;
>> + }
>> + t = btf__type_by_id(btf, tid);
>> + if (member->name_off) {
>> + name = btf__name_by_offset(btf, member->name_off);
>> + if (name && strcmp(member_name, name) == 0) {
>> + if (anon_offset)
> Nit: anon_offset is always non-null.
>
Addressing nits in v2.
>> + *anon_offset = cur_offset;
>> + retval = member;
>> + goto out;
>> + }
>> + } else if (t) {
> Nit: result of `btf__resolve_type()` is not checked against NULL in
> most places in veristat.c. When bpf object file is opened by
> libbpf the BTF is setup by function btf.c:btf_new(), which
> does some sanity including checks for ids of member types.
> See btf.c:btf_sanity_check().
>
>> + struct btf_anon_stack *tmp;
>> +
>> + tmp = realloc(anon_stack, (top + 1) * sizeof(*anon_stack));
>> + if (!tmp) {
>> + retval = ERR_PTR(-ENOMEM);
>> + goto out;
>> + }
>> + anon_stack = tmp;
>> + /* Anonymous union/struct: push to stack */
>> + anon_stack[top].t = t;
>> + anon_stack[top++].offset = cur_offset + member->offset;
> I think it is necessary to check that `t` is struct or union,
> otherwise something like 'struct foo { int :64; int bar; }'
> will cause trouble.
This is a good idea, I agree.
>> + }
>> + }
>> + } 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;
>> +
>> + if (!btf_is_union(t) && !btf_is_struct(t))
>> + return -EINVAL;
>> +
>> + member = btf_find_member(btf, t, name, &anon_offset);
>> + if (IS_ERR(member))
>> + return -EINVAL;
>> +
>> + member_tid = btf__resolve_type(btf, member->type);
>> + member_type = btf__type_by_id(btf, member_tid);
>> +
>> + if (btf_kflag(t)) {
>> + sinfo->offset += (BTF_MEMBER_BIT_OFFSET(member->offset) + anon_offset) / 8;
>> + sinfo->size = BTF_MEMBER_BITFIELD_SIZE(member->offset) / 8;
> Bitfields are not handled by `set_global_var`, as ->size is in bytes.
> Maybe just error out here saying that setting bitfields is not supported?
> Alternatively, there is a utility function btf_member_bit_offset(),
> maybe declare a similar btf_member_bit_size() and remove the
> btf_kflag(t) condition here? Just to make it a bit easier to understand.
Right, we don't support setting bitfields, It makes sense to error out.
>> + } else {
>> + 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));
>> + if (!btf_is_union(base_type) && !btf_is_struct(base_type))
>> + return 0;
> What would happen if preset "foo.bar" would be specified for variable
> "foo" being e.g. of type "int"? It seems the ".bar" part would be just
> ignored.
>
good point, I'll change this for v2.
>> +
>> + strcpy(expr, var);
> Nit: strncpy ?
yes
>
>> + 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 +1620,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 +1655,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 +1715,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] != '.'))
> var_name comes from BTF and presets[k].name comes from command line, right?
> Meaning that there might be a case when strlen(presets[k].name) < strlen(var_name)
> and access presets[k].name[var_len] would be out of bounds. Wdyt?
checks
```
+(presets[k].name[var_len] != '\0' && presets[k].name[var_len] != '.')
```
Are executed only if
```
strncmp(var_name, presets[k].name, var_len) == 0
```
returns 0 (because of ||), which means there are at least var_len
non-terminal symbols in
presets[k].name.
>> continue;
>>
>> if (presets[k].applied) {
>> @@ -1598,13 +1728,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;
> This is removed to handle cases with presets "foo.bar" and "foo.buz", right?
> Maybe extend the test case a bit?
It has been extended, check this out:
+ " -G \"union1.struct3.var_u8_l = 0xaa\" "\
+ " -G \"union1.struct3.var_u8_h = 0xaa\" "\
>> }
>> }
>> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-24 12:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-20 22:45 [PATCH bpf-next] selftests/bpf: support struct/union presets in veristat Mykyta Yatsenko
2025-03-22 1:09 ` Eduard Zingerman
2025-03-24 12:01 ` Mykyta Yatsenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox