diff mbox series

[v2,bpf-next,10/12] selftests/bpf: merge test_stub.c into testing_helpers.c

Message ID 20211103220845.2676888-11-andrii@kernel.org (mailing list archive)
State Accepted
Delegated to: BPF
Headers show
Series libbpf: add unified bpf_prog_load() low-level API | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 8 maintainers not CCed: linux-kselftest@vger.kernel.org songliubraving@fb.com shuah@kernel.org yhs@fb.com kafai@fb.com netdev@vger.kernel.org kpsingh@kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch fail CHECK: Alignment should match open parenthesis ERROR: do not initialise globals to 0 WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next success VM_Test

Commit Message

Andrii Nakryiko Nov. 3, 2021, 10:08 p.m. UTC 
Move testing prog and object load wrappers (bpf_prog_test_load and
bpf_test_load_program) into testing_helpers.{c,h} and get rid of
otherwise useless test_stub.c. Make testing_helpers.c available to
non-test_progs binaries as well.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/Makefile          | 33 +++++------
 tools/testing/selftests/bpf/test_stub.c       | 44 ---------------
 tools/testing/selftests/bpf/testing_helpers.c | 55 +++++++++++++++++++
 tools/testing/selftests/bpf/testing_helpers.h |  6 ++
 4 files changed, 78 insertions(+), 60 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/test_stub.c

Comments

Dave Marchevsky Nov. 5, 2021, 7:51 a.m. UTC | #1 
On 11/3/21 6:08 PM, Andrii Nakryiko wrote:   
> Move testing prog and object load wrappers (bpf_prog_test_load and
> bpf_test_load_program) into testing_helpers.{c,h} and get rid of
> otherwise useless test_stub.c. Make testing_helpers.c available to
> non-test_progs binaries as well.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

>  tools/testing/selftests/bpf/Makefile          | 33 +++++------
>  tools/testing/selftests/bpf/test_stub.c       | 44 ---------------
>  tools/testing/selftests/bpf/testing_helpers.c | 55 +++++++++++++++++++
>  tools/testing/selftests/bpf/testing_helpers.h |  6 ++
>  4 files changed, 78 insertions(+), 60 deletions(-)
>  delete mode 100644 tools/testing/selftests/bpf/test_stub.c

[...]
Alexei Starovoitov Nov. 10, 2021, 1:48 a.m. UTC | #2 
On Wed, Nov 3, 2021 at 3:09 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Move testing prog and object load wrappers (bpf_prog_test_load and
> bpf_test_load_program) into testing_helpers.{c,h} and get rid of
> otherwise useless test_stub.c. Make testing_helpers.c available to
> non-test_progs binaries as well.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
...
> -int extra_prog_load_log_flags = 0;
> -
> -int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
> -                      struct bpf_object **pobj, int *prog_fd)
> -{
> -       struct bpf_prog_load_attr attr;
> -
> -       memset(&attr, 0, sizeof(struct bpf_prog_load_attr));
> -       attr.file = file;
> -       attr.prog_type = type;
> -       attr.expected_attach_type = 0;
> -       attr.prog_flags = BPF_F_TEST_RND_HI32;
> -       attr.log_level = extra_prog_load_log_flags;
> -
> -       return bpf_prog_load_xattr(&attr, pobj, prog_fd);
> -}

> +int extra_prog_load_log_flags = 0;
> +
> +int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
> +                      struct bpf_object **pobj, int *prog_fd)
> +{
> +       struct bpf_object *obj;
> +       struct bpf_program *prog;
> +       int err;
> +
> +       obj = bpf_object__open(file);
> +       if (!obj)
> +               return -errno;
> +
> +       prog = bpf_object__next_program(obj, NULL);
> +       if (!prog) {
> +               err = -ENOENT;
> +               goto err_out;
> +       }
> +
> +       if (type != BPF_PROG_TYPE_UNSPEC)
> +               bpf_program__set_type(prog, type);
> +
> +       err = bpf_object__load(obj);
> +       if (err)
> +               goto err_out;
> +
> +       *pobj = obj;
> +       *prog_fd = bpf_program__fd(prog);
> +
> +       return 0;
> +err_out:
> +       bpf_object__close(obj);
> +       return err;
> +}

Andrii,
That part of the commit broke verbose output in the test.
-v and -vv no longer work for tests that use bpf_prog_test_load()
because extra_prog_load_log_flags are ignored.

Please fix.
Andrii Nakryiko Nov. 10, 2021, 2:38 a.m. UTC | #3 
On Tue, Nov 9, 2021 at 5:48 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Nov 3, 2021 at 3:09 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > Move testing prog and object load wrappers (bpf_prog_test_load and
> > bpf_test_load_program) into testing_helpers.{c,h} and get rid of
> > otherwise useless test_stub.c. Make testing_helpers.c available to
> > non-test_progs binaries as well.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ...
> > -int extra_prog_load_log_flags = 0;
> > -
> > -int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
> > -                      struct bpf_object **pobj, int *prog_fd)
> > -{
> > -       struct bpf_prog_load_attr attr;
> > -
> > -       memset(&attr, 0, sizeof(struct bpf_prog_load_attr));
> > -       attr.file = file;
> > -       attr.prog_type = type;
> > -       attr.expected_attach_type = 0;
> > -       attr.prog_flags = BPF_F_TEST_RND_HI32;
> > -       attr.log_level = extra_prog_load_log_flags;
> > -
> > -       return bpf_prog_load_xattr(&attr, pobj, prog_fd);
> > -}
>
> > +int extra_prog_load_log_flags = 0;
> > +
> > +int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
> > +                      struct bpf_object **pobj, int *prog_fd)
> > +{
> > +       struct bpf_object *obj;
> > +       struct bpf_program *prog;
> > +       int err;
> > +
> > +       obj = bpf_object__open(file);
> > +       if (!obj)
> > +               return -errno;
> > +
> > +       prog = bpf_object__next_program(obj, NULL);
> > +       if (!prog) {
> > +               err = -ENOENT;
> > +               goto err_out;
> > +       }
> > +
> > +       if (type != BPF_PROG_TYPE_UNSPEC)
> > +               bpf_program__set_type(prog, type);
> > +
> > +       err = bpf_object__load(obj);
> > +       if (err)
> > +               goto err_out;
> > +
> > +       *pobj = obj;
> > +       *prog_fd = bpf_program__fd(prog);
> > +
> > +       return 0;
> > +err_out:
> > +       bpf_object__close(obj);
> > +       return err;
> > +}
>
> Andrii,
> That part of the commit broke verbose output in the test.
> -v and -vv no longer work for tests that use bpf_prog_test_load()
> because extra_prog_load_log_flags are ignored.
>
> Please fix.

Ah, right, sorry, I should have used bpf_object__load_xattr() and
passed those extra_prog_load_log_flags to it. I'll fix it.

How important are BPF_F_TEST_RND_HI32 flags, btw? We need a
bpf_program__set_extra_flags() to be able to set them, I'll add that
as well. "extra" because they will be |'d with whatever the flags are
determined by SEC_DEF().
Alexei Starovoitov Nov. 10, 2021, 3:31 a.m. UTC | #4 
On Tue, Nov 9, 2021 at 6:38 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 5:48 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Nov 3, 2021 at 3:09 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> > >
> > > Move testing prog and object load wrappers (bpf_prog_test_load and
> > > bpf_test_load_program) into testing_helpers.{c,h} and get rid of
> > > otherwise useless test_stub.c. Make testing_helpers.c available to
> > > non-test_progs binaries as well.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ...
> > > -int extra_prog_load_log_flags = 0;
> > > -
> > > -int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
> > > -                      struct bpf_object **pobj, int *prog_fd)
> > > -{
> > > -       struct bpf_prog_load_attr attr;
> > > -
> > > -       memset(&attr, 0, sizeof(struct bpf_prog_load_attr));
> > > -       attr.file = file;
> > > -       attr.prog_type = type;
> > > -       attr.expected_attach_type = 0;
> > > -       attr.prog_flags = BPF_F_TEST_RND_HI32;
> > > -       attr.log_level = extra_prog_load_log_flags;
> > > -
> > > -       return bpf_prog_load_xattr(&attr, pobj, prog_fd);
> > > -}
> >
> > > +int extra_prog_load_log_flags = 0;
> > > +
> > > +int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
> > > +                      struct bpf_object **pobj, int *prog_fd)
> > > +{
> > > +       struct bpf_object *obj;
> > > +       struct bpf_program *prog;
> > > +       int err;
> > > +
> > > +       obj = bpf_object__open(file);
> > > +       if (!obj)
> > > +               return -errno;
> > > +
> > > +       prog = bpf_object__next_program(obj, NULL);
> > > +       if (!prog) {
> > > +               err = -ENOENT;
> > > +               goto err_out;
> > > +       }
> > > +
> > > +       if (type != BPF_PROG_TYPE_UNSPEC)
> > > +               bpf_program__set_type(prog, type);
> > > +
> > > +       err = bpf_object__load(obj);
> > > +       if (err)
> > > +               goto err_out;
> > > +
> > > +       *pobj = obj;
> > > +       *prog_fd = bpf_program__fd(prog);
> > > +
> > > +       return 0;
> > > +err_out:
> > > +       bpf_object__close(obj);
> > > +       return err;
> > > +}
> >
> > Andrii,
> > That part of the commit broke verbose output in the test.
> > -v and -vv no longer work for tests that use bpf_prog_test_load()
> > because extra_prog_load_log_flags are ignored.
> >
> > Please fix.
>
> Ah, right, sorry, I should have used bpf_object__load_xattr() and
> passed those extra_prog_load_log_flags to it. I'll fix it.

Thanks. It's no rush.

> How important are BPF_F_TEST_RND_HI32 flags, btw? We need a
> bpf_program__set_extra_flags() to be able to set them, I'll add that
> as well. "extra" because they will be |'d with whatever the flags are
> determined by SEC_DEF().

I think it was useful in the past and recently RND_HI32 caused Martin
an extra day of painful debugging, but in the end, I think, it was
great that it was there.
Martin, pls correct me if I'm misremembering.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index c4497a4af3fe..5588c622d266 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -178,10 +178,6 @@  $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(wildcard bpf_testmod/Makefile bpf_tes
 	$(Q)$(MAKE) $(submake_extras) -C bpf_testmod
 	$(Q)cp bpf_testmod/bpf_testmod.ko $@
 
-$(OUTPUT)/test_stub.o: test_stub.c $(BPFOBJ)
-	$(call msg,CC,,$@)
-	$(Q)$(CC) -c $(CFLAGS) -o $@ $<
-
 DEFAULT_BPFTOOL := $(HOST_SCRATCH_DIR)/sbin/bpftool
 
 $(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
@@ -194,18 +190,23 @@  $(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
 
 TEST_GEN_PROGS_EXTENDED += $(DEFAULT_BPFTOOL)
 
-$(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/test_stub.o $(BPFOBJ)
-
-$(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
-$(OUTPUT)/test_skb_cgroup_id_user: cgroup_helpers.c
-$(OUTPUT)/test_sock: cgroup_helpers.c
-$(OUTPUT)/test_sock_addr: cgroup_helpers.c
-$(OUTPUT)/test_sockmap: cgroup_helpers.c
-$(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c
-$(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
-$(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
-$(OUTPUT)/test_sock_fields: cgroup_helpers.c
-$(OUTPUT)/test_sysctl: cgroup_helpers.c
+$(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
+
+$(OUTPUT)/test_dev_cgroup: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_skb_cgroup_id_user: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_sock: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_sock_addr: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_sockmap: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c testing_helpers.o
+$(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_cgroup_storage: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_sock_fields: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_sysctl: cgroup_helpers.c testing_helpers.o
+$(OUTPUT)/test_tag: testing_helpers.o
+$(OUTPUT)/test_lirc_mode2_user: testing_helpers.o
+$(OUTPUT)/xdping: testing_helpers.o
+$(OUTPUT)/flow_dissector_load: testing_helpers.o
+$(OUTPUT)/test_maps: testing_helpers.o
 
 BPFTOOL ?= $(DEFAULT_BPFTOOL)
 $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)    \
diff --git a/tools/testing/selftests/bpf/test_stub.c b/tools/testing/selftests/bpf/test_stub.c
deleted file mode 100644
index 47e132726203..000000000000
--- a/tools/testing/selftests/bpf/test_stub.c
+++ /dev/null
@@ -1,44 +0,0 @@ 
-// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-/* Copyright (C) 2019 Netronome Systems, Inc. */
-
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-#include <string.h>
-
-int extra_prog_load_log_flags = 0;
-
-int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
-		       struct bpf_object **pobj, int *prog_fd)
-{
-	struct bpf_prog_load_attr attr;
-
-	memset(&attr, 0, sizeof(struct bpf_prog_load_attr));
-	attr.file = file;
-	attr.prog_type = type;
-	attr.expected_attach_type = 0;
-	attr.prog_flags = BPF_F_TEST_RND_HI32;
-	attr.log_level = extra_prog_load_log_flags;
-
-	return bpf_prog_load_xattr(&attr, pobj, prog_fd);
-}
-
-int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
-			  size_t insns_cnt, const char *license,
-			  __u32 kern_version, char *log_buf,
-		     size_t log_buf_sz)
-{
-	struct bpf_load_program_attr load_attr;
-
-	memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
-	load_attr.prog_type = type;
-	load_attr.expected_attach_type = 0;
-	load_attr.name = NULL;
-	load_attr.insns = insns;
-	load_attr.insns_cnt = insns_cnt;
-	load_attr.license = license;
-	load_attr.kern_version = kern_version;
-	load_attr.prog_flags = BPF_F_TEST_RND_HI32;
-	load_attr.log_level = extra_prog_load_log_flags;
-
-	return bpf_load_program_xattr(&load_attr, log_buf, log_buf_sz);
-}
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 800d503e5cb4..ef61d43adfe4 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -1,7 +1,11 @@ 
 // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+/* Copyright (C) 2019 Netronome Systems, Inc. */
 /* Copyright (C) 2020 Facebook, Inc. */
 #include <stdlib.h>
+#include <string.h>
 #include <errno.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
 #include "testing_helpers.h"
 
 int parse_num_list(const char *s, bool **num_set, int *num_set_len)
@@ -78,3 +82,54 @@  __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info)
 	}
 	return info->prog_id;
 }
+
+int extra_prog_load_log_flags = 0;
+
+int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
+		       struct bpf_object **pobj, int *prog_fd)
+{
+	struct bpf_object *obj;
+	struct bpf_program *prog;
+	int err;
+
+	obj = bpf_object__open(file);
+	if (!obj)
+		return -errno;
+
+	prog = bpf_object__next_program(obj, NULL);
+	if (!prog) {
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	if (type != BPF_PROG_TYPE_UNSPEC)
+		bpf_program__set_type(prog, type);
+
+	err = bpf_object__load(obj);
+	if (err)
+		goto err_out;
+
+	*pobj = obj;
+	*prog_fd = bpf_program__fd(prog);
+
+	return 0;
+err_out:
+	bpf_object__close(obj);
+	return err;
+}
+
+int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
+			  size_t insns_cnt, const char *license,
+			  __u32 kern_version, char *log_buf,
+			  size_t log_buf_sz)
+{
+	LIBBPF_OPTS(bpf_prog_load_opts, opts,
+		.kern_version = kern_version,
+		.prog_flags = BPF_F_TEST_RND_HI32,
+		.log_level = extra_prog_load_log_flags,
+		.log_buf = log_buf,
+		.log_size = log_buf_sz,
+	);
+
+	return bpf_prog_load(type, NULL, license, insns, insns_cnt, &opts);
+}
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index d4f8e749611b..f46ebc476ee8 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -6,3 +6,9 @@ 
 
 int parse_num_list(const char *s, bool **set, int *set_len);
 __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info);
+int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
+		       struct bpf_object **pobj, int *prog_fd);
+int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
+			  size_t insns_cnt, const char *license,
+			  __u32 kern_version, char *log_buf,
+			  size_t log_buf_sz);