On Fri, Feb 21, 2025 at 2:33 PM 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 | 290 ++++++++++++++++++++++++-
> 1 file changed, 289 insertions(+), 1 deletion(-)
>
Just a few minor things below, but overall looks great!
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 06af5029885b..1e1bc0dfa50a 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -154,6 +154,16 @@ struct filter {
> bool abs;
> };
>
> +struct var_preset {
> + char *name;
> + enum { INTEGRAL, NAME } type;
> + union {
> + long long ivalue;
> + char *svalue;
> + };
> + bool applied;
> +};
> +
> static struct env {
> char **filenames;
> int filename_cnt;
> @@ -195,6 +205,8 @@ static struct env {
> int progs_processed;
> int progs_skipped;
> int top_src_lines;
> + struct var_preset *presets;
> + int npresets;
> } env;
>
> static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va_list args)
> @@ -246,12 +258,15 @@ static const struct argp_option opts[] = {
> { "test-reg-invariants", 'r', NULL, 0,
> "Force BPF verifier failure on register invariant violation (BPF_F_TEST_REG_INVARIANTS program flag)" },
> { "top-src-lines", 'S', "N", 0, "Emit N most frequent source code lines" },
> + { "set-global-vars", 'G', "GLOBALS", 0, "Set global variables provided in the expression, for example \"var1 = 1\"" },
GLOBAL (singular)
> {},
> };
>
[...]
> +static int append_var_preset(struct var_preset **presets, int *cnt, const char *expr)
> +{
> + void *tmp;
> + struct var_preset *cur;
> + char var[256], val[256];
> +
> + tmp = realloc(*presets, (*cnt + 1) * sizeof(**presets));
> + if (!tmp)
> + return -ENOMEM;
> + *presets = tmp;
> + cur = &(*presets)[*cnt];
> + cur->applied = false;
> +
> + if (sscanf(expr, "%s = %s\n", var, val) != 2) {
> + fprintf(stderr, "Could not parse expression %s\n", expr);
> + return -EINVAL;
> + }
> +
> + if (isalpha(*val)) {
technically enum could be _WHATEVER, underscore is legal character
> + cur->svalue = strdup(val);
> + cur->type = NAME;
total nit, but NAME confused me a bit, as I assumed that the name of
the global variable. It's really just for enums, right? So ENUM or
ENUMERATOR would make sense and be more precise?
> + } else if (*val == '-' || isdigit(*val)) {
instead of doing this detection based on characters, why not try to
parse a number (and I'd use sscanf("%lli") which will handle 1234 and
0x1234 transparently)? And you can use %n to check that all characters
were parsed (i.e., you have exact match)
it's probably fine for starters, but it kind of sucks that 0x1234
isn't supported
> + long long value;
> +
> + errno = 0;
> + value = strtoll(val, NULL, 0);
> + if (errno == ERANGE) {
> + errno = 0;
> + value = strtoull(val, NULL, 0);
> + }
> + cur->ivalue = value;
> + cur->type = INTEGRAL;
> + if (errno) {
> + fprintf(stderr, "Could not parse integer value %s\n", val);
> + return -EINVAL;
> + }
> + } else {
> + fprintf(stderr, "Could not parse value %s\n", val);
> + return -EINVAL;
> + }
> + cur->name = strdup(var);
check for NULL and error out? (no need for logging message, it's rare
to get ENOMEM)
pw-bot: cr
> + (*cnt)++;
> + return 0;
> +}
> +
[...]
> + /* Check if value fits into the target variable size */
> + if (sinfo->size < sizeof(value)) {
> + 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;
> +
> + 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,
> + is_signed ? -max_val : 0, max_val - 1);
> + return -EINVAL;
> + }
> + }
> +
> + ptr = (void *)bpf_map__initial_value(map, &size);
it already returns void *, no need to cast
> + if (!ptr || (sinfo->offset + sinfo->size > size))
nit: unnecessary ()
> + return -EINVAL;
> +
> + if (__BYTE_ORDER == __LITTLE_ENDIAN) {
> + memcpy(ptr + sinfo->offset, &value, sinfo->size);
> + } else if (__BYTE_ORDER == __BIG_ENDIAN) {
either this if condition is superficial, or we'd need to add another
else with error. let's just
} else { /* __BYTE_ORDER == __BIG_ENDIAN */
memcpy()
}
?
> + __u8 src_offset = sizeof(value) - sinfo->size;
> +
> + memcpy(ptr + sinfo->offset, (void *)&value + src_offset, sinfo->size);
> + }
> + return 0;
> +}
> +
[...]
> static int process_obj(const char *filename)
> {
> const char *base_filename = basename(strdupa(filename));
> @@ -1341,6 +1612,11 @@ static int process_obj(const char *filename)
> if (prog_cnt == 1) {
> prog = bpf_object__next_program(obj, NULL);
> bpf_program__set_autoload(prog, true);
> + err = set_global_vars(obj, env.presets, env.npresets);
> + if (err) {
> + fprintf(stderr, "Failed to set global variables\n");
emit error code to help with debugging in the future? same below
> + goto cleanup;
> + }
> process_prog(filename, obj, prog);
> goto cleanup;
> }
[...]
