[net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[Jesper Dangaard Brouer]

This series improves and fixes bpf ELF loader and programs under
samples/bpf.  The bpf_load.c created some hard to debug issues when
the struct (bpf_map_def) used in the ELF maps section format changed
in commit fb30d4b71214 ("bpf: Add tests for map-in-map").

This was hotfixed in commit 409526bea3c3 ("samples/bpf: bpf_load.c
detect and abort if ELF maps section size is wrong") by detecting the
issue and aborting the program.

In most situations the bpf-loader should be able to handle these kind
of changes to the struct size.  This patch series aim to do proper
backward and forward compabilility handling when loading ELF files.

This series also adjust the callback that was introduced in commit
9fd63d05f3e8 ("bpf: Allow bpf sample programs (*_user.c) to change
bpf_map_def") to use the new bpf_map_data structure, before more users
start to use this callback.

Hoping these changes can make the merge window, as above mentioned
commits have not been merged yet, and it would be good to avoid users
hitting these issues.

---

Jesper Dangaard Brouer (4):
      samples/bpf: adjust rlimit RLIMIT_MEMLOCK for traceex2, tracex3 and tracex4
      samples/bpf: make bpf_load.c code compatible with ELF maps section changes
      samples/bpf: load_bpf.c make callback fixup more flexible
      samples/bpf: export map_data[] for more info on maps


 samples/bpf/bpf_load.c           |  229 ++++++++++++++++++++++++++------------
 samples/bpf/bpf_load.h           |   18 ++-
 samples/bpf/map_perf_test_user.c |   14 +-
 samples/bpf/tracex2_user.c       |    7 +
 samples/bpf/tracex3_user.c       |    7 +
 samples/bpf/tracex4_user.c       |    8 +
 6 files changed, 201 insertions(+), 82 deletions(-)

[net-next PATCH 1/4] samples/bpf: adjust rlimit RLIMIT_MEMLOCK for traceex2, tracex3 and tracex4

[Jesper Dangaard Brouer]

Needed to adjust max locked memory RLIMIT_MEMLOCK for testing these bpf samples
as these are using more and larger maps than can fit in distro default 64Kbytes limit.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 samples/bpf/tracex2_user.c |    7 +++++++
 samples/bpf/tracex3_user.c |    7 +++++++
 samples/bpf/tracex4_user.c |    8 ++++++++
 3 files changed, 22 insertions(+)

diff --git a/samples/bpf/tracex2_user.c b/samples/bpf/tracex2_user.c
index ded9804c5034..7fee0f1ba9a3 100644
--- a/samples/bpf/tracex2_user.c
+++ b/samples/bpf/tracex2_user.c
@@ -4,6 +4,7 @@
 #include <signal.h>
 #include <linux/bpf.h>
 #include <string.h>
+#include <sys/resource.h>
 
 #include "libbpf.h"
 #include "bpf_load.h"
@@ -112,6 +113,7 @@ static void int_exit(int sig)
 
 int main(int ac, char **argv)
 {
+	struct rlimit r = {1024*1024, RLIM_INFINITY};
 	char filename[256];
 	long key, next_key, value;
 	FILE *f;
@@ -119,6 +121,11 @@ int main(int ac, char **argv)
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 
+	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
+		perror("setrlimit(RLIMIT_MEMLOCK)");
+		return 1;
+	}
+
 	signal(SIGINT, int_exit);
 
 	/* start 'ping' in the background to have some kfree_skb events */
diff --git a/samples/bpf/tracex3_user.c b/samples/bpf/tracex3_user.c
index 8f7d199d5945..fe372239d505 100644
--- a/samples/bpf/tracex3_user.c
+++ b/samples/bpf/tracex3_user.c
@@ -11,6 +11,7 @@
 #include <stdbool.h>
 #include <string.h>
 #include <linux/bpf.h>
+#include <sys/resource.h>
 
 #include "libbpf.h"
 #include "bpf_load.h"
@@ -112,11 +113,17 @@ static void print_hist(int fd)
 
 int main(int ac, char **argv)
 {
+	struct rlimit r = {1024*1024, RLIM_INFINITY};
 	char filename[256];
 	int i;
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 
+	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
+		perror("setrlimit(RLIMIT_MEMLOCK)");
+		return 1;
+	}
+
 	if (load_bpf_file(filename)) {
 		printf("%s", bpf_log_buf);
 		return 1;
diff --git a/samples/bpf/tracex4_user.c b/samples/bpf/tracex4_user.c
index 03449f773cb1..22c644f1f4c3 100644
--- a/samples/bpf/tracex4_user.c
+++ b/samples/bpf/tracex4_user.c
@@ -12,6 +12,8 @@
 #include <string.h>
 #include <time.h>
 #include <linux/bpf.h>
+#include <sys/resource.h>
+
 #include "libbpf.h"
 #include "bpf_load.h"
 
@@ -50,11 +52,17 @@ static void print_old_objects(int fd)
 
 int main(int ac, char **argv)
 {
+	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
 	char filename[256];
 	int i;
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 
+	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
+		perror("setrlimit(RLIMIT_MEMLOCK, RLIM_INFINITY)");
+		return 1;
+	}
+
 	if (load_bpf_file(filename)) {
 		printf("%s", bpf_log_buf);
 		return 1;

Re: [net-next PATCH 1/4] samples/bpf: adjust rlimit RLIMIT_MEMLOCK for traceex2, tracex3 and tracex4

[Alexei Starovoitov]

On Tue, May 02, 2017 at 02:31:50PM +0200, Jesper Dangaard Brouer wrote:
> Needed to adjust max locked memory RLIMIT_MEMLOCK for testing these bpf samples
> as these are using more and larger maps than can fit in distro default 64Kbytes limit.
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
...
> +	struct rlimit r = {1024*1024, RLIM_INFINITY};
...
> +	struct rlimit r = {1024*1024, RLIM_INFINITY};

why magic numbers?
All other samples do
struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
 
> +	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
> +		perror("setrlimit(RLIMIT_MEMLOCK)");

ip_tunnel.c test does:
perror("setrlimit(RLIMIT_MEMLOCK, RLIM_INFINITY)");
Few others do:
assert(!setrlimit(RLIMIT_MEMLOCK, &r));
and the rest just:
setrlimit(RLIMIT_MEMLOCK, &r);

We probalby need to move this to a helper.

> +	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};

here it's consistent :)

> +	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
> +		perror("setrlimit(RLIMIT_MEMLOCK, RLIM_INFINITY)");

but with different perror ?
Let's do a common helper for all?

Re: [net-next PATCH 1/4] samples/bpf: adjust rlimit RLIMIT_MEMLOCK for traceex2, tracex3 and tracex4

[Jesper Dangaard Brouer]

On Tue, 2 May 2017 17:53:16 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Tue, May 02, 2017 at 02:31:50PM +0200, Jesper Dangaard Brouer wrote:
> > Needed to adjust max locked memory RLIMIT_MEMLOCK for testing these bpf samples
> > as these are using more and larger maps than can fit in distro default 64Kbytes limit.
> > 
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>  
> ...
> > +	struct rlimit r = {1024*1024, RLIM_INFINITY};  
> ...
> > +	struct rlimit r = {1024*1024, RLIM_INFINITY};  
> 
> why magic numbers?
> All other samples do
> struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};

I just wanted to provide some examples showing that it is possible to
set some reasonable limit.

The RLIM_INFINITY setting is basically just disabling the kernels
memory limit checks, and it is sort of a bad coding pattern (that
people will copy) as the two example programs does not need much.

 
> > +	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
> > +		perror("setrlimit(RLIMIT_MEMLOCK)");  
> 
> ip_tunnel.c test does:
> perror("setrlimit(RLIMIT_MEMLOCK, RLIM_INFINITY)");
> Few others do:
> assert(!setrlimit(RLIMIT_MEMLOCK, &r));
> and the rest just:
> setrlimit(RLIMIT_MEMLOCK, &r);
> 
> We probalby need to move this to a helper.
> 
> > +	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};  
> 
> here it's consistent :)
> 
> > +	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
> > +		perror("setrlimit(RLIMIT_MEMLOCK, RLIM_INFINITY)");  
> 
> but with different perror ?
> Let's do a common helper for all?

Sure, it makes sense to streamline this into a helper, just not in this
patchset ;-)  Lets do that later...

And I would argue that this helper should allow users to specify some
expected/reasonable memory usage size, as the kernel side checks would
then provide some value, instead of being effectively disabled.  I can
easily imagine someone increasing a _kern.c hash map max size to
100 million, without realizing that this can OOM the machine.

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

Re: [net-next PATCH 1/4] samples/bpf: adjust rlimit RLIMIT_MEMLOCK for traceex2, tracex3 and tracex4

[David Miller]

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 2 May 2017 17:53:16 -0700

> On Tue, May 02, 2017 at 02:31:50PM +0200, Jesper Dangaard Brouer wrote:
>> Needed to adjust max locked memory RLIMIT_MEMLOCK for testing these bpf samples
>> as these are using more and larger maps than can fit in distro default 64Kbytes limit.
>> 
>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ...
>> +	struct rlimit r = {1024*1024, RLIM_INFINITY};
> ...
>> +	struct rlimit r = {1024*1024, RLIM_INFINITY};
> 
> why magic numbers?
> All other samples do
> struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};

Let's not do that.

People run these tests often as root, so the safer we make running
these test the better.

A weird magic limit is better than none at all.

[net-next PATCH 2/4] samples/bpf: make bpf_load.c code compatible with ELF maps section changes

[Jesper Dangaard Brouer]

This patch does proper parsing of the ELF "maps" section, in-order to
be both backwards and forwards compatible with changes to the map
definition struct bpf_map_def, which gets compiled into the ELF file.

The assumption is that new features with value zero, means that they
are not in-use.  For backward compatibility where loading an ELF file
with a smaller struct bpf_map_def, only copy objects ELF size, leaving
rest of loaders struct zero.  For forward compatibility where ELF file
have a larger struct bpf_map_def, only copy loaders own struct size
and verify that rest of the larger struct is zero, assuming this means
the newer feature was not activated, thus it should be safe for this
older loader to load this newer ELF file.

Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
Fixes: 409526bea3c3 ("samples/bpf: bpf_load.c detect and abort if ELF maps section size is wrong")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 samples/bpf/bpf_load.c |  224 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 155 insertions(+), 69 deletions(-)

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 4221dc359453..fedec29c7817 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -39,6 +39,16 @@ int event_fd[MAX_PROGS];
 int prog_cnt;
 int prog_array_fd = -1;
 
+/* Keeping relevant info on maps */
+struct bpf_map_data {
+	int fd;
+	char *name;
+	size_t elf_offset;
+	struct bpf_map_def def;
+};
+struct bpf_map_data map_data[MAX_MAPS];
+int map_data_count = 0;
+
 static int populate_prog_array(const char *event, int prog_fd)
 {
 	int ind = atoi(event), err;
@@ -186,42 +196,39 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 	return 0;
 }
 
-static int load_maps(struct bpf_map_def *maps, int nr_maps,
-		     const char **map_names, fixup_map_cb fixup_map)
+static int load_maps(struct bpf_map_data *maps, int nr_maps,
+		     fixup_map_cb fixup_map)
 {
 	int i;
-	/*
-	 * Warning: Using "maps" pointing to ELF data_maps->d_buf as
-	 * an array of struct bpf_map_def is a wrong assumption about
-	 * the ELF maps section format.
-	 */
+
 	for (i = 0; i < nr_maps; i++) {
 		if (fixup_map)
-			fixup_map(&maps[i], map_names[i], i);
+			fixup_map(&maps[i].def, maps[i].name, i);
 
-		if (maps[i].type == BPF_MAP_TYPE_ARRAY_OF_MAPS ||
-		    maps[i].type == BPF_MAP_TYPE_HASH_OF_MAPS) {
-			int inner_map_fd = map_fd[maps[i].inner_map_idx];
+		if (maps[i].def.type == BPF_MAP_TYPE_ARRAY_OF_MAPS ||
+		    maps[i].def.type == BPF_MAP_TYPE_HASH_OF_MAPS) {
+			int inner_map_fd = map_fd[maps[i].def.inner_map_idx];
 
-			map_fd[i] = bpf_create_map_in_map(maps[i].type,
-							  maps[i].key_size,
-							  inner_map_fd,
-							  maps[i].max_entries,
-							  maps[i].map_flags);
+			map_fd[i] = bpf_create_map_in_map(maps[i].def.type,
+							maps[i].def.key_size,
+							inner_map_fd,
+							maps[i].def.max_entries,
+							maps[i].def.map_flags);
 		} else {
-			map_fd[i] = bpf_create_map(maps[i].type,
-						   maps[i].key_size,
-						   maps[i].value_size,
-						   maps[i].max_entries,
-						   maps[i].map_flags);
+			map_fd[i] = bpf_create_map(maps[i].def.type,
+						   maps[i].def.key_size,
+						   maps[i].def.value_size,
+						   maps[i].def.max_entries,
+						   maps[i].def.map_flags);
 		}
 		if (map_fd[i] < 0) {
 			printf("failed to create a map: %d %s\n",
 			       errno, strerror(errno));
 			return 1;
 		}
+		maps[i].fd = map_fd[i];
 
-		if (maps[i].type == BPF_MAP_TYPE_PROG_ARRAY)
+		if (maps[i].def.type == BPF_MAP_TYPE_PROG_ARRAY)
 			prog_array_fd = map_fd[i];
 	}
 	return 0;
@@ -251,7 +258,8 @@ static int get_sec(Elf *elf, int i, GElf_Ehdr *ehdr, char **shname,
 }
 
 static int parse_relo_and_apply(Elf_Data *data, Elf_Data *symbols,
-				GElf_Shdr *shdr, struct bpf_insn *insn)
+				GElf_Shdr *shdr, struct bpf_insn *insn,
+				struct bpf_map_data *maps, int nr_maps)
 {
 	int i, nrels;
 
@@ -261,6 +269,8 @@ static int parse_relo_and_apply(Elf_Data *data, Elf_Data *symbols,
 		GElf_Sym sym;
 		GElf_Rel rel;
 		unsigned int insn_idx;
+		bool match = false;
+		int j, map_idx;
 
 		gelf_getrel(data, i, &rel);
 
@@ -274,11 +284,21 @@ static int parse_relo_and_apply(Elf_Data *data, Elf_Data *symbols,
 			return 1;
 		}
 		insn[insn_idx].src_reg = BPF_PSEUDO_MAP_FD;
-		/*
-		 * Warning: Using sizeof(struct bpf_map_def) here is a
-		 * wrong assumption about ELF maps section format
-		 */
-		insn[insn_idx].imm = map_fd[sym.st_value / sizeof(struct bpf_map_def)];
+
+		/* Match FD relocation against recorded map_data[] offset */
+		for (map_idx = 0; map_idx < nr_maps; map_idx++) {
+			if (maps[map_idx].elf_offset == sym.st_value) {
+				match = true;
+				break;
+			}
+		}
+		if (match) {
+			insn[insn_idx].imm = maps[map_idx].fd;
+		} else {
+			printf("invalid relo for insn[%d] no map_data match\n",
+			       insn_idx);
+			return 1;
+		}
 	}
 
 	return 0;
@@ -297,40 +317,112 @@ static int cmp_symbols(const void *l, const void *r)
 		return 0;
 }
 
-static int get_sorted_map_names(Elf *elf, Elf_Data *symbols, int maps_shndx,
-				int strtabidx, char **map_names)
+static int load_elf_maps_section(struct bpf_map_data *maps, int maps_shndx,
+				 Elf *elf, Elf_Data *symbols, int strtabidx)
 {
-	GElf_Sym map_symbols[MAX_MAPS];
-	int i, nr_maps = 0;
+	int map_sz_elf, map_sz_copy;
+	bool validate_zero = false;
+	Elf_Data *data_maps;
+	int i, nr_maps;
+	GElf_Sym *sym;
+	Elf_Scn *scn;
+	int copy_sz;
+
+	if (maps_shndx < 0)
+		return -EINVAL;
+	if (!symbols)
+		return -EINVAL;
+
+	/* Get data for maps section via elf index */
+	scn = elf_getscn(elf, maps_shndx);
+	if (scn)
+		data_maps = elf_getdata(scn, NULL);
+	if (!scn || !data_maps) {
+		printf("Failed to get Elf_Data from maps section %d\n",
+		       maps_shndx);
+		return -EINVAL;
+	}
 
-	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
-		assert(nr_maps < MAX_MAPS);
-		if (!gelf_getsym(symbols, i, &map_symbols[nr_maps]))
+	/* For each map get corrosponding symbol table entry */
+	sym = calloc(MAX_MAPS+1, sizeof(GElf_Sym));
+	for (i = 0, nr_maps = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
+		assert(nr_maps < MAX_MAPS+1);
+		if (!gelf_getsym(symbols, i, &sym[nr_maps]))
 			continue;
-		if (map_symbols[nr_maps].st_shndx != maps_shndx)
+		if (sym[nr_maps].st_shndx != maps_shndx)
 			continue;
+		/* Only increment iif maps section */
 		nr_maps++;
 	}
 
-	qsort(map_symbols, nr_maps, sizeof(GElf_Sym), cmp_symbols);
+	/* Align to map_fd[] order, via sort on offset in sym.st_value */
+	qsort(sym, nr_maps, sizeof(GElf_Sym), cmp_symbols);
+
+	/* Keeping compatible with ELF maps section changes
+	 * ------------------------------------------------
+	 * The program size of struct bpf_map_def is known by loader
+	 * code, but struct stored in ELF file can be different.
+	 *
+	 * Unfortunately sym[i].st_size is zero.  To calculate the
+	 * struct size stored in the ELF file, assume all struct have
+	 * the same size, and simply divide with number of map
+	 * symbols.
+	 */
+	map_sz_elf = data_maps->d_size / nr_maps;
+	map_sz_copy = sizeof(struct bpf_map_def);
+	if (map_sz_elf < map_sz_copy) {
+		/*
+		 * Backward compat, loading older ELF file with
+		 * smaller struct, keeping remaining bytes zero.
+		 */
+		map_sz_copy = map_sz_elf;
+	} else if (map_sz_elf > map_sz_copy) {
+		/*
+		 * Forward compat, loading newer ELF file with larger
+		 * struct with unknown features. Assume zero means
+		 * feature not used.  Thus, validate rest of struct
+		 * data is zero.
+		 */
+		validate_zero = true;
+	}
 
+	/* Memcpy relevant part of ELF maps data to loader maps */
 	for (i = 0; i < nr_maps; i++) {
-		char *map_name;
-
-		map_name = elf_strptr(elf, strtabidx, map_symbols[i].st_name);
-		if (!map_name) {
-			printf("cannot get map symbol\n");
-			return -1;
-		}
-
-		map_names[i] = strdup(map_name);
-		if (!map_names[i]) {
+		unsigned char *addr, *end;
+		struct bpf_map_def *def;
+		const char *map_name;
+		size_t offset;
+
+		map_name = elf_strptr(elf, strtabidx, sym[i].st_name);
+		maps[i].name = strdup(map_name);
+		if (!maps[i].name) {
 			printf("strdup(%s): %s(%d)\n", map_name,
 			       strerror(errno), errno);
-			return -1;
+			free(sym);
+			return -errno;
+		}
+
+		/* Symbol value is offset into ELF maps section data area */
+		offset = sym[i].st_value;
+		def = (struct bpf_map_def *)(data_maps->d_buf + offset);
+		maps[i].elf_offset = offset;
+		memset(&maps[i].def, 0, sizeof(struct bpf_map_def));
+		memcpy(&maps[i].def, def, map_sz_copy);
+
+		/* Verify no newer features were requested */
+		if (validate_zero) {
+			addr = (unsigned char*) def + map_sz_copy;
+			end  = (unsigned char*) def + map_sz_elf;
+			for (; addr < end; addr++) {
+				if (*addr != 0) {
+					free(sym);
+					return -EFBIG;
+				}
+			}
 		}
 	}
 
+	free(sym);
 	return nr_maps;
 }
 
@@ -341,7 +433,8 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
 	GElf_Ehdr ehdr;
 	GElf_Shdr shdr, shdr_prog;
 	Elf_Data *data, *data_prog, *data_maps = NULL, *symbols = NULL;
-	char *shname, *shname_prog, *map_names[MAX_MAPS] = { NULL };
+	char *shname, *shname_prog;
+	int nr_maps = 0;
 
 	/* reset global variables */
 	kern_version = 0;
@@ -389,8 +482,12 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
 			}
 			memcpy(&kern_version, data->d_buf, sizeof(int));
 		} else if (strcmp(shname, "maps") == 0) {
+			int j;
+
 			maps_shndx = i;
 			data_maps = data;
+			for (j = 0; j < MAX_MAPS; j++)
+				map_data[j].fd = -1;
 		} else if (shdr.sh_type == SHT_SYMTAB) {
 			strtabidx = shdr.sh_link;
 			symbols = data;
@@ -405,27 +502,17 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
 	}
 
 	if (data_maps) {
-		int nr_maps;
-		int prog_elf_map_sz;
-
-		nr_maps = get_sorted_map_names(elf, symbols, maps_shndx,
-					       strtabidx, map_names);
-		if (nr_maps < 0)
-			goto done;
-
-		/* Deduce map struct size stored in ELF maps section */
-		prog_elf_map_sz = data_maps->d_size / nr_maps;
-		if (prog_elf_map_sz != sizeof(struct bpf_map_def)) {
-			printf("Error: ELF maps sec wrong size (%d/%lu),"
-			       " old kern.o file?\n",
-			       prog_elf_map_sz, sizeof(struct bpf_map_def));
+		nr_maps = load_elf_maps_section(map_data, maps_shndx,
+						elf, symbols, strtabidx);
+		if (nr_maps < 0) {
+			printf("Error: Failed loading ELF maps (errno:%d):%s\n",
+			       nr_maps, strerror(-nr_maps));
 			ret = 1;
 			goto done;
 		}
-
-		if (load_maps(data_maps->d_buf, nr_maps,
-			      (const char **)map_names, fixup_map))
+		if (load_maps(map_data, nr_maps, fixup_map))
 			goto done;
+		map_data_count = nr_maps;
 
 		processed_sec[maps_shndx] = true;
 	}
@@ -453,7 +540,8 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
 			processed_sec[shdr.sh_info] = true;
 			processed_sec[i] = true;
 
-			if (parse_relo_and_apply(data, symbols, &shdr, insns))
+			if (parse_relo_and_apply(data, symbols, &shdr, insns,
+						 map_data, nr_maps))
 				continue;
 
 			if (memcmp(shname_prog, "kprobe/", 7) == 0 ||
@@ -488,8 +576,6 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
 
 	ret = 0;
 done:
-	for (i = 0; i < MAX_MAPS; i++)
-		free(map_names[i]);
 	close(fd);
 	return ret;
 }

Re: [net-next PATCH 2/4] samples/bpf: make bpf_load.c code compatible with ELF maps section changes

[Alexei Starovoitov]

On Tue, May 02, 2017 at 02:31:56PM +0200, Jesper Dangaard Brouer wrote:
> This patch does proper parsing of the ELF "maps" section, in-order to
> be both backwards and forwards compatible with changes to the map
> definition struct bpf_map_def, which gets compiled into the ELF file.
> 
> The assumption is that new features with value zero, means that they
> are not in-use.  For backward compatibility where loading an ELF file
> with a smaller struct bpf_map_def, only copy objects ELF size, leaving
> rest of loaders struct zero.  For forward compatibility where ELF file
> have a larger struct bpf_map_def, only copy loaders own struct size
> and verify that rest of the larger struct is zero, assuming this means
> the newer feature was not activated, thus it should be safe for this
> older loader to load this newer ELF file.
> 
> Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
> Fixes: 409526bea3c3 ("samples/bpf: bpf_load.c detect and abort if ELF maps section size is wrong")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

I would just merge patches 2 and 3 to reduce churn,
but it looks like great improvement already.
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [net-next PATCH 2/4] samples/bpf: make bpf_load.c code compatible with ELF maps section changes

[Jesper Dangaard Brouer]

On Tue, 2 May 2017 17:54:51 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Tue, May 02, 2017 at 02:31:56PM +0200, Jesper Dangaard Brouer wrote:
> > This patch does proper parsing of the ELF "maps" section, in-order to
> > be both backwards and forwards compatible with changes to the map
> > definition struct bpf_map_def, which gets compiled into the ELF file.
> > 
> > The assumption is that new features with value zero, means that they
> > are not in-use.  For backward compatibility where loading an ELF file
> > with a smaller struct bpf_map_def, only copy objects ELF size, leaving
> > rest of loaders struct zero.  For forward compatibility where ELF file
> > have a larger struct bpf_map_def, only copy loaders own struct size
> > and verify that rest of the larger struct is zero, assuming this means
> > the newer feature was not activated, thus it should be safe for this
> > older loader to load this newer ELF file.
> > 
> > Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
> > Fixes: 409526bea3c3 ("samples/bpf: bpf_load.c detect and abort if ELF maps section size is wrong")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>  
> 
> I would just merge patches 2 and 3 to reduce churn,
> but it looks like great improvement already.

I could have combined them, but I prefer keeping them separate to keep
the ELF changes separated from changing a sample program e.g.
map_perf_test_user.c.  IHMO is is cleaner this way.

> Acked-by: Alexei Starovoitov <ast@kernel.org>
 
Thanks

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

[net-next PATCH 3/4] samples/bpf: load_bpf.c make callback fixup more flexible

[Jesper Dangaard Brouer]

Do this change before others start to use this callback.
Change map_perf_test_user.c which seems to be the only user.

This patch extends capabilities of commit 9fd63d05f3e8 ("bpf:
Allow bpf sample programs (*_user.c) to change bpf_map_def").

Give fixup callback access to struct bpf_map_data, instead of
only stuct bpf_map_def.  This add flexibility to allow userspace
to reassign the map file descriptor.  This is very useful when
wanting to share maps between several bpf programs.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 samples/bpf/bpf_load.c           |   17 ++++++++---------
 samples/bpf/bpf_load.h           |   10 ++++++++--
 samples/bpf/map_perf_test_user.c |   14 +++++++-------
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index fedec29c7817..74456b3eb89a 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -39,13 +39,6 @@ int event_fd[MAX_PROGS];
 int prog_cnt;
 int prog_array_fd = -1;
 
-/* Keeping relevant info on maps */
-struct bpf_map_data {
-	int fd;
-	char *name;
-	size_t elf_offset;
-	struct bpf_map_def def;
-};
 struct bpf_map_data map_data[MAX_MAPS];
 int map_data_count = 0;
 
@@ -202,8 +195,14 @@ static int load_maps(struct bpf_map_data *maps, int nr_maps,
 	int i;
 
 	for (i = 0; i < nr_maps; i++) {
-		if (fixup_map)
-			fixup_map(&maps[i].def, maps[i].name, i);
+		if (fixup_map) {
+			fixup_map(&maps[i], i);
+			/* Allow userspace to assign map FD prior to creation */
+			if (maps[i].fd != -1) {
+				map_fd[i] = maps[i].fd;
+				continue;
+			}
+		}
 
 		if (maps[i].def.type == BPF_MAP_TYPE_ARRAY_OF_MAPS ||
 		    maps[i].def.type == BPF_MAP_TYPE_HASH_OF_MAPS) {
diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
index 05822f83173a..4d4fd4678a64 100644
--- a/samples/bpf/bpf_load.h
+++ b/samples/bpf/bpf_load.h
@@ -15,8 +15,14 @@ struct bpf_map_def {
 	unsigned int inner_map_idx;
 };
 
-typedef void (*fixup_map_cb)(struct bpf_map_def *map, const char *map_name,
-			     int idx);
+struct bpf_map_data {
+	int fd;
+	char *name;
+	size_t elf_offset;
+	struct bpf_map_def def;
+};
+
+typedef void (*fixup_map_cb)(struct bpf_map_data *map, int idx);
 
 extern int map_fd[MAX_MAPS];
 extern int prog_fd[MAX_PROGS];
diff --git a/samples/bpf/map_perf_test_user.c b/samples/bpf/map_perf_test_user.c
index 6ac778153315..1a8894b5ac51 100644
--- a/samples/bpf/map_perf_test_user.c
+++ b/samples/bpf/map_perf_test_user.c
@@ -320,21 +320,21 @@ static void fill_lpm_trie(void)
 	assert(!r);
 }
 
-static void fixup_map(struct bpf_map_def *map, const char *name, int idx)
+static void fixup_map(struct bpf_map_data *map, int idx)
 {
 	int i;
 
-	if (!strcmp("inner_lru_hash_map", name)) {
+	if (!strcmp("inner_lru_hash_map", map->name)) {
 		inner_lru_hash_idx = idx;
-		inner_lru_hash_size = map->max_entries;
+		inner_lru_hash_size = map->def.max_entries;
 	}
 
-	if (!strcmp("array_of_lru_hashs", name)) {
+	if (!strcmp("array_of_lru_hashs", map->name)) {
 		if (inner_lru_hash_idx == -1) {
 			printf("inner_lru_hash_map must be defined before array_of_lru_hashs\n");
 			exit(1);
 		}
-		map->inner_map_idx = inner_lru_hash_idx;
+		map->def.inner_map_idx = inner_lru_hash_idx;
 		array_of_lru_hashs_idx = idx;
 	}
 
@@ -345,9 +345,9 @@ static void fixup_map(struct bpf_map_def *map, const char *name, int idx)
 
 	/* Only change the max_entries for the enabled test(s) */
 	for (i = 0; i < NR_TESTS; i++) {
-		if (!strcmp(test_map_names[i], name) &&
+		if (!strcmp(test_map_names[i], map->name) &&
 		    (check_test_flags(i))) {
-			map->max_entries = num_map_entries;
+			map->def.max_entries = num_map_entries;
 		}
 	}
 }

[net-next PATCH 4/4] samples/bpf: export map_data[] for more info on maps

[Jesper Dangaard Brouer]

Giving *_user.c side tools access to map_data[] provides easier
access to information on the maps being loaded.  Still provide
the guarantee that the order maps are being defined in inside the
_kern.c file corresponds with the order in the array.  Now user
tools are not blind, but can inspect and verify the maps that got
loaded from the ELF binary.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 samples/bpf/bpf_load.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
index 4d4fd4678a64..ca0563d04744 100644
--- a/samples/bpf/bpf_load.h
+++ b/samples/bpf/bpf_load.h
@@ -24,12 +24,18 @@ struct bpf_map_data {
 
 typedef void (*fixup_map_cb)(struct bpf_map_data *map, int idx);
 
-extern int map_fd[MAX_MAPS];
 extern int prog_fd[MAX_PROGS];
 extern int event_fd[MAX_PROGS];
 extern char bpf_log_buf[BPF_LOG_BUF_SIZE];
 extern int prog_cnt;
 
+/* There is a one-to-one mapping between map_fd[] and map_data[].
+ * The map_data[] just contains more rich info on the given map.
+ */
+extern int map_fd[MAX_MAPS];
+extern struct bpf_map_data map_data[MAX_MAPS];
+extern int map_data_count;
+
 /* parses elf file compiled by llvm .c->.o
  * . parses 'maps' section and creates maps via BPF syscall
  * . parses 'license' section and passes it to syscall

Re: [net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Tue, 02 May 2017 14:31:45 +0200

> This series improves and fixes bpf ELF loader and programs under
> samples/bpf.  The bpf_load.c created some hard to debug issues when
> the struct (bpf_map_def) used in the ELF maps section format changed
> in commit fb30d4b71214 ("bpf: Add tests for map-in-map").
> 
> This was hotfixed in commit 409526bea3c3 ("samples/bpf: bpf_load.c
> detect and abort if ELF maps section size is wrong") by detecting the
> issue and aborting the program.
> 
> In most situations the bpf-loader should be able to handle these kind
> of changes to the struct size.  This patch series aim to do proper
> backward and forward compabilility handling when loading ELF files.
> 
> This series also adjust the callback that was introduced in commit
> 9fd63d05f3e8 ("bpf: Allow bpf sample programs (*_user.c) to change
> bpf_map_def") to use the new bpf_map_data structure, before more users
> start to use this callback.
> 
> Hoping these changes can make the merge window, as above mentioned
> commits have not been merged yet, and it would be good to avoid users
> hitting these issues.

Alexei and Daniel, please review.

Re: [net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[Daniel Borkmann]

On 05/02/2017 09:40 PM, David Miller wrote:
> From: Jesper Dangaard Brouer <brouer@redhat.com>
> Date: Tue, 02 May 2017 14:31:45 +0200
>
>> This series improves and fixes bpf ELF loader and programs under
>> samples/bpf.  The bpf_load.c created some hard to debug issues when
>> the struct (bpf_map_def) used in the ELF maps section format changed
>> in commit fb30d4b71214 ("bpf: Add tests for map-in-map").
>>
>> This was hotfixed in commit 409526bea3c3 ("samples/bpf: bpf_load.c
>> detect and abort if ELF maps section size is wrong") by detecting the
>> issue and aborting the program.
>>
>> In most situations the bpf-loader should be able to handle these kind
>> of changes to the struct size.  This patch series aim to do proper
>> backward and forward compabilility handling when loading ELF files.
>>
>> This series also adjust the callback that was introduced in commit
>> 9fd63d05f3e8 ("bpf: Allow bpf sample programs (*_user.c) to change
>> bpf_map_def") to use the new bpf_map_data structure, before more users
>> start to use this callback.
>>
>> Hoping these changes can make the merge window, as above mentioned
>> commits have not been merged yet, and it would be good to avoid users
>> hitting these issues.
>
> Alexei and Daniel, please review.

I'm on it.

Re: [net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[Daniel Borkmann]

On 05/02/2017 02:31 PM, Jesper Dangaard Brouer wrote:
> This series improves and fixes bpf ELF loader and programs under
> samples/bpf.  The bpf_load.c created some hard to debug issues when
> the struct (bpf_map_def) used in the ELF maps section format changed
> in commit fb30d4b71214 ("bpf: Add tests for map-in-map").
>
> This was hotfixed in commit 409526bea3c3 ("samples/bpf: bpf_load.c
> detect and abort if ELF maps section size is wrong") by detecting the
> issue and aborting the program.
>
> In most situations the bpf-loader should be able to handle these kind
> of changes to the struct size.  This patch series aim to do proper
> backward and forward compabilility handling when loading ELF files.
>
> This series also adjust the callback that was introduced in commit
> 9fd63d05f3e8 ("bpf: Allow bpf sample programs (*_user.c) to change
> bpf_map_def") to use the new bpf_map_data structure, before more users
> start to use this callback.
>
> Hoping these changes can make the merge window, as above mentioned
> commits have not been merged yet, and it would be good to avoid users
> hitting these issues.

Overall, set looks good to me. The last patch doesn't have a
user yet, so probably better to drop it until there is an actual
user in the tree.

Long term, I'd like to see the samples being migrated to use the
tools/lib/bpf/ library from the tree, so that we can avoid duplicating
effort with having two libs in the tree (f.e. elf map validation is
performed to a certain degree in the other one, but w/o compat
support last time I looked).

Anyway, other than that:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[Jesper Dangaard Brouer]

On Tue, 02 May 2017 23:10:04 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 05/02/2017 02:31 PM, Jesper Dangaard Brouer wrote:
> > This series improves and fixes bpf ELF loader and programs under
> > samples/bpf.  The bpf_load.c created some hard to debug issues when
> > the struct (bpf_map_def) used in the ELF maps section format changed
> > in commit fb30d4b71214 ("bpf: Add tests for map-in-map").
> >
> > This was hotfixed in commit 409526bea3c3 ("samples/bpf: bpf_load.c
> > detect and abort if ELF maps section size is wrong") by detecting the
> > issue and aborting the program.
> >
> > In most situations the bpf-loader should be able to handle these kind
> > of changes to the struct size.  This patch series aim to do proper
> > backward and forward compabilility handling when loading ELF files.
> >
> > This series also adjust the callback that was introduced in commit
> > 9fd63d05f3e8 ("bpf: Allow bpf sample programs (*_user.c) to change
> > bpf_map_def") to use the new bpf_map_data structure, before more users
> > start to use this callback.
> >
> > Hoping these changes can make the merge window, as above mentioned
> > commits have not been merged yet, and it would be good to avoid users
> > hitting these issues.  
> 
> Overall, set looks good to me. The last patch doesn't have a
> user yet, so probably better to drop it until there is an actual
> user in the tree.

The reason for simply exporting map_data[] was that in patch 3, the
data-struct (bpf_map_data) is already exposed, thus users can already
grab and store those into a separate data structure.  Thus, it seemed
natural to simply export/expose the map_data[] array directly.  Guess,
I could have combined patch 4 and 3.  As patch-3 uses the data struct,
but in an indirect way.

To Daniel, if you still feel we should drop patch 4, then let me know.
It is only the other patches that are time critical, as patch 4 is
trivial to introduce once the first sample program uses this directly
(instead of indirectly through the callback).


> Long term, I'd like to see the samples being migrated to use the
> tools/lib/bpf/ library from the tree, so that we can avoid duplicating
> effort with having two libs in the tree (f.e. elf map validation is
> performed to a certain degree in the other one, but w/o compat
> support last time I looked).

Yes, I agree that we should migrate to use the tools/lib/bpf/ library.
But as you also say, it actually have similar compat loader issues,
although it does more validation.  Once we start this migration, I'll
also fix the compat loader issues in this lib.
 
> Anyway, other than that:
> 
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Thanks

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

Re: [net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[Daniel Borkmann]

On 05/03/2017 08:16 AM, Jesper Dangaard Brouer wrote:
> On Tue, 02 May 2017 23:10:04 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 05/02/2017 02:31 PM, Jesper Dangaard Brouer wrote:
>>> This series improves and fixes bpf ELF loader and programs under
>>> samples/bpf.  The bpf_load.c created some hard to debug issues when
>>> the struct (bpf_map_def) used in the ELF maps section format changed
>>> in commit fb30d4b71214 ("bpf: Add tests for map-in-map").
>>>
>>> This was hotfixed in commit 409526bea3c3 ("samples/bpf: bpf_load.c
>>> detect and abort if ELF maps section size is wrong") by detecting the
>>> issue and aborting the program.
>>>
>>> In most situations the bpf-loader should be able to handle these kind
>>> of changes to the struct size.  This patch series aim to do proper
>>> backward and forward compabilility handling when loading ELF files.
>>>
>>> This series also adjust the callback that was introduced in commit
>>> 9fd63d05f3e8 ("bpf: Allow bpf sample programs (*_user.c) to change
>>> bpf_map_def") to use the new bpf_map_data structure, before more users
>>> start to use this callback.
>>>
>>> Hoping these changes can make the merge window, as above mentioned
>>> commits have not been merged yet, and it would be good to avoid users
>>> hitting these issues.
>>
>> Overall, set looks good to me. The last patch doesn't have a
>> user yet, so probably better to drop it until there is an actual
>> user in the tree.
>
> The reason for simply exporting map_data[] was that in patch 3, the
> data-struct (bpf_map_data) is already exposed, thus users can already
> grab and store those into a separate data structure.  Thus, it seemed
> natural to simply export/expose the map_data[] array directly.  Guess,
> I could have combined patch 4 and 3.  As patch-3 uses the data struct,
> but in an indirect way.
>
> To Daniel, if you still feel we should drop patch 4, then let me know.
> It is only the other patches that are time critical, as patch 4 is
> trivial to introduce once the first sample program uses this directly
> (instead of indirectly through the callback).

Ah, if there's still code coming from your side that will make use
of it, I'm fine with this, though it's usually best to add it with
an actual user. Given it's sample code, I don't mind too much if that
happens at a later point in time.

Thanks,
Daniel

Re: [net-next PATCH 0/4] Improve bpf ELF-loader under samples/bpf

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Tue, 02 May 2017 14:31:45 +0200

> This series improves and fixes bpf ELF loader and programs under
> samples/bpf.  The bpf_load.c created some hard to debug issues when
> the struct (bpf_map_def) used in the ELF maps section format changed
> in commit fb30d4b71214 ("bpf: Add tests for map-in-map").
> 
> This was hotfixed in commit 409526bea3c3 ("samples/bpf: bpf_load.c
> detect and abort if ELF maps section size is wrong") by detecting the
> issue and aborting the program.
> 
> In most situations the bpf-loader should be able to handle these kind
> of changes to the struct size.  This patch series aim to do proper
> backward and forward compabilility handling when loading ELF files.
> 
> This series also adjust the callback that was introduced in commit
> 9fd63d05f3e8 ("bpf: Allow bpf sample programs (*_user.c) to change
> bpf_map_def") to use the new bpf_map_data structure, before more users
> start to use this callback.
> 
> Hoping these changes can make the merge window, as above mentioned
> commits have not been merged yet, and it would be good to avoid users
> hitting these issues.

I've applied this series, thanks!