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

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
      selftests/bpf: 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/include/uapi/linux/bpf_common.h          |    7 +
 tools/lib/bpf/libbpf.c                         |   32 +++--
 tools/testing/selftests/bpf/Makefile           |   12 ++
 tools/testing/selftests/bpf/test_libbpf.sh     |   49 ++++++++
 tools/testing/selftests/bpf/test_libbpf_open.c |  150 ++++++++++++++++++++++++
 5 files changed, 234 insertions(+), 16 deletions(-)
 create mode 100755 tools/testing/selftests/bpf/test_libbpf.sh
 create mode 100644 tools/testing/selftests/bpf/test_libbpf_open.c

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 <redacted>
---
 tools/include/uapi/linux/bpf_common.h |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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

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 <redacted>
---
 tools/lib/bpf/libbpf.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

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;
 		}
 

V2: Moved program into selftests/bpf from tools/libbpf

This program can be used on its own for testing/debugging if a
BPF ELF-object file can be loaded with libbpf (from tools/lib/bpf).

If something is wrong with the ELF object, the program have
a --debug mode that will display the ELF sections and especially
the skipped sections.  This allows for quickly identifying the
problematic ELF section number, which can be corrolated with the
readelf tool.

The program signal error via return codes, and also have
a --quiet mode, which is practical for use in scripts like
selftests/bpf.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 tools/testing/selftests/bpf/Makefile           |    2 
 tools/testing/selftests/bpf/test_libbpf_open.c |  150 ++++++++++++++++++++++++
 2 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/test_libbpf_open.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index bf05bc5e36e5..de7b85e2e532 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile

@@ -20,7 +20,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
 	test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o     \
 	sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o test_tracepoint.o \
 	test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
-	sample_map_ret0.o test_tcpbpf_kern.o
+	sample_map_ret0.o test_tcpbpf_kern.o test_libbpf_open
 
 TEST_PROGS := test_kmod.sh test_xdp_redirect.sh test_xdp_meta.sh \
 	test_offload.py

diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
new file mode 100644
index 000000000000..8fcd1c076add
--- /dev/null
+++ b/tools/testing/selftests/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);
+}

This script test_libbpf.sh will be part of the 'make run_tests'
invocation, but can also be invoked manually in this directory,
and a verbose mode can be enabled via setting the environment
variable $VERBOSE like:

 $ VERBOSE=yes ./test_libbpf.sh

The script contains some tests that are commented out, as they
currently fail.  They are reminders about what we need to improve
for the libbpf loader library.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 tools/testing/selftests/bpf/Makefile       |   14 +++++++-
 tools/testing/selftests/bpf/test_libbpf.sh |   49 ++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 2 deletions(-)
 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 de7b85e2e532..8b5667714250 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
 

@@ -20,17 +21,26 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
 	test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o     \
 	sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o test_tracepoint.o \
 	test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
-	sample_map_ret0.o test_tcpbpf_kern.o test_libbpf_open
+	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
 
+# Compile but not part of 'make run_tests'
+TEST_GEN_PROGS_EXTENDED = test_libbpf_open
+
 include ../lib.mk
 
 BPFOBJ := $(OUTPUT)/libbpf.a $(OUTPUT)/cgroup_helpers.c
 
 $(TEST_GEN_PROGS): $(BPFOBJ)
 
+$(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/libbpf.a
+
 .PHONY: force
 
 # force a rebuild of BPFOBJ when its dependencies are updated

diff --git a/tools/testing/selftests/bpf/test_libbpf.sh b/tools/testing/selftests/bpf/test_libbpf.sh
new file mode 100755
index 000000000000..d97dc914cd49
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_libbpf.sh

@@ -0,0 +1,49 @@
+#!/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
+	if [ -n "$VERBOSE" ]; then
+	    ./test_libbpf_open $1
+	else
+	    ./test_libbpf_open --quiet $1
+	fi
+}
+
+# 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

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.

Note, for samples/bpf/ the '-target bpf' parameter to clang cannot be used
due to incompatibility with asm embedded headers, that some of the samples
include. This is explained in more details by Yonghong Song in bpf_devel_QA.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 tools/lib/bpf/libbpf.c |    7 +++++++
 1 file changed, 7 insertions(+)

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) {

On Tue, Feb 06, 2018 at 03:54:28PM +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).

but does this approach work for all extra sections and relocations emitted
when source is compiled with -g ?
To address this case bpf_load.c does:
  if (shdr.sh_type == SHT_REL) {
          struct bpf_insn *insns;

          /* locate prog sec that need map fixup (relocations) */
          if (get_sec(elf, shdr.sh_info, &ehdr, &shname_prog,
                      &shdr_prog, &data_prog))
                  continue;

          if (shdr_prog.sh_type != SHT_PROGBITS ||
              !(shdr_prog.sh_flags & SHF_EXECINSTR))
                  continue;

why the same approach is not applicable here?

I guess we can apply this workaround as-is but it looks incomplete.

On Tue, 6 Feb 2018 08:00:59 -0800 Alexei Starovoitov [off-list ref] wrote:

On Tue, Feb 06, 2018 at 03:54:28PM +0100, Jesper Dangaard Brouer wrote:

quoted

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).  

but does this approach work for all extra sections and relocations emitted
when source is compiled with -g ?

No, but I plan to follow up and do a more complete solution later. This
is a workaround to get the Suricata use-case working and also that
samples/bpf/ can be loaded.

To address this case bpf_load.c does:
  if (shdr.sh_type == SHT_REL) {
          struct bpf_insn *insns;

          /* locate prog sec that need map fixup (relocations) */
          if (get_sec(elf, shdr.sh_info, &ehdr, &shname_prog,
                      &shdr_prog, &data_prog))
                  continue;

          if (shdr_prog.sh_type != SHT_PROGBITS ||
              !(shdr_prog.sh_flags & SHF_EXECINSTR))
                  continue;

why the same approach is not applicable here?

As described above bpf_object__elf_collect() skip the "real" section
that the relo-section want to lookup (based on the same kind of
check), but libbpf is now missing the section idx in its internal
structures... and thus the relo lookup of the idx fails. (bpf_load.c
does the lookup in the ELF obj directly, thus it does not have this
problem).

I guess we can apply this workaround as-is but it looks incomplete.

Yes, it is a workaround to move forward... it requires a larger change
to libbpf, so it stores idx'es of skipped sections.

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

On 02/06/2018 06:03 PM, Jesper Dangaard Brouer wrote:

On Tue, 6 Feb 2018 08:00:59 -0800 Alexei Starovoitov [off-list ref] wrote:

quoted

On Tue, Feb 06, 2018 at 03:54:28PM +0100, Jesper Dangaard Brouer wrote:

quoted

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).  

but does this approach work for all extra sections and relocations emitted
when source is compiled with -g ?

No, but I plan to follow up and do a more complete solution later. This
is a workaround to get the Suricata use-case working and also that
samples/bpf/ can be loaded.

Aside from a needed fix in any case, is there a specifc reason why Suricata
cannot rely on 'clang -target bpf'? Is it asm inline headers in your case?

quoted

To address this case bpf_load.c does:
  if (shdr.sh_type == SHT_REL) {
          struct bpf_insn *insns;

          /* locate prog sec that need map fixup (relocations) */
          if (get_sec(elf, shdr.sh_info, &ehdr, &shname_prog,
                      &shdr_prog, &data_prog))
                  continue;

          if (shdr_prog.sh_type != SHT_PROGBITS ||
              !(shdr_prog.sh_flags & SHF_EXECINSTR))
                  continue;

why the same approach is not applicable here?

As described above bpf_object__elf_collect() skip the "real" section
that the relo-section want to lookup (based on the same kind of
check), but libbpf is now missing the section idx in its internal
structures... and thus the relo lookup of the idx fails. (bpf_load.c
does the lookup in the ELF obj directly, thus it does not have this
problem).

Out of curiosity, I just double checked iproute2 loader (examples/bpf/):

$ clang -O2 -g -emit-llvm -c bpf_cyclic.c -o - | llc -march=bpf -mcpu=probe -filetype=obj -o bpf_cyclic.o
$ readelf -a bpf_cyclic.o | grep "\["
  [Nr] Name              Type             Address           Offset
  [ 0]                   NULL             0000000000000000  00000000
  [ 1] .strtab           STRTAB           0000000000000000  000016b0
  [ 2] .text             PROGBITS         0000000000000000  00000040
  [ 3] 0xabccba/0        PROGBITS         0000000000000000  00000040
  [ 4] .rel0xabccba/0    REL              0000000000000000  00001120
  [ 5] classifier        PROGBITS         0000000000000000  000000e8
  [ 6] .relclassifier    REL              0000000000000000  00001130
  [ 7] maps              PROGBITS         0000000000000000  00000118
  [ 8] license           PROGBITS         0000000000000000  0000013c
  [ 9] .debug_str        PROGBITS         0000000000000000  00000140
  [10] .debug_loc        PROGBITS         0000000000000000  000003d5
  [11] .rel.debug_loc    REL              0000000000000000  00001140
  [12] .debug_abbrev     PROGBITS         0000000000000000  0000045a
  [13] .debug_info       PROGBITS         0000000000000000  0000055c
  [14] .rel.debug_info   REL              0000000000000000  000011c0
  [15] .debug_ranges     PROGBITS         0000000000000000  0000088c
  [16] .rel.debug_ranges REL              0000000000000000  000015d0
  [17] .debug_macinfo    PROGBITS         0000000000000000  000008ec
  [18] .debug_pubnames   PROGBITS         0000000000000000  000008ed
  [19] .rel.debug_pubnam REL              0000000000000000  00001650
  [20] .debug_pubtypes   PROGBITS         0000000000000000  00000954
  [21] .rel.debug_pubtyp REL              0000000000000000  00001660
  [22] .eh_frame         PROGBITS         0000000000000000  000009c0
  [23] .rel.eh_frame     REL              0000000000000000  00001670
  [24] .debug_line       PROGBITS         0000000000000000  00000a10
  [25] .rel.debug_line   REL              0000000000000000  00001690
  [26] .symtab           SYMTAB           0000000000000000  00000b08
# tc qdisc add dev lo clsact
# tc filter add dev lo ingress bpf da obj bpf_cyclic.o
# tc filter show dev lo ingress
filter protocol all pref 49152 bpf chain 0
filter protocol all pref 49152 bpf chain 0 handle 0x1 bpf_cyclic.o:[classifier] direct-action not_in_hw id 6 tag 736a8a004dead229

So no problems. What it does internally is pretty similar to what Alexei
described; for programs, they need to have ELF section header type of
SHT_PROGBITS and section header flags must match on SHF_EXECINSTR in
the relocation parsing.

Now, picking out two, and looking at the flags:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
[...]
  [ 5] classifier        PROGBITS         0000000000000000  000000e8
       0000000000000030  0000000000000000  AX       0     0     8
[...]
  [22] .eh_frame         PROGBITS         0000000000000000  000009c0
       0000000000000050  0000000000000000   A       0     0     8
[...]
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings)
  I (info), L (link order), G (group), T (TLS), E (exclude), x (unknown)
  O (extra OS processing required) o (OS specific), p (processor specific)

So .eh_frame doesn't even have SHF_EXECINSTR set. Why it cannot test on
this? Doing strcmp(name, ".rel.eh_frame") == 0 test is indeed a bit
fragile in the sense that we would also need to strcmp() all the others
listed above since libbpf could trip over them just as well. When you
check the SHT_REL sections, the target section index sits in GElf_Shdr's
.sh_info, so the only thing that would need to be done in this case is
to look up the ELF section header with index from .sh_info, get the
GElf_Shdr section header and check for a match on SHT_PROGBITS/SHF_EXECINSTR,
otherwise skip that SHT_REL section. A direct lookup of the index in
the obj would not require any complex section/index tracking or larger
rework in libbpf, hmm, what am I missing?

quoted

I guess we can apply this workaround as-is but it looks incomplete.

Yes, it is a workaround to move forward... it requires a larger change
to libbpf, so it stores idx'es of skipped sections.

Thanks,
Daniel

On Tue, 6 Feb 2018 20:05:43 +0100 Daniel Borkmann [off-list ref] wrote:

On 02/06/2018 06:03 PM, Jesper Dangaard Brouer wrote:

[...]

quoted

[...] I plan to follow up and do a more complete solution later. This
is a workaround to get the Suricata use-case working and also that
samples/bpf/ can be loaded.  

Aside from a needed fix in any case, is there a specifc reason why Suricata
cannot rely on 'clang -target bpf'? Is it asm inline headers in your case?

Below is the error I get when using 'clang' with '-target bpf'

$ dirs
~/git/suricata/src/ebpf

$ clang -Wall -Iinclude -O2 -D__KERNEL__  -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
In file included from xdp_filter.c:19:
In file included from /usr/bin/../lib64/clang/4.0.1/include/stdint.h:63:
In file included from /usr/include/stdint.h:26:
In file included from /usr/include/bits/libc-header-start.h:33:
In file included from /usr/include/features.h:434:
/usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
# include <gnu/stubs-32.h>
          ^~~~~~~~~~~~~~~~

I'll leave it up to Eric Leblond to figure out that he need to change
in the eBPF programs to make it compile with '-target bpf'.  Maybe you
can offer him some guidance here?

Direct link to code:
 https://github.com/OISF/suricata/blob/master/ebpf/xdp_filter.c

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

On 02/07/2018 01:40 PM, Jesper Dangaard Brouer wrote:

On Tue, 6 Feb 2018 20:05:43 +0100 Daniel Borkmann [off-list ref] wrote:

quoted

On 02/06/2018 06:03 PM, Jesper Dangaard Brouer wrote:

[...]

quoted

quoted

[...] I plan to follow up and do a more complete solution later. This
is a workaround to get the Suricata use-case working and also that
samples/bpf/ can be loaded.  

Aside from a needed fix in any case, is there a specifc reason why Suricata
cannot rely on 'clang -target bpf'? Is it asm inline headers in your case?

Below is the error I get when using 'clang' with '-target bpf'

$ dirs
~/git/suricata/src/ebpf

$ clang -Wall -Iinclude -O2 -D__KERNEL__  -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
In file included from xdp_filter.c:19:
In file included from /usr/bin/../lib64/clang/4.0.1/include/stdint.h:63:
In file included from /usr/include/stdint.h:26:
In file included from /usr/include/bits/libc-header-start.h:33:
In file included from /usr/include/features.h:434:
/usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
# include <gnu/stubs-32.h>
          ^~~~~~~~~~~~~~~~

I'll leave it up to Eric Leblond to figure out that he need to change
in the eBPF programs to make it compile with '-target bpf'.  Maybe you
can offer him some guidance here?

Direct link to code:
 https://github.com/OISF/suricata/blob/master/ebpf/xdp_filter.c

Sure, you just need glibc-devel.i686, see:

$ clang -Wall -Iinclude -O2 -D__KERNEL__ -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
  In file included from xdp_filter.c:19:
  In file included from /home/darkstar/llvm/build/lib/clang/7.0.0/include/stdint.h:63:
  In file included from /usr/include/stdint.h:25:
  In file included from /usr/include/features.h:392:
  /usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
  # include <gnu/stubs-32.h>
            ^~~~~~~~~~~~~~~~
  1 error generated.
# yum install glibc-devel.i686
  [...]
$ clang -Wall -Iinclude -O2 -D__KERNEL__ -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
$

Alternatively, you could do something like done in selftests to provide a
dummy, see commit 1c2dd16add7e ("selftests/bpf: get rid of -D__x86_64__").

Cheers,
Daniel

On Wed, 7 Feb 2018 14:19:00 +0100
Daniel Borkmann [off-list ref] wrote:

On 02/07/2018 01:40 PM, Jesper Dangaard Brouer wrote:

quoted

On Tue, 6 Feb 2018 20:05:43 +0100 Daniel Borkmann [off-list ref] wrote:  

quoted

On 02/06/2018 06:03 PM, Jesper Dangaard Brouer wrote:  

[...]  

quoted

quoted

[...] I plan to follow up and do a more complete solution later. This
is a workaround to get the Suricata use-case working and also that
samples/bpf/ can be loaded.    

Aside from a needed fix in any case, is there a specifc reason why Suricata
cannot rely on 'clang -target bpf'? Is it asm inline headers in your case?  

Below is the error I get when using 'clang' with '-target bpf'

$ dirs
~/git/suricata/src/ebpf

$ clang -Wall -Iinclude -O2 -D__KERNEL__  -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
In file included from xdp_filter.c:19:
In file included from /usr/bin/../lib64/clang/4.0.1/include/stdint.h:63:
In file included from /usr/include/stdint.h:26:
In file included from /usr/include/bits/libc-header-start.h:33:
In file included from /usr/include/features.h:434:
/usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
# include <gnu/stubs-32.h>
          ^~~~~~~~~~~~~~~~

I'll leave it up to Eric Leblond to figure out that he need to change
in the eBPF programs to make it compile with '-target bpf'.  Maybe you
can offer him some guidance here?

Direct link to code:
 https://github.com/OISF/suricata/blob/master/ebpf/xdp_filter.c  

Sure, you just need glibc-devel.i686, see:

$ clang -Wall -Iinclude -O2 -D__KERNEL__ -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
  In file included from xdp_filter.c:19:
  In file included from /home/darkstar/llvm/build/lib/clang/7.0.0/include/stdint.h:63:
  In file included from /usr/include/stdint.h:25:
  In file included from /usr/include/features.h:392:
  /usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
  # include <gnu/stubs-32.h>
            ^~~~~~~~~~~~~~~~
  1 error generated.
# yum install glibc-devel.i686
  [...]
$ clang -Wall -Iinclude -O2 -D__KERNEL__ -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf

$

Could you please explain why if makes a difference to install glibc-devel.i686 ?


How will people compiling suricata figure out the new dependency, that
on their 64-bit (x86_64) distro's they also need to install the 32-bit
(i686) variant of glibc-devel ?

Alternatively, you could do something like done in selftests to provide a
dummy, see commit 1c2dd16add7e ("selftests/bpf: get rid of -D__x86_64__").

That is a funny way to workaround the problem (having an empty
<gnu/stubs.h> file in include path), but it might be a better solution
to avoid frustrations for people compiling suricata.


An alternative solution is to NOT:
 #include <stdint.h>
 #include <string.h>

And then change:
 uint64_t -> __u64
 uint32_t -> __u32
 uint16_t -> __u16
 uint8_t  -> __u8

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

On 02/07/2018 03:58 PM, Jesper Dangaard Brouer wrote:

On Wed, 7 Feb 2018 14:19:00 +0100
Daniel Borkmann [off-list ref] wrote:

quoted

On 02/07/2018 01:40 PM, Jesper Dangaard Brouer wrote:

quoted

On Tue, 6 Feb 2018 20:05:43 +0100 Daniel Borkmann [off-list ref] wrote:  

quoted

On 02/06/2018 06:03 PM, Jesper Dangaard Brouer wrote:  

[...]  

quoted

quoted

[...] I plan to follow up and do a more complete solution later. This
is a workaround to get the Suricata use-case working and also that
samples/bpf/ can be loaded.    

Aside from a needed fix in any case, is there a specifc reason why Suricata
cannot rely on 'clang -target bpf'? Is it asm inline headers in your case?  

Below is the error I get when using 'clang' with '-target bpf'

$ dirs
~/git/suricata/src/ebpf

$ clang -Wall -Iinclude -O2 -D__KERNEL__  -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
In file included from xdp_filter.c:19:
In file included from /usr/bin/../lib64/clang/4.0.1/include/stdint.h:63:
In file included from /usr/include/stdint.h:26:
In file included from /usr/include/bits/libc-header-start.h:33:
In file included from /usr/include/features.h:434:
/usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
# include <gnu/stubs-32.h>
          ^~~~~~~~~~~~~~~~

I'll leave it up to Eric Leblond to figure out that he need to change
in the eBPF programs to make it compile with '-target bpf'.  Maybe you
can offer him some guidance here?

Direct link to code:
 https://github.com/OISF/suricata/blob/master/ebpf/xdp_filter.c  

Sure, you just need glibc-devel.i686, see:

$ clang -Wall -Iinclude -O2 -D__KERNEL__ -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf
  In file included from xdp_filter.c:19:
  In file included from /home/darkstar/llvm/build/lib/clang/7.0.0/include/stdint.h:63:
  In file included from /usr/include/stdint.h:25:
  In file included from /usr/include/features.h:392:
  /usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
  # include <gnu/stubs-32.h>
            ^~~~~~~~~~~~~~~~
  1 error generated.
# yum install glibc-devel.i686
  [...]
$ clang -Wall -Iinclude -O2 -D__KERNEL__ -target bpf -emit-llvm -c xdp_filter.c -o - | llc -march=bpf -filetype=obj -o xdp_filter.bpf

quoted

$

Could you please explain why if makes a difference to install glibc-devel.i686 ?

Well, see what /usr/include/gnu/stubs.h is doing, on x86_64 it's:

  #if !defined __x86_64__
  # include <gnu/stubs-32.h>
  #endif
  #if defined __x86_64__ && defined __LP64__
  # include <gnu/stubs-64.h>
  #endif
  #if defined __x86_64__ && defined __ILP32__
  # include <gnu/stubs-x32.h>
  #endif

If you do clang -target bpf, then clang will have '__bpf__' defined instead
of '__x86_64__' hence the gnu/stubs-32.h include attempt, and the workaround
used in selftests with -D__x86_64__. But the -D__x86_64__ is not portable, so
yeah, either dummy stubs if you need to include the headers or also other the
workaround you mention below.

[...]

quoted

Alternatively, you could do something like done in selftests to provide a
dummy, see commit 1c2dd16add7e ("selftests/bpf: get rid of -D__x86_64__").

That is a funny way to workaround the problem (having an empty
<gnu/stubs.h> file in include path), but it might be a better solution
to avoid frustrations for people compiling suricata.

An alternative solution is to NOT:
 #include <stdint.h>
 #include <string.h>

And then change:
 uint64_t -> __u64
 uint32_t -> __u32
 uint16_t -> __u16
 uint8_t  -> __u8

Hi Eric,

I've improved the Suricata ebpf makefile, in-order to avoid generating
the .eh_frame sections.  This required changing the code a bit, to
allow using clang -target bpf.

The makefile have also been improved to stop on clang compile errors,
instead of generating an almost empty BPF ELF file.

Could I ask you to get these changes into Suricata, through correct
process for this Open Source project?

--Jesper

---

Jesper Dangaard Brouer (3):
      suricata/ebpf: take clang -target bpf include issue of stdint.h into account
      suricata/ebpf: compile with clang -target bpf
      suricata/ebpf: improving the ebpf makefile


 ebpf/Makefile.am     |   22 ++++++++++++++++++----
 ebpf/bypass_filter.c |   27 +++++++++++++--------------
 ebpf/filter.c        |    3 +--
 ebpf/hash_func01.h   |   12 ++++++------
 ebpf/lb.c            |   11 +++++------
 ebpf/vlan_filter.c   |    5 ++---
 ebpf/xdp_filter.c    |   42 ++++++++++++++++++++----------------------
 7 files changed, 65 insertions(+), 57 deletions(-)

From: Jesper Dangaard Brouer <redacted>

This patch prepares code before enabling the clang -target bpf.

The clang compiler does not like #include <stdint.h> when
using '-target bpf' it will fail with:

 fatal error: 'gnu/stubs-32.h' file not found

This is because using clang -target bpf, then clang will have '__bpf__'
defined instead of '__x86_64__' hence the gnu/stubs-32.h include
attempt as /usr/include/gnu/stubs.h contains, on x86_64:

  #if !defined __x86_64__
  # include <gnu/stubs-32.h>
  #endif
  #if defined __x86_64__ && defined __LP64__
  # include <gnu/stubs-64.h>
  #endif
  #if defined __x86_64__ && defined __ILP32__
  # include <gnu/stubs-x32.h>
  #endif

This can be worked around by installing the 32-bit version of
glibc-devel.i686 on your distribution.

But the BPF programs does not really need to include stdint.h,
if converting:
  uint64_t -> __u64
  uint32_t -> __u32
  uint16_t -> __u16
  uint8_t  -> __u8

This patch does this type syntax conversion.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 ebpf/bypass_filter.c |   27 +++++++++++++--------------
 ebpf/filter.c        |    3 +--
 ebpf/hash_func01.h   |   12 ++++++------
 ebpf/lb.c            |   11 +++++------
 ebpf/vlan_filter.c   |    5 ++---
 ebpf/xdp_filter.c    |   42 ++++++++++++++++++++----------------------
 6 files changed, 47 insertions(+), 53 deletions(-)

diff --git a/ebpf/bypass_filter.c b/ebpf/bypass_filter.c
index d2ce12aa1cd5..be81032b16cf 100644
--- a/ebpf/bypass_filter.c
+++ b/ebpf/bypass_filter.c

@@ -15,7 +15,6 @@
  * 02110-1301, USA.
  */
 
-#include <stdint.h>
 #include <stddef.h>
 #include <linux/bpf.h>
 

@@ -51,9 +50,9 @@ struct flowv6_keys {
 } __attribute__((__aligned__(8)));
 
 struct pair {
-    uint64_t time;
-    uint64_t packets;
-    uint64_t bytes;
+    __u64 time;
+    __u64 packets;
+    __u64 bytes;
 } __attribute__((__aligned__(8)));
 
 struct bpf_map_def SEC("maps") flow_table_v4 = {

@@ -77,10 +76,10 @@ struct bpf_map_def SEC("maps") flow_table_v6 = {
  */
 static __always_inline int ipv4_filter(struct __sk_buff *skb)
 {
-    uint32_t nhoff, verlen;
+    __u32 nhoff, verlen;
     struct flowv4_keys tuple;
     struct pair *value;
-    uint16_t port;
+    __u16 port;
 
     nhoff = skb->cb[0];
 

@@ -107,8 +106,8 @@ static __always_inline int ipv4_filter(struct __sk_buff *skb)
 #if 0
     if ((tuple.port16[0] == 22) || (tuple.port16[1] == 22))
     {
-        uint16_t sp = tuple.port16[0];
-        //uint16_t dp = tuple.port16[1];
+        __u16 sp = tuple.port16[0];
+        //__u16 dp = tuple.port16[1];
         char fmt[] = "Parsed SSH flow: %u %d -> %u\n";
         bpf_trace_printk(fmt, sizeof(fmt), tuple.src, sp, tuple.dst);
     }

@@ -118,8 +117,8 @@ static __always_inline int ipv4_filter(struct __sk_buff *skb)
     if (value) {
 #if 0
         {
-            uint16_t sp = tuple.port16[0];
-            //uint16_t dp = tuple.port16[1];
+            __u16 sp = tuple.port16[0];
+            //__u16 dp = tuple.port16[1];
             char bfmt[] = "Found flow: %u %d -> %u\n";
             bpf_trace_printk(bfmt, sizeof(bfmt), tuple.src, sp, tuple.dst);
         }

@@ -139,11 +138,11 @@ static __always_inline int ipv4_filter(struct __sk_buff *skb)
  */
 static __always_inline int ipv6_filter(struct __sk_buff *skb)
 {
-    uint32_t nhoff;
-    uint8_t nhdr;
+    __u32 nhoff;
+    __u8 nhdr;
     struct flowv6_keys tuple;
     struct pair *value;
-    uint16_t port;
+    __u16 port;
 
     nhoff = skb->cb[0];
 

@@ -223,4 +222,4 @@ int SEC("filter") hashfilter(struct __sk_buff *skb) {
 
 char __license[] SEC("license") = "GPL";
 
-uint32_t __version SEC("version") = LINUX_VERSION_CODE;
+__u32 __version SEC("version") = LINUX_VERSION_CODE;

diff --git a/ebpf/filter.c b/ebpf/filter.c
index 1976ffcf188f..4fe95d4fb005 100644
--- a/ebpf/filter.c
+++ b/ebpf/filter.c

@@ -15,7 +15,6 @@
  * 02110-1301, USA.
  */
 
-#include <stdint.h>
 #include <stddef.h>
 #include <linux/bpf.h>
 

@@ -56,4 +55,4 @@ int SEC("filter") hashfilter(struct __sk_buff *skb) {
 
 char __license[] SEC("license") = "GPL";
 
-uint32_t __version SEC("version") = LINUX_VERSION_CODE;
+__u32 __version SEC("version") = LINUX_VERSION_CODE;

diff --git a/ebpf/hash_func01.h b/ebpf/hash_func01.h
index 060346f67a6a..38255812e376 100644
--- a/ebpf/hash_func01.h
+++ b/ebpf/hash_func01.h

@@ -4,12 +4,12 @@
  * From: http://www.azillionmonkeys.com/qed/hash.html
  */
 
-#define get16bits(d) (*((const uint16_t *) (d)))
+#define get16bits(d) (*((const __u16 *) (d)))
 
 static __always_inline
-uint32_t SuperFastHash (const char *data, int len, uint32_t initval) {
-	uint32_t hash = initval;
-	uint32_t tmp;
+__u32 SuperFastHash (const char *data, int len, __u32 initval) {
+	__u32 hash = initval;
+	__u32 tmp;
 	int rem;
 
 	if (len <= 0 || data == NULL) return 0;

@@ -23,7 +23,7 @@ uint32_t SuperFastHash (const char *data, int len, uint32_t initval) {
 		hash  += get16bits (data);
 		tmp    = (get16bits (data+2) << 11) ^ hash;
 		hash   = (hash << 16) ^ tmp;
-		data  += 2*sizeof (uint16_t);
+		data  += 2*sizeof (__u16);
 		hash  += hash >> 11;
 	}
 

@@ -31,7 +31,7 @@ uint32_t SuperFastHash (const char *data, int len, uint32_t initval) {
 	switch (rem) {
         case 3: hash += get16bits (data);
                 hash ^= hash << 16;
-                hash ^= ((signed char)data[sizeof (uint16_t)]) << 18;
+                hash ^= ((signed char)data[sizeof (__u16)]) << 18;
                 hash += hash >> 11;
                 break;
         case 2: hash += get16bits (data);

diff --git a/ebpf/lb.c b/ebpf/lb.c
index 14974784d925..7551781c5f80 100644
--- a/ebpf/lb.c
+++ b/ebpf/lb.c

@@ -15,7 +15,6 @@
  * 02110-1301, USA.
  */
 
-#include <stdint.h>
 #include <stddef.h>
 #include <linux/bpf.h>
 

@@ -36,8 +35,8 @@
 
 static __always_inline int ipv4_hash(struct __sk_buff *skb)
 {
-    uint32_t nhoff;
-    uint32_t src, dst;
+    __u32 nhoff;
+    __u32 src, dst;
 
     nhoff = skb->cb[0];
     src = load_word(skb, nhoff + offsetof(struct iphdr, saddr));

@@ -54,8 +53,8 @@ static __always_inline int ipv4_hash(struct __sk_buff *skb)
 
 static __always_inline int ipv6_hash(struct __sk_buff *skb)
 {
-    uint32_t nhoff;
-    uint32_t src, dst, hash;
+    __u32 nhoff;
+    __u32 src, dst, hash;
 
     nhoff = skb->cb[0];
     hash = 0;

@@ -107,4 +106,4 @@ char __license[] __section("license") = "GPL";
 
 /* libbpf needs version section to check sync of eBPF code and kernel
  * but socket filter don't need it */
-uint32_t __version __section("version") = LINUX_VERSION_CODE;
+__u32 __version __section("version") = LINUX_VERSION_CODE;

diff --git a/ebpf/vlan_filter.c b/ebpf/vlan_filter.c
index 5c3865273d3c..d797b94bfbd5 100644
--- a/ebpf/vlan_filter.c
+++ b/ebpf/vlan_filter.c

@@ -15,7 +15,6 @@
  * 02110-1301, USA.
  */
 
-#include <stdint.h>
 #include <stddef.h>
 #include <linux/bpf.h>
 

@@ -24,7 +23,7 @@
 #define LINUX_VERSION_CODE 263682
 
 int SEC("filter") hashfilter(struct __sk_buff *skb) {
-    uint16_t vlan_id = skb->vlan_tci & 0x0fff;
+    __u16 vlan_id = skb->vlan_tci & 0x0fff;
     /* accept VLAN 2 and 4 and drop the rest */
     switch (vlan_id) {
         case 2:

@@ -38,4 +37,4 @@ int SEC("filter") hashfilter(struct __sk_buff *skb) {
 
 char __license[] SEC("license") = "GPL";
 
-uint32_t __version SEC("version") = LINUX_VERSION_CODE;
+__u32 __version SEC("version") = LINUX_VERSION_CODE;

diff --git a/ebpf/xdp_filter.c b/ebpf/xdp_filter.c
index 9d3ff0910c30..3ee9dd0b114c 100644
--- a/ebpf/xdp_filter.c
+++ b/ebpf/xdp_filter.c

@@ -16,8 +16,6 @@
  */
 
 #define KBUILD_MODNAME "foo"
-#include <stdint.h>
-#include <string.h>
 #include <stddef.h>
 #include <linux/bpf.h>
 

@@ -70,9 +68,9 @@ struct flowv6_keys {
 } __attribute__((__aligned__(8)));
 
 struct pair {
-    uint64_t time;
-    uint64_t packets;
-    uint64_t bytes;
+    __u64 time;
+    __u64 packets;
+    __u64 bytes;
 } __attribute__((__aligned__(8)));
 
 struct bpf_map_def SEC("maps") flow_table_v4 = {

@@ -128,7 +126,7 @@ struct bpf_map_def SEC("maps") tx_peer_int = {
 };
 
 static __always_inline int get_sport(void *trans_data, void *data_end,
-        uint8_t protocol)
+        __u8 protocol)
 {
     struct tcphdr *th;
     struct udphdr *uh;

@@ -150,7 +148,7 @@ static __always_inline int get_sport(void *trans_data, void *data_end,
 }
 
 static __always_inline int get_dport(void *trans_data, void *data_end,
-        uint8_t protocol)
+        __u8 protocol)
 {
     struct tcphdr *th;
     struct udphdr *uh;

@@ -178,12 +176,12 @@ static int __always_inline filter_ipv4(void *data, __u64 nh_off, void *data_end)
     int sport;
     struct flowv4_keys tuple;
     struct pair *value;
-    uint32_t key0 = 0;
+    __u32 key0 = 0;
 #if BUILD_CPUMAP
-    uint32_t cpu_dest;
-    uint32_t *cpu_max = bpf_map_lookup_elem(&cpus_count, &key0);
-    uint32_t *cpu_selected;
-    uint32_t cpu_hash;
+    __u32 cpu_dest;
+    __u32 *cpu_max = bpf_map_lookup_elem(&cpus_count, &key0);
+    __u32 *cpu_selected;
+    __u32 cpu_hash;
 #endif
     int *iface_peer;
     int tx_port = 0;

@@ -191,7 +189,7 @@ static int __always_inline filter_ipv4(void *data, __u64 nh_off, void *data_end)
     if ((void *)(iph + 1) > data_end)
         return XDP_PASS;
 
-    tuple.ip_proto = (uint32_t) iph->protocol;
+    tuple.ip_proto = (__u32) iph->protocol;
     tuple.src = iph->saddr;
     tuple.dst = iph->daddr;
 

@@ -203,8 +201,8 @@ static int __always_inline filter_ipv4(void *data, __u64 nh_off, void *data_end)
     if (sport == -1)
         return XDP_PASS;
 
-    tuple.port16[0] = (uint16_t)sport;
-    tuple.port16[1] = (uint16_t)dport;
+    tuple.port16[0] = (__u16)sport;
+    tuple.port16[1] = (__u16)dport;
     value = bpf_map_lookup_elem(&flow_table_v4, &tuple);
 #if 0
     {

@@ -260,12 +258,12 @@ static int __always_inline filter_ipv6(void *data, __u64 nh_off, void *data_end)
     int sport;
     struct flowv6_keys tuple;
     struct pair *value;
-    uint32_t key0 = 0;
+    __u32 key0 = 0;
 #if BUILD_CPUMAP
-    uint32_t cpu_dest;
+    __u32 cpu_dest;
     int *cpu_max = bpf_map_lookup_elem(&cpus_count, &key0);
-    uint32_t *cpu_selected;
-    uint32_t cpu_hash;
+    __u32 *cpu_selected;
+    __u32 cpu_hash;
 #endif
     int tx_port = 0;
     int *iface_peer;

@@ -336,8 +334,8 @@ int SEC("xdp") xdp_hashfilter(struct xdp_md *ctx)
     void *data = (void *)(long)ctx->data;
     struct ethhdr *eth = data;
     int rc = XDP_PASS;
-    uint16_t h_proto;
-	uint64_t nh_off;
+    __u16 h_proto;
+    __u64 nh_off;
 
 	nh_off = sizeof(*eth);
 	if (data + nh_off > data_end)

@@ -376,4 +374,4 @@ int SEC("xdp") xdp_hashfilter(struct xdp_md *ctx)
 
 char __license[] SEC("license") = "GPL";
 
-uint32_t __version SEC("version") = LINUX_VERSION_CODE;
+__u32 __version SEC("version") = LINUX_VERSION_CODE;

From: Jesper Dangaard Brouer <redacted>

Enable compiling eBPF programs with clang -target bpf.

This is mostly to workaround a bug in libbpf, where clang > ver 4.0.0
generates some ELF sections (.eh_frame) when -target bpf is NOT specified,
and libbpf fails loading such files.

Notice libbpf is provided by the kernel, and in kernel v4.16 the library
will contain the needed function for attatching to the XDP hook.

Kernel commit 949abbe88436 ("libbpf: add function to setup XDP")
 https://git.kernel.org/torvalds/c/949abbe88436

As it looks now, the library fix will not get into kernel v4.16. Thus, we
need this workaround for Suricata.  In-order to recommend people installing
the library libbpf from kernel v4.16.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 ebpf/Makefile.am |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ebpf/Makefile.am b/ebpf/Makefile.am
index 66db35f80c7e..d0ee44ae668e 100644
--- a/ebpf/Makefile.am
+++ b/ebpf/Makefile.am

@@ -6,7 +6,7 @@ BPF_CFLAGS = -Iinclude
 all: lb.bpf filter.bpf bypass_filter.bpf xdp_filter.bpf vlan_filter.bpf
 
 %.bpf: %.c
-	${CC} -Wall $(BPF_CFLAGS) -O2 -D__KERNEL__ -D__ASM_SYSREG_H -emit-llvm -c $< -o - | ${LLC} -march=bpf -filetype=obj -o $@
+	${CC} -Wall $(BPF_CFLAGS) -O2 -D__KERNEL__ -D__ASM_SYSREG_H -target bpf -emit-llvm -c $< -o - | ${LLC} -march=bpf -filetype=obj -o $@
 
 CLEANFILES = *.bpf
 

From: Jesper Dangaard Brouer <redacted>

The current ebpf/Makefile.am have the problem that clang compile
errors still result in an ELF .bpf output file.  This is obviously
problematic as the the problem is first seen runtime when loading
the bpf-prog.  This this is cause by the uses of a pipe from
clang to llc.

To address this problem, split up the clang and llc invocations
up into two separate commands, to get proper reaction based on
the compiler exit code. The clang compiler is used as a
frontend (+ optimizer) and instructed (via -S -emit-llvm) to
generate LLVM IR (Intermediate Representation) with suffix .ll.
The LLVM llc command is used as a compiler backend taking IR and
producing BPF machine bytecode, and storing this into a ELF
object.  In the last step the IR .ll suffix code it removed.

The official documentation of the IR language:
 http://llvm.org/docs/LangRef.html

Also fix the previous make portability warning:
 '%-style pattern rules are a GNU make extension'
I instead use some static pattern rules:
 https://www.gnu.org/software/make/manual/html_node/Static-Usage.html

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 ebpf/Makefile.am |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/ebpf/Makefile.am b/ebpf/Makefile.am
index d0ee44ae668e..89a3304e953b 100644
--- a/ebpf/Makefile.am
+++ b/ebpf/Makefile.am

@@ -3,11 +3,25 @@ if BUILD_EBPF
 # Maintaining a local copy of UAPI linux/bpf.h
 BPF_CFLAGS = -Iinclude
 
-all: lb.bpf filter.bpf bypass_filter.bpf xdp_filter.bpf vlan_filter.bpf
+CLANG = ${CC}
 
-%.bpf: %.c
-	${CC} -Wall $(BPF_CFLAGS) -O2 -D__KERNEL__ -D__ASM_SYSREG_H -target bpf -emit-llvm -c $< -o - | ${LLC} -march=bpf -filetype=obj -o $@
+BPF_TARGETS  = lb.bpf
+BPF_TARGETS += filter.bpf
+BPF_TARGETS += bypass_filter.bpf
+BPF_TARGETS += xdp_filter.bpf
+BPF_TARGETS += vlan_filter.bpf
 
-CLEANFILES = *.bpf
+all: $(BPF_TARGETS)
+
+$(BPF_TARGETS): %.bpf: %.c
+#      From C-code to LLVM-IR format suffix .ll (clang -S -emit-llvm)
+	${CLANG} -Wall $(BPF_CFLAGS) -O2 \
+		-D__KERNEL__ -D__ASM_SYSREG_H \
+		-target bpf -S -emit-llvm $< -o ${@:.bpf=.ll}
+#      From LLVM-IR to BPF-bytecode in ELF-obj file
+	${LLC} -march=bpf -filetype=obj ${@:.bpf=.ll} -o $@
+	${RM} ${@:.bpf=.ll}
+
+CLEANFILES = *.bpf *.ll
 
 endif

Hello Jesper,

On Wed, 2018-02-07 at 23:21 +0100, Jesper Dangaard Brouer wrote:

Hi Eric,

I've improved the Suricata ebpf makefile, in-order to avoid
generating
the .eh_frame sections.  This required changing the code a bit, to
allow using clang -target bpf.

The makefile have also been improved to stop on clang compile errors,
instead of generating an almost empty BPF ELF file.

Could I ask you to get these changes into Suricata, through correct
process for this Open Source project?

Sure, I'm reviewing the code, testing it and I will do a Pull Request
on github.

Thanks a lot for that, that's a really valuable help!

BR,
-- 
Eric Leblond [off-list ref]

Hi,

On Wed, 2018-02-07 at 23:21 +0100, Jesper Dangaard Brouer wrote:

From: Jesper Dangaard Brouer <redacted>

This patch prepares code before enabling the clang -target bpf.

The clang compiler does not like #include <stdint.h> when
using '-target bpf' it will fail with:

 fatal error: 'gnu/stubs-32.h' file not found

...

This can be worked around by installing the 32-bit version of
glibc-devel.i686 on your distribution.

But the BPF programs does not really need to include stdint.h,
if converting:
  uint64_t -> __u64
  uint32_t -> __u32
  uint16_t -> __u16
  uint8_t  -> __u8

This patch does this type syntax conversion.

There is an issue for system like Debian because they don't have a
asm/types.h in the include path if the architecture is not defined
which is the case due to target bpf. This results in:

clang-5.0 -Wall -Iinclude -O2 \
	-D__KERNEL__ -D__ASM_SYSREG_H \
	-target bpf -S -emit-llvm vlan_filter.c -o vlan_filter.ll
In file included from vlan_filter.c:19:
In file included from include/linux/bpf.h:11:
/usr/include/linux/types.h:5:10: fatal error: 'asm/types.h' file not
found
#include <asm/types.h>
         ^~~~~~~~~~~~~
1 error generated.
Makefile:523: recipe for target 'vlan_filter.bpf' failed

To go into details, the Debian package providing the 'asm/typs.h'
include is the the headers or linux-libc-dev. But this package comes
with a flavor and thus we have a prefix: 
 linux-libc-dev:amd64: /usr/include/x86_64-linux-gnu/asm/types.h

"Fun" part here is that if you build a debian package of the via make
in Linux tree then the linux-libc-dev package is correct.

So I propose the following patch that fixes the issue for me:

diff --git a/ebpf/Makefile.am b/ebpf/Makefile.am
index 89a3304e9..712b05343 100644
--- a/ebpf/Makefile.am
+++ b/ebpf/Makefile.am

@@ -16,6 +16,7 @@ all: $(BPF_TARGETS)
 $(BPF_TARGETS): %.bpf: %.c
 #      From C-code to LLVM-IR format suffix .ll (clang -S -emit-llvm)
        ${CLANG} -Wall $(BPF_CFLAGS) -O2 \
+               -I/usr/include/$(host_cpu)-$(host_os)/ \
                -D__KERNEL__ -D__ASM_SYSREG_H \
                -target bpf -S -emit-llvm $< -o ${@:.bpf=.ll}
 #      From LLVM-IR to BPF-bytecode in ELF-obj file

Let me know if it is ok for you.

Best regards,
-- 
Eric Leblond [off-list ref]

On Thu, 08 Feb 2018 00:52:09 +0100 Eric Leblond [off-list ref] wrote:

Hi,

On Wed, 2018-02-07 at 23:21 +0100, Jesper Dangaard Brouer wrote:

quoted

From: Jesper Dangaard Brouer <redacted>

This patch prepares code before enabling the clang -target bpf.

The clang compiler does not like #include <stdint.h> when
using '-target bpf' it will fail with:

 fatal error: 'gnu/stubs-32.h' file not found  

...

quoted

This can be worked around by installing the 32-bit version of
glibc-devel.i686 on your distribution.

But the BPF programs does not really need to include stdint.h,
if converting:
  uint64_t -> __u64
  uint32_t -> __u32
  uint16_t -> __u16
  uint8_t  -> __u8

This patch does this type syntax conversion.  

There is an issue for system like Debian because they don't have a
asm/types.h in the include path if the architecture is not defined
which is the case due to target bpf. This results in:

clang-5.0 -Wall -Iinclude -O2 \
	-D__KERNEL__ -D__ASM_SYSREG_H \
	-target bpf -S -emit-llvm vlan_filter.c -o vlan_filter.ll
In file included from vlan_filter.c:19:
In file included from include/linux/bpf.h:11:
/usr/include/linux/types.h:5:10: fatal error: 'asm/types.h' file not
found
#include <asm/types.h>
         ^~~~~~~~~~~~~
1 error generated.
Makefile:523: recipe for target 'vlan_filter.bpf' failed

To go into details, the Debian package providing the 'asm/typs.h'
include is the the headers or linux-libc-dev. But this package comes
with a flavor and thus we have a prefix: 
 linux-libc-dev:amd64: /usr/include/x86_64-linux-gnu/asm/types.h

Oh, the joy of distro choices.

Cool solution. These variables originate from configure/automake.

Would it be more technical correct to use(?): $(build_cpu)-$(build_os)

I verified that the variables are the same (notice 'make -p' trick):

$ make -p | egrep '_os'
build_os = linux-gnu
host_os = linux-gnu

$ make -p | egrep '_cpu'
host_cpu = x86_64
build_cpu = x86_64

                -D__KERNEL__ -D__ASM_SYSREG_H \
                -target bpf -S -emit-llvm $< -o ${@:.bpf=.ll}
 #      From LLVM-IR to BPF-bytecode in ELF-obj file

Let me know if it is ok for you.

I'm fine with this fix.

I wonder if we should check other distros?

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