Re: [PATCH bpf-next] selftests/bpf: implement setting global variables in veristat

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/02/2025 22:56, Eduard Zingerman wrote:
On Mon, 2025-02-03 at 16:40 +0000, Mykyta Yatsenko 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` 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  --set-global-vars "a = 0; b = 1; c = 2; d = 3;"
```

Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
---
This is super useful, thank you!
Maybe also add an ability to read variables list from a file?
(e.g. using -g @file-name syntax as in -f).

Worked fine for my small example, but failed to affect an object file
with multiple programs, see below.

Also, given that it is non-trivial to see if variable had indeed been set,
I think it would be useful to add a selftest that does
system("./veristat -l7 -v -g ...") and matches log output to check that
values are set correctly, e.g. I used the following simple test:

	const volatile u8  _u8  = 0;
	const volatile u16 _u16 = 0;
	const volatile u32 _u32 = 0;
	const volatile u64 _u64 = 0;
	const volatile s8  _s8  = 0;
	const volatile s16 _s16 = 0;
	const volatile s32 _s32 = 0;
	const volatile s64 _s64 = 0;

	SEC("socket")
	int test_globals(void *ctx)
	{
		volatile unsigned long cnt;
		cnt = _u8;
		cnt = _u16;
		cnt = _u32;
		cnt = _u64;
		cnt = _s8;
		cnt = _s16;
		cnt = _s32;
		cnt = _s64;
		return cnt;
	}

  tools/testing/selftests/bpf/veristat.c | 189 +++++++++++++++++++++++++
  1 file changed, 189 insertions(+)
[...]

@@ -1292,6 +1312,169 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
  	return 0;
  };
+static int parse_var_presets(char *expr, struct var_preset *presets, int capacity, int *size)
+{
+	char *state;
+	char *next;
+	int i = 0;
+
+	while ((next = strtok_r(i ? NULL : expr, ";", &state))) {
+		char *eq_ptr = strchr(next, '=');
+		char *name_ptr = next;
+		char *name_end = eq_ptr - 1;
+		char *val_ptr = eq_ptr + 1;
+
+		if (!eq_ptr)
+			continue;
Nit: error message here?

+
+		if (i >= capacity) {
+			fprintf(stderr, "Too many global variable presets\n");
+			return -EINVAL;
+		}
+		while (isspace(*name_ptr))
+			++name_ptr;
+		while (isspace(*name_end))
+			--name_end;
+
+		*(name_end + 1) = '\0';
+		presets[i].name = strdup(name_ptr);
+		errno = 0;
+		presets[i].value = strtoll(val_ptr, NULL, 10);
Nit: using base of 0 would allow to specify values either as decimals or in hex
      (using '0x' prefix).

+		if (errno == ERANGE) {
+			errno = 0;
+			presets[i].value = strtoull(val_ptr, NULL, 10);
+		}
+		if (errno) {
+			fprintf(stderr, "Could not parse integer value %s\n", val_ptr);
+			return -EINVAL;
+		}
+		++i;
+	}
+	*size = i;
+	return 0;
+}
+
+static bool is_signed_type(const struct btf_type *type)
+{
+	if (btf_is_int(type))
Nit: enums could be signed as well.

+		return btf_int_encoding(type) & BTF_INT_SIGNED;
+	return true;
+}
+
+static const struct btf_type *var_base_type(const struct btf *btf, const struct btf_type *type)
+{
+	switch (btf_kind(type)) {
+	case BTF_KIND_VAR:
+	case BTF_KIND_TYPE_TAG:
+	case BTF_KIND_CONST:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_RESTRICT:
+	case BTF_KIND_TYPEDEF:
+	case BTF_KIND_DECL_TAG:
+		return var_base_type(btf, btf__type_by_id(btf, type->type));
+	}
+	return type;
+}
+
+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 bpf_map *map, struct btf_var_secinfo *sinfo, long long new_val)
+{
+	const struct btf_type *base_type;
+	void *ptr;
+	size_t size;
+
+	base_type = var_base_type(btf, t);
+	if (!is_preset_supported(base_type)) {
+		fprintf(stderr, "Setting global variable for btf kind %d is not supported\n",
+			btf_kind(base_type));
+		return -EINVAL;
+	}
+
+	/* Check if value fits into the target variable size */
+	if  (sinfo->size < sizeof(new_val)) {
+		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 (new_val >= max_val || new_val < -max_val) {
+			fprintf(stderr,
+				"Variable %s value %lld is out of range [%lld; %lld]\n",
+				btf__name_by_offset(btf, t->name_off), new_val,
+				is_signed ? -max_val : 0, max_val - 1);
+			return -EINVAL;
+		}
+	}
+
+	ptr = (void *)bpf_map__initial_value(map, &size);
+	if (!ptr || (sinfo->offset + sinfo->size > size))
+		return -EINVAL;
+
+	memcpy(ptr + sinfo->offset, &new_val, sinfo->size);
will this work for big endian?

+	return 0;
+}
+
+static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, int npresets)
+{
+	struct btf_var_secinfo *sinfo;
+	const char *sec_name;
+	const struct btf_type *type;
+	struct bpf_map *map;
+	struct btf *btf;
+	int i, j, k, n, cnt, err, preset_cnt = 0;
+
+	if (npresets == 0)
+		return 0;
+
+	btf = bpf_object__btf(obj);
+	if (!btf)
+		return -EINVAL;
+
+	cnt = btf__type_cnt(btf);
+	for (i  = 0; i != cnt; ++i) {
+		type = btf__type_by_id(btf, i);
+
+		if (!btf_is_datasec(type))
+			continue;
+
+		sinfo = btf_var_secinfos(type);
+		sec_name = btf__name_by_offset(btf, type->name_off);
+		map = bpf_object__find_map_by_name(obj, sec_name);
+		if (!map)
+			continue;
+
+		n = btf_vlen(type);
+		for (j = 0; j < n; ++j, ++sinfo) {
+			const struct btf_type *var_type = btf__type_by_id(btf, sinfo->type);
+			const char *var_name = btf__name_by_offset(btf, var_type->name_off);
+
+			if (!btf_is_var(var_type))
+				continue;
+
+			for (k = 0; k < npresets; ++k) {
+				if (strcmp(var_name, presets[k].name) != 0)
+					continue;
+
+				err = set_global_var(obj, btf, var_type, map, sinfo,
+						     presets[k].value);
+				if (err)
+					return err;
+
+				preset_cnt++;
+				break;
+			}
+		}
+	}
+	if (preset_cnt != npresets)
+		fprintf(stderr, "Some global variable presets have not been applied\n");
Nit: it would be nice to print which ones were not set.

+
+	return 0;
+}
+
  static int process_obj(const char *filename)
  {
  	const char *base_filename = basename(strdupa(filename));
@@ -1338,6 +1521,12 @@ static int process_obj(const char *filename)
  		prog_cnt++;
  	}
+ err = set_global_vars(obj, env.presets, env.npresets);
+	if (err) {
+		fprintf(stderr, "Failed to set global variables\n");
+		goto cleanup;
+	}
+
  	if (prog_cnt == 1) {
  		prog = bpf_object__next_program(obj, NULL);
  		bpf_program__set_autoload(prog, true);
Same needs to happen for the loop below when prog_cnt != 1, e.g.:

@@ -1544,6 +1544,12 @@ static int process_obj(const char *filename)
                         goto cleanup;
                 }
+ err = set_global_vars(tobj, env.presets, env.npresets);
+               if (err) {
+                       fprintf(stderr, "Failed to set global variables\n");
+                       goto cleanup;
+               }
+
                 lprog = NULL;
                 bpf_object__for_each_program(tprog, tobj) {
                         const char *tprog_name = bpf_program__name(tprog);

Or, better yet, get rid of the `prog_cnt == 1` special case.

Thanks for the suggestions, make sense, going to address in v2.




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux