[bpf-next PATCH 0/5] tools/libbpf improvements and selftests

[bpf-next PATCH 0/5] tools/libbpf improvements and selftests

[Jesper Dangaard Brouer]

While playing with using libbpf for the Suricata project, we had
issues LLVM >= 4.0.1 generating ELF files that could not be loaded
with libbpf (tools/lib/bpf/).

During the troubleshooting phase, I wrote a test program and improved
the debugging output in libbpf.  I turned this into a selftests
program, and it also serves as a code example for libbpf in itself.

I discovered that there are at least three ELF load issues with
libbpf.  I left them as TODO comments in (tools/testing/selftests/bpf)
test_libbpf.sh. I've only fixed the load issue with eh_frames.  We can
work on the other issues later.

---

Jesper Dangaard Brouer (5):
      bpf: Sync kernel ABI header with tooling header for bpf_common.h
      tools/libbpf: improve the pr_debug statements to contain section numbers
      tools/libbpf: add test program for loading BPF ELF files
      selftests/bpf: add selftest that use test_libbpf_open
      tools/libbpf: handle issues with bpf ELF objects containing .eh_frames


 tools/testing/selftests/bpf/Makefile       |    9 +++++-
 tools/testing/selftests/bpf/test_libbpf.sh |   45 ++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/bpf/test_libbpf.sh

[bpf-next PATCH 1/5] bpf: Sync kernel ABI header with tooling header for bpf_common.h

[Jesper Dangaard Brouer]

I recently fixed up a lot of commits that forgot to keep the tooling
headers in sync.  And then I forgot to do the same thing in commit
cb5f7334d479 ("bpf: add comments to BPF ld/ldx sizes"). Let correct
that before people notice ;-).

Lawrence did partly fix/sync this for bpf.h in commit d6d4f60c3a09
("bpf: add selftest for tcpbpf").

Fixes: cb5f7334d479 ("bpf: add comments to BPF ld/ldx sizes")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 0 files changed

diff --git a/tools/include/uapi/linux/bpf_common.h b/tools/include/uapi/linux/bpf_common.h
index 18be90725ab0..ee97668bdadb 100644
--- a/tools/include/uapi/linux/bpf_common.h
+++ b/tools/include/uapi/linux/bpf_common.h
@@ -15,9 +15,10 @@
 
 /* ld/ldx fields */
 #define BPF_SIZE(code)  ((code) & 0x18)
-#define		BPF_W		0x00
-#define		BPF_H		0x08
-#define		BPF_B		0x10
+#define		BPF_W		0x00 /* 32-bit */
+#define		BPF_H		0x08 /* 16-bit */
+#define		BPF_B		0x10 /*  8-bit */
+/* eBPF		BPF_DW		0x18    64-bit */
 #define BPF_MODE(code)  ((code) & 0xe0)
 #define		BPF_IMM		0x00
 #define		BPF_ABS		0x20

[bpf-next PATCH 2/5] tools/libbpf: improve the pr_debug statements to contain section numbers

[Jesper Dangaard Brouer]

While debugging a bpf ELF loading issue, I needed to correlate the
ELF section number with the failed relocation section reference.
Thus, add section numbers/index to the pr_debug.

In debug mode, also print section that were skipped.  This helped
me identify that a section (.eh_frame) was skipped, and this was
the reason the relocation section (.rel.eh_frame) could not find
that section number.

The section numbers corresponds to the readelf tools Section Headers [Nr].

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 0 files changed

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 30c776375118..b4eeaa3ebff5 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -315,8 +315,8 @@ bpf_program__init(void *data, size_t size, char *section_name, int idx,
 
 	prog->section_name = strdup(section_name);
 	if (!prog->section_name) {
-		pr_warning("failed to alloc name for prog under section %s\n",
-			   section_name);
+		pr_warning("failed to alloc name for prog under section(%d) %s\n",
+			   idx, section_name);
 		goto errout;
 	}
 
@@ -759,29 +759,29 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 
 		idx++;
 		if (gelf_getshdr(scn, &sh) != &sh) {
-			pr_warning("failed to get section header from %s\n",
-				   obj->path);
+			pr_warning("failed to get section(%d) header from %s\n",
+				   idx, obj->path);
 			err = -LIBBPF_ERRNO__FORMAT;
 			goto out;
 		}
 
 		name = elf_strptr(elf, ep->e_shstrndx, sh.sh_name);
 		if (!name) {
-			pr_warning("failed to get section name from %s\n",
-				   obj->path);
+			pr_warning("failed to get section(%d) name from %s\n",
+				   idx, obj->path);
 			err = -LIBBPF_ERRNO__FORMAT;
 			goto out;
 		}
 
 		data = elf_getdata(scn, 0);
 		if (!data) {
-			pr_warning("failed to get section data from %s(%s)\n",
-				   name, obj->path);
+			pr_warning("failed to get section(%d) data from %s(%s)\n",
+				   idx, name, obj->path);
 			err = -LIBBPF_ERRNO__FORMAT;
 			goto out;
 		}
-		pr_debug("section %s, size %ld, link %d, flags %lx, type=%d\n",
-			 name, (unsigned long)data->d_size,
+		pr_debug("section(%d) %s, size %ld, link %d, flags %lx, type=%d\n",
+			 idx, name, (unsigned long)data->d_size,
 			 (int)sh.sh_link, (unsigned long)sh.sh_flags,
 			 (int)sh.sh_type);
 
@@ -836,6 +836,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 				obj->efile.reloc[n].shdr = sh;
 				obj->efile.reloc[n].data = data;
 			}
+		} else {
+			pr_debug("skip section(%d) %s\n", idx, name);
 		}
 		if (err)
 			goto out;
@@ -1115,8 +1117,7 @@ static int bpf_object__collect_reloc(struct bpf_object *obj)
 
 		prog = bpf_object__find_prog_by_idx(obj, idx);
 		if (!prog) {
-			pr_warning("relocation failed: no %d section\n",
-				   idx);
+			pr_warning("relocation failed: no section(%d)\n", idx);
 			return -LIBBPF_ERRNO__RELOC;
 		}
 

[bpf-next PATCH 3/5] tools/libbpf: add test program for loading BPF ELF files

[Jesper Dangaard Brouer]

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 0 files changed

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 83714ca1f22b..f968702f4ef6 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -147,11 +147,11 @@ LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
 
 CMD_TARGETS = $(LIB_FILE)
 
-TARGETS = $(CMD_TARGETS)
+TARGETS = $(CMD_TARGETS) test_libbpf_open
 
 all: fixdep all_cmd
 
-all_cmd: $(CMD_TARGETS)
+all_cmd: $(TARGETS)
 
 $(BPF_IN): force elfdep bpfdep
 	@(test -f ../../include/uapi/linux/bpf.h -a -f ../../../include/uapi/linux/bpf.h && ( \
@@ -168,6 +168,9 @@ $(OUTPUT)libbpf.so: $(BPF_IN)
 $(OUTPUT)libbpf.a: $(BPF_IN)
 	$(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
 
+test_libbpf_open: test_libbpf_open.c $(OUTPUT)libbpf.a
+	$(QUIET_LINK)$(CC) -lelf -Wall $< $(OUTPUT)libbpf.a -o $(OUTPUT)$@
+
 define do_install
 	if [ ! -d '$(DESTDIR_SQ)$2' ]; then		\
 		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2';	\
diff --git a/tools/lib/bpf/test_libbpf_open.c b/tools/lib/bpf/test_libbpf_open.c
new file mode 100644
index 000000000000..8fcd1c076add
--- /dev/null
+++ b/tools/lib/bpf/test_libbpf_open.c
@@ -0,0 +1,150 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * Copyright (c) 2018 Jesper Dangaard Brouer, Red Hat Inc.
+ */
+static const char *__doc__ =
+	"Libbpf test program for loading BPF ELF object files";
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdarg.h>
+#include <bpf/libbpf.h>
+#include <getopt.h>
+
+static const struct option long_options[] = {
+	{"help",	no_argument,		NULL, 'h' },
+	{"debug",	no_argument,		NULL, 'D' },
+	{"quiet",	no_argument,		NULL, 'q' },
+	{0, 0, NULL,  0 }
+};
+
+static void usage(char *argv[])
+{
+	int i;
+
+	printf("\nDOCUMENTATION:\n%s\n\n", __doc__);
+	printf(" Usage: %s (options-see-below) BPF_FILE\n", argv[0]);
+	printf(" Listing options:\n");
+	for (i = 0; long_options[i].name != 0; i++) {
+		printf(" --%-12s", long_options[i].name);
+		printf(" short-option: -%c",
+		       long_options[i].val);
+		printf("\n");
+	}
+	printf("\n");
+}
+
+#define DEFINE_PRINT_FN(name, enabled) \
+static int libbpf_##name(const char *fmt, ...)  	\
+{							\
+        va_list args;					\
+        int ret;					\
+							\
+        va_start(args, fmt);				\
+	if (enabled) {					\
+		fprintf(stderr, "[" #name "] ");	\
+		ret = vfprintf(stderr, fmt, args);	\
+	}						\
+        va_end(args);					\
+        return ret;					\
+}
+DEFINE_PRINT_FN(warning, 1)
+DEFINE_PRINT_FN(info, 1)
+DEFINE_PRINT_FN(debug, 1)
+
+#define EXIT_FAIL_LIBBPF EXIT_FAILURE
+#define EXIT_FAIL_OPTION 2
+
+int test_walk_progs(struct bpf_object *obj, bool verbose)
+{
+	struct bpf_program *prog;
+	int cnt = 0;
+
+	bpf_object__for_each_program(prog, obj) {
+		cnt++;
+		if (verbose)
+			printf("Prog (count:%d) section_name: %s\n", cnt,
+			       bpf_program__title(prog, false));
+	}
+	return 0;
+}
+
+int test_walk_maps(struct bpf_object *obj, bool verbose)
+{
+	struct bpf_map *map;
+	int cnt = 0;
+
+	bpf_map__for_each(map, obj) {
+		cnt++;
+		if (verbose)
+			printf("Map (count:%d) name: %s\n", cnt,
+			       bpf_map__name(map));
+	}
+	return 0;
+}
+
+int test_open_file(char *filename, bool verbose)
+{
+	struct bpf_object *bpfobj = NULL;
+	long err;
+
+	if (verbose)
+		printf("Open BPF ELF-file with libbpf: %s\n", filename);
+
+	/* Load BPF ELF object file and check for errors */
+	bpfobj = bpf_object__open(filename);
+	err = libbpf_get_error(bpfobj);
+	if (err) {
+		char err_buf[128];
+		libbpf_strerror(err, err_buf, sizeof(err_buf));
+		if (verbose)
+			printf("Unable to load eBPF objects in file '%s': %s\n",
+			       filename, err_buf);
+		return EXIT_FAIL_LIBBPF;
+	}
+	test_walk_progs(bpfobj, verbose);
+	test_walk_maps(bpfobj, verbose);
+
+	if (verbose)
+		printf("Close BPF ELF-file with libbpf: %s\n",
+		       bpf_object__name(bpfobj));
+	bpf_object__close(bpfobj);
+
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	char filename[1024] = { 0 };
+	bool verbose = 1;
+	int longindex = 0;
+	int opt;
+
+	libbpf_set_print(libbpf_warning, libbpf_info, NULL);
+
+	/* Parse commands line args */
+	while ((opt = getopt_long(argc, argv, "hDq",
+				  long_options, &longindex)) != -1) {
+		switch (opt) {
+		case 'D':
+			libbpf_set_print(libbpf_warning, libbpf_info,
+					 libbpf_debug);
+			break;
+		case 'q': /* Use in scripting mode */
+			verbose = 0;
+			break;
+		case 'h':
+		default:
+			usage(argv);
+			return EXIT_FAIL_OPTION;
+		}
+	}
+	if (optind >= argc) {
+		usage(argv);
+		printf("ERROR: Expected BPF_FILE argument after options\n");
+		return EXIT_FAIL_OPTION;
+	}
+	snprintf(filename, sizeof(filename), "%s", argv[optind]);
+
+	return test_open_file(filename, verbose);
+}

[bpf-next PATCH 4/5] selftests/bpf: add selftest that use test_libbpf_open

[Jesper Dangaard Brouer]

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 tools/testing/selftests/bpf/Makefile       |    9 +++++-
 tools/testing/selftests/bpf/test_libbpf.sh |   45 ++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/bpf/test_libbpf.sh

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index bf05bc5e36e5..ea2e7d498f5a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -13,6 +13,7 @@ endif
 CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) -I../../../include
 LDLIBS += -lcap -lelf -lrt -lpthread
 
+# Order correspond to 'make run_tests' order
 TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
 	test_align test_verifier_log test_dev_cgroup test_tcpbpf_user
 
@@ -22,7 +23,11 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
 	test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
 	sample_map_ret0.o test_tcpbpf_kern.o
 
-TEST_PROGS := test_kmod.sh test_xdp_redirect.sh test_xdp_meta.sh \
+# Order correspond to 'make run_tests' order
+TEST_PROGS := test_kmod.sh \
+	test_libbpf.sh \
+	test_xdp_redirect.sh \
+	test_xdp_meta.sh \
 	test_offload.py
 
 include ../lib.mk
@@ -36,6 +41,8 @@ $(TEST_GEN_PROGS): $(BPFOBJ)
 # force a rebuild of BPFOBJ when its dependencies are updated
 force:
 
+$(OUTPUT)/test_libbpf_open: $(BPFOBJ)
+
 $(BPFOBJ): force
 	$(MAKE) -C $(BPFDIR) OUTPUT=$(OUTPUT)/
 
diff --git a/tools/testing/selftests/bpf/test_libbpf.sh b/tools/testing/selftests/bpf/test_libbpf.sh
new file mode 100755
index 000000000000..bd623d2cbdb8
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_libbpf.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+export TESTNAME=test_libbpf
+
+# Determine selftest success via shell exit code
+exit_handler()
+{
+	if (( $? == 0 )); then
+		echo "selftests: $TESTNAME [PASS]";
+	else
+		echo "$TESTNAME: failed at file $LAST_LOADED" 1>&2
+		echo "selftests: $TESTNAME [FAILED]";
+	fi
+}
+
+libbpf_open_file()
+{
+	LAST_LOADED=$1
+	./test_libbpf_open --quiet $1
+}
+
+# Exit script immediately (well catched by trap handler) if any
+# program/thing exits with a non-zero status.
+set -e
+
+# (Use 'trap -l' to list meaning of numbers)
+trap exit_handler 0 2 3 6 9
+
+libbpf_open_file test_l4lb.o
+
+# TODO: fix libbpf to load noinline functions
+# [warning] libbpf: incorrect bpf_call opcode
+#libbpf_open_file test_l4lb_noinline.o
+
+# TODO: fix test_xdp_meta.c to load with libbpf
+# [warning] libbpf: test_xdp_meta.o doesn't provide kernel version
+#libbpf_open_file test_xdp_meta.o
+
+# TODO: fix libbpf to handle .eh_frame
+# [warning] libbpf: relocation failed: no section(10)
+#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o
+
+# Success
+exit 0

[bpf-next PATCH 5/5] tools/libbpf: handle issues with bpf ELF objects containing .eh_frames

[Jesper Dangaard Brouer]

If clang >= 4.0.1 is missing the option '-target bpf', it will cause
llc/llvm to create two ELF sections for "Exception Frames", with
section names '.eh_frame' and '.rel.eh_frame'.

The BPF ELF loader library libbpf fails when loading files with these
sections.  The other in-kernel BPF ELF loader in samples/bpf/bpf_load.c,
handle this gracefully. And iproute2 loader also seems to work with these
"eh" sections.

The issue in libbpf is caused by bpf_object__elf_collect() skip the
'.eh_frame' and thus doesn't create an internal data structure
pointing to this ELF section index.  Later when the relocation section
'.rel.eh_frame' is processed, it tries to find the '.eh_frame' via the
ELF section idx, which is that fails (in bpf_object__collect_reloc).

I couldn't find a way to see that the '.rel.eh_frame' was irrelevant
(that is only determined by looking at the section it reference, which
we no longer have info available on).

Thus, my solution is simply to match on the name of the relocation
section, to skip that too.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 0 files changed

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b4eeaa3ebff5..84e8bbe07347 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -822,6 +822,13 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 			void *reloc = obj->efile.reloc;
 			int nr_reloc = obj->efile.nr_reloc + 1;
 
+			/* Skip decoding of "eh" exception frames */
+			if (strcmp(name, ".rel.eh_frame") == 0) {
+				pr_debug("skip relo section %s(%d) for section(%d)\n",
+					 name, idx, sh.sh_info);
+				continue;
+			}
+
 			reloc = realloc(reloc,
 					sizeof(*obj->efile.reloc) * nr_reloc);
 			if (!reloc) {

Re: [bpf-next PATCH 5/5] tools/libbpf: handle issues with bpf ELF objects containing .eh_frames

[Eric Leblond]

Hi,

On Sat, 2018-01-27 at 18:27 +0100, Jesper Dangaard Brouer wrote:
> If clang >= 4.0.1 is missing the option '-target bpf', it will cause
> llc/llvm to create two ELF sections for "Exception Frames", with
> section names '.eh_frame' and '.rel.eh_frame'.
> 
> The BPF ELF loader library libbpf fails when loading files with these
> sections.  The other in-kernel BPF ELF loader in
> samples/bpf/bpf_load.c,
> handle this gracefully. And iproute2 loader also seems to work with
> these
> "eh" sections.
> 
> The issue in libbpf is caused by bpf_object__elf_collect() skip the
> '.eh_frame' and thus doesn't create an internal data structure
> pointing to this ELF section index.  Later when the relocation
> section
> '.rel.eh_frame' is processed, it tries to find the '.eh_frame' via
> the
> ELF section idx, which is that fails (in bpf_object__collect_reloc).
> 
> I couldn't find a way to see that the '.rel.eh_frame' was irrelevant
> (that is only determined by looking at the section it reference,
> which
> we no longer have info available on).
> 
> Thus, my solution is simply to match on the name of the relocation
> section, to skip that too.

I confirm this fixes the issue I have seen when loading XDP filter with
libbpf in Suricata.

BR,
-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/

Re: [bpf-next PATCH 0/5] tools/libbpf improvements and selftests

[Daniel Borkmann]

Hi Jesper,

On 01/27/2018 06:26 PM, Jesper Dangaard Brouer wrote:
> While playing with using libbpf for the Suricata project, we had
> issues LLVM >= 4.0.1 generating ELF files that could not be loaded
> with libbpf (tools/lib/bpf/).
> 
> During the troubleshooting phase, I wrote a test program and improved
> the debugging output in libbpf.  I turned this into a selftests
> program, and it also serves as a code example for libbpf in itself.
> 
> I discovered that there are at least three ELF load issues with
> libbpf.  I left them as TODO comments in (tools/testing/selftests/bpf)
> test_libbpf.sh. I've only fixed the load issue with eh_frames.  We can
> work on the other issues later.

The series looks great, and given the fix, we should get this into bpf.
Only one small request, could you move the test_libbpf_open.c into the
BPF selftests as well? We have test_libbpf.sh there which is the only
one making use of test_libbpf_open.c, so I think it fits much better if
we put them both into selftests. Otherwise rest is fine, thanks!

Regarding the TODO comments:

+# TODO: fix libbpf to load noinline functions
+# [warning] libbpf: incorrect bpf_call opcode
+#libbpf_open_file test_l4lb_noinline.o

Right, so this would require a newer llvm version that supports calls.
Maybe there could be a fallback to turn the noinline into __always_inline
for older llvms with dumping a warning to the user that this case cannot
properly be tested due to old llvm version.

+# TODO: fix test_xdp_meta.c to load with libbpf
+# [warning] libbpf: test_xdp_meta.o doesn't provide kernel version
+#libbpf_open_file test_xdp_meta.o

The kernel version is only required for tracing programs, although
it's just fine as well to put a dummy 'int _version SEC("version") = 1;'
here. The test_xdp_meta.sh uses iproute2 for loading into the kernel,
but no objections to add a version section there.

+# TODO: fix libbpf to handle .eh_frame
+# [warning] libbpf: relocation failed: no section(10)
+#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o

This is resolved in your last patch then, right? So the two could just
be swapped and this one uncommented, although it would require that
tracex3_kern.o has been built earlier.

Thanks,
Daniel

> Jesper Dangaard Brouer (5):
>       bpf: Sync kernel ABI header with tooling header for bpf_common.h
>       tools/libbpf: improve the pr_debug statements to contain section numbers
>       tools/libbpf: add test program for loading BPF ELF files
>       selftests/bpf: add selftest that use test_libbpf_open
>       tools/libbpf: handle issues with bpf ELF objects containing .eh_frames
> 
> 
>  tools/testing/selftests/bpf/Makefile       |    9 +++++-
>  tools/testing/selftests/bpf/test_libbpf.sh |   45 ++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/bpf/test_libbpf.sh
> 
> --
> 

Re: [bpf-next PATCH 0/5] tools/libbpf improvements and selftests

[Jesper Dangaard Brouer]

On Thu, 1 Feb 2018 11:59:29 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:

> Hi Jesper,
> 
> On 01/27/2018 06:26 PM, Jesper Dangaard Brouer wrote:
> > While playing with using libbpf for the Suricata project, we had
> > issues LLVM >= 4.0.1 generating ELF files that could not be loaded
> > with libbpf (tools/lib/bpf/).
> > 
> > During the troubleshooting phase, I wrote a test program and improved
> > the debugging output in libbpf.  I turned this into a selftests
> > program, and it also serves as a code example for libbpf in itself.
> > 
> > I discovered that there are at least three ELF load issues with
> > libbpf.  I left them as TODO comments in (tools/testing/selftests/bpf)
> > test_libbpf.sh. I've only fixed the load issue with eh_frames.  We can
> > work on the other issues later.  
> 
> The series looks great, and given the fix, we should get this into bpf.
>
> Only one small request, could you move the test_libbpf_open.c into the
> BPF selftests as well? We have test_libbpf.sh there which is the only
> one making use of test_libbpf_open.c, so I think it fits much better if
> we put them both into selftests. Otherwise rest is fine, thanks!

I'm happy that you noticed, but I will argue that the location of
test_libbpf_open.c is the right place.

I deliberately placed test_libbpf_open.c in tools/lib/bpf/ which is
together with the library that it uses, because it serves as an example
of howto use the library libbpf.

Plus, it is functional on its own. When people on the mailing list
report issues with libbpf, we can ask them to run the tool on their
bpf-elf objfile and quickly figure out that is wrong.

$ ./test_libbpf_open --debug ../../../samples/bpf/tracex1_kern.o 
Open BPF ELF-file with libbpf: ../../../samples/bpf/tracex1_kern.o
[debug] libbpf: loading ../../../samples/bpf/tracex1_kern.o
[debug] libbpf: section(1) .strtab, size 119, link 0, flags 0, type=3
[debug] libbpf: skip section(1) .strtab
[debug] libbpf: section(2) .text, size 0, link 0, flags 6, type=1
[debug] libbpf: skip section(2) .text
[debug] libbpf: section(3) kprobe/__netif_receive_skb_core, size 352, link 0, flags 6, type=1
[debug] libbpf: found program kprobe/__netif_receive_skb_core
[debug] libbpf: section(4) .rodata.str1.1, size 15, link 0, flags 32, type=1
[debug] libbpf: skip section(4) .rodata.str1.1
[debug] libbpf: section(5) license, size 4, link 0, flags 3, type=1
[debug] libbpf: license of ../../../samples/bpf/tracex1_kern.o is GPL
[debug] libbpf: section(6) version, size 4, link 0, flags 3, type=1
[debug] libbpf: kernel version of ../../../samples/bpf/tracex1_kern.o is 40f00
[debug] libbpf: section(7) .eh_frame, size 40, link 0, flags 2, type=1
[debug] libbpf: skip section(7) .eh_frame
[debug] libbpf: section(8) .rel.eh_frame, size 16, link 9, flags 0, type=9
[debug] libbpf: section(9) .symtab, size 144, link 1, flags 0, type=2
[warning] libbpf: relocation failed: no section(7)
Unable to load eBPF objects in file '../../../samples/bpf/tracex1_kern.o': Relocation failed

If the ELF objfile is non-broken, it will in "non-quiet" mode also list
the programs and maps found in the file:

$ ./test_libbpf_open  ../../testing/selftests/bpf/test_xdp.o
Open BPF ELF-file with libbpf: ../../testing/selftests/bpf/test_xdp.o
Prog (count:1) section_name: xdp_tx_iptunnel
Map (count:1) name: rxcnt
Map (count:2) name: vip2tnl
Close BPF ELF-file with libbpf: ../../testing/selftests/bpf/test_xdp.o



> Regarding the TODO comments:
> 
> +# TODO: fix libbpf to load noinline functions
> +# [warning] libbpf: incorrect bpf_call opcode
> +#libbpf_open_file test_l4lb_noinline.o
> 
> Right, so this would require a newer llvm version that supports calls.
> Maybe there could be a fallback to turn the noinline into __always_inline
> for older llvms with dumping a warning to the user that this case cannot
> properly be tested due to old llvm version.

Okay, good to know... guess it will be a while before we can enable
this test then.

> +# TODO: fix test_xdp_meta.c to load with libbpf
> +# [warning] libbpf: test_xdp_meta.o doesn't provide kernel version
> +#libbpf_open_file test_xdp_meta.o
> 
> The kernel version is only required for tracing programs, although
> it's just fine as well to put a dummy 'int _version SEC("version") = 1;'
> here. The test_xdp_meta.sh uses iproute2 for loading into the kernel,
> but no objections to add a version section there.

A lot of bpf-prog does not seems to set the version section, and other
loaders seems to ignore this ... maybe we should remove this
requirement from libbpf?


> +# TODO: fix libbpf to handle .eh_frame
> +# [warning] libbpf: relocation failed: no section(10)
> +#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o
> 
> This is resolved in your last patch then, right? So the two could just
> be swapped and this one uncommented, although it would require that
> tracex3_kern.o has been built earlier.

Yes, after my patch, we can enable this test, but as you write this
would require creating a Makefile build dependency, to samples/bpf,
like we have for tools/lib/bpf ... but I find that rather ugly, so I
didn't enable the test.

Either we should create a test-BPF prog in selftests/bpf/ that gets
compiled in a wrong fashion, or say this case will be covered by other
BPF-progs.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [bpf-next PATCH 0/5] tools/libbpf improvements and selftests

[Daniel Borkmann]

On 02/01/2018 03:41 PM, Jesper Dangaard Brouer wrote:
> On Thu, 1 Feb 2018 11:59:29 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
>> Hi Jesper,
>>
>> On 01/27/2018 06:26 PM, Jesper Dangaard Brouer wrote:
>>> While playing with using libbpf for the Suricata project, we had
>>> issues LLVM >= 4.0.1 generating ELF files that could not be loaded
>>> with libbpf (tools/lib/bpf/).
>>>
>>> During the troubleshooting phase, I wrote a test program and improved
>>> the debugging output in libbpf.  I turned this into a selftests
>>> program, and it also serves as a code example for libbpf in itself.
>>>
>>> I discovered that there are at least three ELF load issues with
>>> libbpf.  I left them as TODO comments in (tools/testing/selftests/bpf)
>>> test_libbpf.sh. I've only fixed the load issue with eh_frames.  We can
>>> work on the other issues later.  
>>
>> The series looks great, and given the fix, we should get this into bpf.
>>
>> Only one small request, could you move the test_libbpf_open.c into the
>> BPF selftests as well? We have test_libbpf.sh there which is the only
>> one making use of test_libbpf_open.c, so I think it fits much better if
>> we put them both into selftests. Otherwise rest is fine, thanks!
> 
> I'm happy that you noticed, but I will argue that the location of
> test_libbpf_open.c is the right place.
> 
> I deliberately placed test_libbpf_open.c in tools/lib/bpf/ which is
> together with the library that it uses, because it serves as an example
> of howto use the library libbpf.
> 
> Plus, it is functional on its own. When people on the mailing list
> report issues with libbpf, we can ask them to run the tool on their
> bpf-elf objfile and quickly figure out that is wrong.

Don't get me wrong, I'm not at all against such tool or test, I think
it's a great idea and needed. I just think that tools/lib/bpf/ is not
the right place to put it into lib directory. Right now, as you say,
it's a mixture of example code on how to use the lib, and tool at the
same time to dump/test load an object file with libbpf.

I think there are a couple of options depending on the main use case:
sample, pure test case, or tool. I think the latter would be the most
fitting and useful in fact. Often when having loader issues (like the
one you ran into in your last patch), then 'readelf -a' helps to check
out sections and their correlations, such a tool could display it more
user friendly, leaving out the unrelated bits, and could do a dry-load
as well at the same time.

Then lets integrate such an object dump into bpftool, and run it from
the BPF selftests. This would be most beneficial, imo. bpftool already
uses libbpf as a loader and it could provide a new subcommand for the
dump e.g. 'bpftool prog dump object FILE' to debug the contents of it.
At the same time we could also have a dry-run for loading the object,
and given 49a086c201a9 ("bpftool: implement prog load command"), we're
basically there already. It could be a 'bpftool probe OBJ' that would
dump the verifier log unconditionally and in case of success just
closes the prog and exits again. I think this would bring the most
benefit for users to have this functionality integrated in bpftool.
Could you change the few bits to integrate it there? That would be
really great.

> $ ./test_libbpf_open --debug ../../../samples/bpf/tracex1_kern.o 
> Open BPF ELF-file with libbpf: ../../../samples/bpf/tracex1_kern.o
> [debug] libbpf: loading ../../../samples/bpf/tracex1_kern.o
> [debug] libbpf: section(1) .strtab, size 119, link 0, flags 0, type=3
> [debug] libbpf: skip section(1) .strtab
> [debug] libbpf: section(2) .text, size 0, link 0, flags 6, type=1
> [debug] libbpf: skip section(2) .text
> [debug] libbpf: section(3) kprobe/__netif_receive_skb_core, size 352, link 0, flags 6, type=1
> [debug] libbpf: found program kprobe/__netif_receive_skb_core
> [debug] libbpf: section(4) .rodata.str1.1, size 15, link 0, flags 32, type=1
> [debug] libbpf: skip section(4) .rodata.str1.1
> [debug] libbpf: section(5) license, size 4, link 0, flags 3, type=1
> [debug] libbpf: license of ../../../samples/bpf/tracex1_kern.o is GPL
> [debug] libbpf: section(6) version, size 4, link 0, flags 3, type=1
> [debug] libbpf: kernel version of ../../../samples/bpf/tracex1_kern.o is 40f00
> [debug] libbpf: section(7) .eh_frame, size 40, link 0, flags 2, type=1
> [debug] libbpf: skip section(7) .eh_frame
> [debug] libbpf: section(8) .rel.eh_frame, size 16, link 9, flags 0, type=9
> [debug] libbpf: section(9) .symtab, size 144, link 1, flags 0, type=2
> [warning] libbpf: relocation failed: no section(7)
> Unable to load eBPF objects in file '../../../samples/bpf/tracex1_kern.o': Relocation failed
[...]
>> +# TODO: fix test_xdp_meta.c to load with libbpf
>> +# [warning] libbpf: test_xdp_meta.o doesn't provide kernel version
>> +#libbpf_open_file test_xdp_meta.o
>>
>> The kernel version is only required for tracing programs, although
>> it's just fine as well to put a dummy 'int _version SEC("version") = 1;'
>> here. The test_xdp_meta.sh uses iproute2 for loading into the kernel,
>> but no objections to add a version section there.
> 
> A lot of bpf-prog does not seems to set the version section, and other
> loaders seems to ignore this ... maybe we should remove this
> requirement from libbpf?

I don't really have a strong opinion on that one. For tracing programs,
this is basically required. I think it's good that bpf_object__validate()
would warn about it and return an explicit error (-LIBBPF_ERRNO__KVERSION)
to the user, in case of kernel, it's -EINVAL, which doesn't really say
much what went wrong (aka 'missing kernel version'). Maybe libbpf API
could support both options, though, so that in case of networking this
can just be ignored.

>> +# TODO: fix libbpf to handle .eh_frame
>> +# [warning] libbpf: relocation failed: no section(10)
>> +#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o
>>
>> This is resolved in your last patch then, right? So the two could just
>> be swapped and this one uncommented, although it would require that
>> tracex3_kern.o has been built earlier.
> 
> Yes, after my patch, we can enable this test, but as you write this
> would require creating a Makefile build dependency, to samples/bpf,
> like we have for tools/lib/bpf ... but I find that rather ugly, so I
> didn't enable the test.
> 
> Either we should create a test-BPF prog in selftests/bpf/ that gets
> compiled in a wrong fashion, or say this case will be covered by other
> BPF-progs.

That makes sense, potentially the existing progs could be compiled and
test-loaded with both variants of invoking clang and passing to llc.

Thanks,
Daniel

Re: [bpf-next PATCH 0/5] tools/libbpf improvements and selftests

[Jesper Dangaard Brouer]

On Thu, 1 Feb 2018 18:56:14 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:

> Don't get me wrong, I'm not at all against such tool or test, I think
> it's a great idea and needed. I just think that tools/lib/bpf/ is not
> the right place to put it into lib directory. Right now, as you say,
> it's a mixture of example code on how to use the lib, and tool at the
> same time to dump/test load an object file with libbpf.

Okay, to avoid polluting the directory of the library with test/samples
programs, bow for your suggestion of moving the file to the selftests
directory.

I'm at FOSDEM now, and I cannot send a V2 patch right now... If you
need this in fast (due merge timing), you can make the change yourself
and apply it... else I'll send a V2 on Tuesday.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [bpf-next PATCH 0/5] tools/libbpf improvements and selftests

[Daniel Borkmann]

On 02/04/2018 10:19 AM, Jesper Dangaard Brouer wrote:
> On Thu, 1 Feb 2018 18:56:14 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
>> Don't get me wrong, I'm not at all against such tool or test, I think
>> it's a great idea and needed. I just think that tools/lib/bpf/ is not
>> the right place to put it into lib directory. Right now, as you say,
>> it's a mixture of example code on how to use the lib, and tool at the
>> same time to dump/test load an object file with libbpf.
> 
> Okay, to avoid polluting the directory of the library with test/samples
> programs, bow for your suggestion of moving the file to the selftests
> directory.
> 
> I'm at FOSDEM now, and I cannot send a V2 patch right now... If you
> need this in fast (due merge timing), you can make the change yourself
> and apply it... else I'll send a V2 on Tuesday.

Okay, then lets go with Tuesday since currently bpf pull-request is still
pending, thus this fix would go out with the next one anyway.

Thanks,
Daniel