Strange samples/bpf loading error for maps on net-next?

Strange samples/bpf loading error for maps on net-next?

[Jesper Dangaard Brouer]

To provoke this bug, remember that you MUST call:

 make headers_install

In the kernels root directory, else you will be compiling samples/bpf/
against the older headers previously installed.

The error looks like:

 $ sudo ./sockex1
 bpf_load_program() err=22
 fd 0 is not pointing to valid bpf_map
 sockex1: [...]/samples/bpf/sockex1_user.c:26: main: Assertion `setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd, sizeof(prog_fd[0])) == 0' failed.
 Aborted

I've found that the bug were introduced in
 commit: fb30d4b71214 ("bpf: Add tests for map-in-map")

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

Re: Strange samples/bpf loading error for maps on net-next?

[Alexei Starovoitov]

On Thu, Apr 27, 2017 at 01:15:42PM +0200, Jesper Dangaard Brouer wrote:
> 
> To provoke this bug, remember that you MUST call:
> 
>  make headers_install
> 
> In the kernels root directory, else you will be compiling samples/bpf/
> against the older headers previously installed.
> 
> The error looks like:
> 
>  $ sudo ./sockex1
>  bpf_load_program() err=22
>  fd 0 is not pointing to valid bpf_map
>  sockex1: [...]/samples/bpf/sockex1_user.c:26: main: Assertion `setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd, sizeof(prog_fd[0])) == 0' failed.
>  Aborted
> 
> I've found that the bug were introduced in
>  commit: fb30d4b71214 ("bpf: Add tests for map-in-map")

Great debugging!
Indeed that change made samples/bpf/bpf_load.c to be incompatible with .o
generated earlier. We should really get rid of that loader and
switch to tools/lib/bpf/. I believe Eric Leblond already made it
resilient to 'struct bpf_map_def' changes.

Re: Strange samples/bpf loading error for maps on net-next?

[Jesper Dangaard Brouer]

On Thu, 27 Apr 2017 22:49:51 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Thu, Apr 27, 2017 at 01:15:42PM +0200, Jesper Dangaard Brouer wrote:
> > 
> > To provoke this bug, remember that you MUST call:
> > 
> >  make headers_install
> > 
> > In the kernels root directory, else you will be compiling samples/bpf/
> > against the older headers previously installed.
> > 
> > The error looks like:
> > 
> >  $ sudo ./sockex1
> >  bpf_load_program() err=22
> >  fd 0 is not pointing to valid bpf_map
> >  sockex1: [...]/samples/bpf/sockex1_user.c:26: main: Assertion `setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd, sizeof(prog_fd[0])) == 0' failed.
> >  Aborted
> > 
> > I've found that the bug were introduced in
> >  commit: fb30d4b71214 ("bpf: Add tests for map-in-map")  
> 
> Great debugging!
> Indeed that change made samples/bpf/bpf_load.c to be incompatible with .o
> generated earlier. We should really get rid of that loader and
> switch to tools/lib/bpf/. I believe Eric Leblond already made it
> resilient to 'struct bpf_map_def' changes.

Yes, exactly it is problem in samples/bpf/bpf_load.c.  As it assumes
the contents of the ELF file maps section will always chunks in
sizeof(struct bpf_map_def) and just uses that directly as a pointer to
an array of type struct bpf_map_def, which of-cause silently blows up
when changing struct bpf_map_def.  That cost me many hours to discover
that yesterday.

I started implementing more correct parsing of the ELF maps section, it
is doable, but as you say, maybe we should just get rid of this loader?
I will at least fixup bpf_load.c and perhaps just abort the program the
program if I detect a difference between the ELF size and struct size.
And send this as a patch later today...

I've also looked at the loaded Daniel implemented[1] in iproute2, and
it is much cleaner.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git/tree/lib/bpf.c
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

[net-next PATCH V1] samples/bpf: bpf_load.c detect and abort if ELF maps section size is wrong

[Jesper Dangaard Brouer]

The struct bpf_map_def was extended in commit fb30d4b71214 ("bpf: Add tests
for map-in-map") with member unsigned int inner_map_idx.  This changed the size
of the maps section in the generated ELF _kern.o files.

Unfortunately the loader in bpf_load.c does not detect or handle this.  Thus,
older _kern.o files became incompatible, and caused hard-to-debug errors
where the syscall validation rejected BPF_MAP_CREATE request.

This patch only detect the situation and aborts load_bpf_file(). It also
add code comments warning people that read this loader for inspiration
for these pitfalls.

Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
Is it worth to implement proper backward-compat loading of older ELF objects
with this bpf-loader?

 samples/bpf/bpf_load.c |   40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 0d449d8032d1..b84ac9dbff18 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -185,12 +185,16 @@ 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 len,
+static int load_maps(struct bpf_map_def *maps, int nr_maps,
 		     const char **map_names, fixup_map_cb fixup_map)
 {
 	int i;
-
-	for (i = 0; i < len / sizeof(struct bpf_map_def); 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);
 
@@ -269,6 +273,10 @@ 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)];
 	}
 
@@ -311,18 +319,18 @@ static int get_sorted_map_names(Elf *elf, Elf_Data *symbols, int maps_shndx,
 		map_name = elf_strptr(elf, strtabidx, map_symbols[i].st_name);
 		if (!map_name) {
 			printf("cannot get map symbol\n");
-			return 1;
+			return -1;
 		}
 
 		map_names[i] = strdup(map_name);
 		if (!map_names[i]) {
 			printf("strdup(%s): %s(%d)\n", map_name,
 			       strerror(errno), errno);
-			return 1;
+			return -1;
 		}
 	}
 
-	return 0;
+	return nr_maps;
 }
 
 static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
@@ -396,11 +404,25 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
 	}
 
 	if (data_maps) {
-		if (get_sorted_map_names(elf, symbols, maps_shndx, strtabidx,
-					 map_names))
+		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;
 
-		if (load_maps(data_maps->d_buf, data_maps->d_size,
+		/* 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));
+			ret = 1;
+			goto done;
+		}
+
+		if (load_maps(data_maps->d_buf, nr_maps,
 			      (const char **)map_names, fixup_map))
 			goto done;
 

Re: [net-next PATCH V1] samples/bpf: bpf_load.c detect and abort if ELF maps section size is wrong

[Alexei Starovoitov]

On Fri, Apr 28, 2017 at 04:25:04PM +0200, Jesper Dangaard Brouer wrote:
> The struct bpf_map_def was extended in commit fb30d4b71214 ("bpf: Add tests
> for map-in-map") with member unsigned int inner_map_idx.  This changed the size
> of the maps section in the generated ELF _kern.o files.
> 
> Unfortunately the loader in bpf_load.c does not detect or handle this.  Thus,
> older _kern.o files became incompatible, and caused hard-to-debug errors
> where the syscall validation rejected BPF_MAP_CREATE request.
> 
> This patch only detect the situation and aborts load_bpf_file(). It also
> add code comments warning people that read this loader for inspiration
> for these pitfalls.
> 
> Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

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

> Is it worth to implement proper backward-compat loading of older ELF objects
> with this bpf-loader?

probably yes, since it looks like a bunch of code in samples/bpf/ still
depend on it and some features are missing in tools/lib/bpf,
so unless we actively work on improving libbpf.a 
we won't be able to get rid of this 'sample' loader for some time.

Re: [net-next PATCH V1] samples/bpf: bpf_load.c detect and abort if ELF maps section size is wrong

[Jesper Dangaard Brouer]

On Fri, 28 Apr 2017 20:35:21 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Fri, Apr 28, 2017 at 04:25:04PM +0200, Jesper Dangaard Brouer wrote:
> > The struct bpf_map_def was extended in commit fb30d4b71214 ("bpf: Add tests
> > for map-in-map") with member unsigned int inner_map_idx.  This changed the size
> > of the maps section in the generated ELF _kern.o files.
> > 
> > Unfortunately the loader in bpf_load.c does not detect or handle this.  Thus,
> > older _kern.o files became incompatible, and caused hard-to-debug errors
> > where the syscall validation rejected BPF_MAP_CREATE request.
> > 
> > This patch only detect the situation and aborts load_bpf_file(). It also
> > add code comments warning people that read this loader for inspiration
> > for these pitfalls.
> > 
> > Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>  
> 
> Thanks!
> Acked-by: Alexei Starovoitov <ast@kernel.org>

Thanks!

> > Is it worth to implement proper backward-compat loading of older ELF objects
> > with this bpf-loader?  
> 
> probably yes, since it looks like a bunch of code in samples/bpf/ still
> depend on it and some features are missing in tools/lib/bpf,
> so unless we actively work on improving libbpf.a 
> we won't be able to get rid of this 'sample' loader for some time.

Okay, I'll work on that next week. Hoping I can make the merge window,
so we avoid having a non-backward compat bpf_load in a kernel release.

p.s. I'll be presenting about XDP in Sweden Thur+Friday next week:
 https://lundlinuxcon.org/?page=current

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

Re: [net-next PATCH V1] samples/bpf: bpf_load.c detect and abort if ELF maps section size is wrong

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Fri, 28 Apr 2017 16:25:04 +0200

> The struct bpf_map_def was extended in commit fb30d4b71214 ("bpf: Add tests
> for map-in-map") with member unsigned int inner_map_idx.  This changed the size
> of the maps section in the generated ELF _kern.o files.
> 
> Unfortunately the loader in bpf_load.c does not detect or handle this.  Thus,
> older _kern.o files became incompatible, and caused hard-to-debug errors
> where the syscall validation rejected BPF_MAP_CREATE request.
> 
> This patch only detect the situation and aborts load_bpf_file(). It also
> add code comments warning people that read this loader for inspiration
> for these pitfalls.
> 
> Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Applied.