On Tue, Feb 11, 2025 at 7:00 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> On 11/02/2025 01:13, Andrii Nakryiko wrote:
> > On Mon, Feb 10, 2025 at 5:51 AM Mykyta Yatsenko
> > <mykyta.yatsenko5@xxxxxxxxx> wrote:
> >> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> >>
> >> To better verify some complex BPF programs we'd like to preset global
> >> variables.
> >> This patch introduces CLI argument `--set-global-vars` or `-G` to
> >> veristat, that allows presetting values to global variables defined
> >> in BPF program. For example:
> >>
> >> prog.c:
> >> ```
> >> enum Enum { ELEMENT1 = 0, ELEMENT2 = 5 };
> >> const volatile __s64 a = 5;
> >> const volatile __u8 b = 5;
> >> const volatile enum Enum c = ELEMENT2;
> >> const volatile bool d = false;
> >>
> >> char arr[4] = {0};
> >>
> >> SEC("tp_btf/sched_switch")
> >> int BPF_PROG(...)
> >> {
> >> bpf_printk("%c\n", arr[a]);
> >> bpf_printk("%c\n", arr[b]);
> >> bpf_printk("%c\n", arr[c]);
> >> bpf_printk("%c\n", arr[d]);
> >> return 0;
> >> }
> >> ```
> >> By default verification of the program fails:
> >> ```
> >> ./veristat prog.bpf.o
> >> ```
> >> By presetting global variables, we can make verification pass:
> >> ```
> >> ./veristat wq.bpf.o -G "a = 0" -G "b = 1" -G "c = 2" -G "d = 3"
> >> ```
> >>
> >> Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> >> ---
> >> tools/testing/selftests/bpf/veristat.c | 319 ++++++++++++++++++++++++-
> >> 1 file changed, 307 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> >> index 06af5029885b..b4521ebb6e6a 100644
> >> --- a/tools/testing/selftests/bpf/veristat.c
> >> +++ b/tools/testing/selftests/bpf/veristat.c
> >> @@ -154,6 +154,15 @@ struct filter {
> >> bool abs;
> >> };
> >>
[...]
> >> +static int enum_value_from_name(const struct btf *btf, const struct btf_type *t,
> >> + const char *evalue, long long *retval)
> >> +{
> >> + if (btf_is_enum(t)) {
> >> + struct btf_enum *e = btf_enum(t);
> >> + int i, n = btf_vlen(t);
> >> +
> >> + for (i = 0; i < n; ++i) {
> >> + const char *cur_name = btf__name_by_offset(btf, e[i].name_off);
> >> +
> >> + if (strcmp(cur_name, evalue) == 0) {
> >> + *retval = e[i].val;
> >> + return 0;
> >> + }
> >> + }
> >> + } else if (btf_is_enum64(t)) {
> >> + struct btf_enum64 *e = btf_enum64(t);
> >> + int i, n = btf_vlen(t);
> >> +
> >> + for (i = 0; i < n; ++i) {
> >> + struct btf_enum64 *cur = e + i;
> >> + const char *cur_name = btf__name_by_offset(btf, cur->name_off);
> > you have two conceptually identical loops, but in one you do `cur = e
> > + i` and in another you do `e[i]` access... why?
> The difference is that for e64 case we get value by the
> `btf_enum64_value` function, which accepts pointer to `btf_enum64`,
> I think it is a bit cleaner to have an explicit assignment `struct
> btf_enum64 *cur = e + i;`, instead of passing `&e[i]`
> into btf_enum64_value. Though, let's make both loops more consistent.
I'd just do `e++` inside for() and get rid of cur altogether.
> >> + __u64 value = btf_enum64_value(cur);
> >> +
> >> + if (strcmp(cur_name, evalue) == 0) {
> >> + *retval = value;
[...]
> >> + }
> >> +
> >> + /* Check if value fits into the target variable size */
> >> + if (sinfo->size < sizeof(preset->ivalue)) {
> >> + bool is_signed = is_signed_type(base_type);
> >> + __u32 unsigned_bits = sinfo->size * 8 - (is_signed ? 1 : 0);
> >> + long long max_val = 1ll << unsigned_bits;
> > what about u64? 1 << 64 ?
>
> This should not be executed for u64, check `if (sinfo->size <
> sizeof(preset->ivalue))` is there for that.
ah, missed that check, ok
>
> >
> >> +
> >> + if (preset->ivalue >= max_val || preset->ivalue < -max_val) {
> >> + fprintf(stderr,
> >> + "Variable %s value %lld is out of range [%lld; %lld]\n",
> >> + btf__name_by_offset(btf, t->name_off), preset->ivalue,
> >> + is_signed ? -max_val : 0, max_val - 1);
> >> + return -EINVAL;
> >> + }
> >> + }
> >> +
[...]
