| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 29 Jun 2020 18:43:51 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <jolsa@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Martin KaFai Lau <kafai@...com>,
David Miller <davem@...hat.com>,
John Fastabend <john.fastabend@...il.com>,
Wenbo Zhang <ethercflow@...il.com>,
KP Singh <kpsingh@...omium.org>,
Andrii Nakryiko <andriin@...com>,
Brendan Gregg <bgregg@...flix.com>,
Florent Revest <revest@...omium.org>,
Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v4 bpf-next 14/14] selftests/bpf: Add test for resolve_btfids
On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@...nel.org> wrote:
>
> Adding test to resolve_btfids tool, that:
> - creates binary with BTF IDs list and set
> - process the binary with resolve_btfids tool
> - verifies that correct BTF ID values are in place
>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
> tools/testing/selftests/bpf/Makefile | 20 +-
> .../selftests/bpf/test_resolve_btfids.c | 201 ++++++++++++++++++
> 2 files changed, 220 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/test_resolve_btfids.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 22aaec74ea0a..547322a5feff 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -37,7 +37,8 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> test_cgroup_storage \
> test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
> test_progs-no_alu32 \
> - test_current_pid_tgid_new_ns
> + test_current_pid_tgid_new_ns \
> + test_resolve_btfids
>
> # Also test bpf-gcc, if present
> ifneq ($(BPF_GCC),)
> @@ -427,6 +428,23 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
> $(call msg,BINARY,,$@)
> $(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
>
> +# test_resolve_btfids
> +#
useless comment, please drop
> +$(SCRATCH_DIR)/resolve_btfids: $(BPFOBJ) FORCE
> + $(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/resolve_btfids \
> + OUTPUT=$(SCRATCH_DIR)/ BPFOBJ=$(BPFOBJ)
Why do you need FORCE here? To force building this tool every single
time, even if nothing changed? See what we did for bpftool rebuilds.
It's not perfect, but works fine in practice.
> +
> +$(OUTPUT)/test_resolve_btfids.o: test_resolve_btfids.c
> + $(call msg,CC,,$@)
> + $(CC) $(CFLAGS) -I$(TOOLSINCDIR) -D"BUILD_STR(s)=#s" -DVMLINUX_BTF="BUILD_STR($(VMLINUX_BTF))" -c -o $@ $<
> +
> +.PHONY: FORCE
> +
> +$(OUTPUT)/test_resolve_btfids: $(OUTPUT)/test_resolve_btfids.o $(SCRATCH_DIR)/resolve_btfids
> + $(call msg,BINARY,,$@)
> + $(CC) -o $@ $< $(BPFOBJ) -lelf -lz && \
> + $(SCRATCH_DIR)/resolve_btfids --btf $(VMLINUX_BTF) $@
> +
Wouldn't it be better to make this just one of the tests of test_progs
and let resolve_btfids process test_progs completely? That should
still work, plus statically resolved BTF IDs against kernel would be
available for other tests immediately. And you will have all the
infrastructure of test_progs available. And this will be tested very
regularly. Win-win-win-win?
> EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) \
> prog_tests/tests.h map_tests/tests.h verifier/tests.h \
> feature \
> diff --git a/tools/testing/selftests/bpf/test_resolve_btfids.c b/tools/testing/selftests/bpf/test_resolve_btfids.c
> new file mode 100644
> index 000000000000..48aeda2ed881
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_resolve_btfids.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <string.h>
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <linux/err.h>
> +#include <stdlib.h>
> +#include <bpf/btf.h>
> +#include <bpf/libbpf.h>
> +#include <linux/btf.h>
> +#include <linux/kernel.h>
> +#include <linux/btf_ids.h>
> +
> +#define __CHECK(condition, format...) ({ \
> + int __ret = !!(condition); \
> + if (__ret) { \
> + fprintf(stderr, "%s:%d:FAIL ", __func__, __LINE__); \
> + fprintf(stderr, format); \
> + } \
> + __ret; \
> +})
> +
> +#define CHECK(condition, format...) \
> + do { \
> + if (__CHECK(condition, format)) \
> + return -1; \
> + } while (0)
it's better to make CHECK return value, makes its use more flexible
> +
> +static struct btf *btf__parse_raw(const char *file)
How about adding this as a libbpf API? It's not the first time I see
this being re-implemented. While simple, libbpf already implements
this internally, so there should be no need to require users do this
all the time. Follow up patch is ok, no need to block on this.
> +{
> + struct btf *btf;
> + struct stat st;
> + __u8 *buf;
> + FILE *f;
> +
> + if (stat(file, &st))
> + return NULL;
> +
> + f = fopen(file, "rb");
> + if (!f)
> + return NULL;
> +
> + buf = malloc(st.st_size);
> + if (!buf) {
> + btf = ERR_PTR(-ENOMEM);
> + goto exit_close;
> + }
> +
> + if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
> + btf = ERR_PTR(-EINVAL);
> + goto exit_free;
> + }
> +
> + btf = btf__new(buf, st.st_size);
> +
> +exit_free:
> + free(buf);
> +exit_close:
> + fclose(f);
> + return btf;
> +}
> +
[...]
> +
> +static int
> +__resolve_symbol(struct btf *btf, int type_id)
> +{
> + const struct btf_type *type;
> + const char *str;
> + unsigned int i;
> +
> + type = btf__type_by_id(btf, type_id);
> + CHECK(!type, "Failed to get type for ID %d\n", type_id);
return otherwise you'll get crash on few lines below; it's unpleasant
to debug crashes in VM in Travis CI
> +
> + for (i = 0; i < ARRAY_SIZE(test_symbols); i++) {
> + if (test_symbols[i].id != -1)
> + continue;
> +
> + if (BTF_INFO_KIND(type->info) != test_symbols[i].type)
> + continue;
> +
> + str = btf__name_by_offset(btf, type->name_off);
> + if (!str) {
CHECK?
> + fprintf(stderr, "failed to get name for BTF ID %d\n",
> + type_id);
> + continue;
> + }
> +
> + if (!strcmp(str, test_symbols[i].name))
> + test_symbols[i].id = type_id;
> + }
> +
> + return 0;
> +}
> +
> +static int resolve_symbols(void)
> +{
> + const char *path = VMLINUX_BTF;
> + struct btf *btf;
> + int type_id;
> + __u32 nr;
> + int err;
> +
> + btf = btf_open(path);
> + CHECK(libbpf_get_error(btf), "Failed to load BTF from %s\n", path);
> +
exit, crash othewise
> + nr = btf__get_nr_types(btf);
> +
> + for (type_id = 0; type_id < nr; type_id++) {
type_id = 1; type_id <= nr
> + err = __resolve_symbol(btf, type_id);
> + if (__CHECK(err, "Failed to resolve symbols\n"))
> + break;
> + }
> +
> + btf__free(btf);
> + return 0;
> +}
> +
[...]
Powered by blists - more mailing lists