'perf test BPF' failing, libbpf regression wrt "basic API for BPF obj name"

So, I noticed that any maps are failing, I'll go dig, but may be some new security tightening, even running as root, this was working recently, was even part of our discussion on the bpf_probe_read_str() trouble with clang's optimizer:

[root@jouet bpf]# cat open.c
#include "bpf.h"

SEC("prog=do_sys_open filename")
int prog(void *ctx, int err, char *filename_ptr)

{
	char filename[128];
	int len = bpf_probe_read_str(filename, sizeof(filename), filename_ptr); 
	if (len > 0) {
		if (len == 1)
			perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, len);
		else if (len < 128)
			perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, len);
        }
	return 1;
}

[root@jouet bpf]#
<SNIP>
Found 1 probe_trace_events.
Opening /sys/kernel/debug/tracing//kprobe_events write=1 Writing event: p:perf_bpf_probe/prog _text+2493856 filename=%si:x64 In map_prologue, ntevs=1
mapping[0]=0
libbpf: failed to create map (name: '__bpf_stdout__'): Invalid argument
libbpf: failed to load object 'open.c'
bpf: load objects failed
event syntax error: 'open.c'
\___ Operation not permitted
<SNIP>

Using 'perf ftrace' to trace just 'perf trace':

[root@jouet bpf]# perf ftrace -G SyS_bpf perf trace -v -e open.c,open cat /tmp/somefile 2> /dev/null

 0)               |  SyS_bpf() {
 0)               |    capable() {
 0)               |      ns_capable_common() {
 0)               |        security_capable() {
 0)   0.045 us    |          cap_capable();
 0)               |          selinux_capable() {
 0)   0.274 us    |            cred_has_capability();
 0)   0.518 us    |          }
 0)   1.464 us    |        }
 0)   1.783 us    |      }
 0)   2.130 us    |    }
 0)   0.458 us    |    check_uarg_tail_zero();
 0)               |    __check_object_size() {
 0)   0.046 us    |      __virt_addr_valid();
 0)   0.040 us    |      check_stack_object();
 0)   0.510 us    |    }
 0)   4.161 us    |  }

[root@jouet bpf]#

/me goes to look at SyS_bpf() in this kernel... (4.14.0+).

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
--- start ---
test child forked, pid 12198
Kernel build dir is set to /lib/modules/4.14.0+/build set env: KBUILD_DIR=/lib/modules/4.14.0+/build

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

Quoted 8 lines

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
--- start ---
test child forked, pid 12198
Kernel build dir is set to /lib/modules/4.14.0+/build set env: KBUILD_DIR=/lib/modules/4.14.0+/build

[ ... ]

Quoted 3 lines

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") is introduced in 4.15.

I think the [email protected] broke on older kernels like 4.14 because the new bpf prog/map name is only introduced since 4.15.

The newer perf needs to be compatible with an older kernel?

Em Wed, Nov 29, 2017 at 01:07:34PM -0800, Martin KaFai Lau escreveu:

Quoted 15 lines · Arnaldo Carvalho de Melo

Quoted 8 lines

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
--- start ---
test child forked, pid 12198
Kernel build dir is set to /lib/modules/4.14.0+/build set env: KBUILD_DIR=/lib/modules/4.14.0+/build

[ ... ]

Quoted 3 lines

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") is introduced in 4.15.

Quoted 4 lines

I think the [email protected] broke on older kernels like 4.14 because the new bpf prog/map name is only introduced since 4.15.

The newer perf needs to be compatible with an older kernel?

Sure :-)

If some ABI breaks it should detect that and adapt, and older perf tools should also gracefully fail in such a case, which I'm not sure will be the case here, haven't checked perf's BPF integration to see how it behaves in such a case, but I will.

- Arnaldo

Quoted 18 lines

Em Wed, Nov 29, 2017 at 01:07:34PM -0800, Martin KaFai Lau escreveu:

Quoted 17 lines · Arnaldo Carvalho de Melo

Quoted 7 lines

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
--- start ---
test child forked, pid 12198
Kernel build dir is set to /lib/modules/4.14.0+/build set env: KBUILD_DIR=/lib/modules/4.14.0+/build

[ ... ]

Quoted 3 lines

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") is introduced in 4.15.

I think the [email protected] broke on older kernels like 4.14 because the new bpf prog/map name is only introduced since 4.15.

Quoted 3 lines

Quoted 1 line

The newer perf needs to be compatible with an older kernel?

Sure :-)

Would the latest features introduced in perf/libbpf supposed to be available in the latest kernel only? What may be the reason that the

latest perf is installed with an older kernel while it does not gain new functionality?

Em Wed, Nov 29, 2017 at 02:31:36PM -0800, Martin KaFai Lau escreveu:

Quoted 17 lines · Arnaldo Carvalho de Melo

Quoted 15 lines

Em Wed, Nov 29, 2017 at 01:07:34PM -0800, Martin KaFai Lau escreveu:

Quoted 14 lines · Arnaldo Carvalho de Melo

Quoted 4 lines

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
Kernel build dir is set to /lib/modules/4.14.0+/build

[ ... ]

Quoted 3 lines

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") is introduced in 4.15.

I think the [email protected] broke on older kernels like 4.14 because the new bpf prog/map name is only introduced since 4.15.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

The newer perf needs to be compatible with an older kernel?

Sure :-)

Would the latest features introduced in perf/libbpf supposed to be available in the latest kernel only? What may be the reason that the

Yes, then the new perf binary should try to use the new stuff, if it fails, use the old one, there is no requirement that one uses perf 4.14 in lockstep with the kernel 4.14 (or any other version), perf 4.15 should work with the 4.14 kernel as well as with 4.15 (or any other future kernel), only limited by what it can grok up to when it was released.

Em Thu, Nov 30, 2017 at 12:01:10AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 25 lines

Em Wed, Nov 29, 2017 at 02:31:36PM -0800, Martin KaFai Lau escreveu:

Quoted 17 lines · Arnaldo Carvalho de Melo

Quoted 15 lines

Em Wed, Nov 29, 2017 at 01:07:34PM -0800, Martin KaFai Lau escreveu:

Quoted 14 lines · Arnaldo Carvalho de Melo

Quoted 4 lines

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
Kernel build dir is set to /lib/modules/4.14.0+/build

[ ... ]

Quoted 3 lines

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") is introduced in 4.15.

I think the [email protected] broke on older kernels like 4.14 because the new bpf prog/map name is only introduced since 4.15.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

The newer perf needs to be compatible with an older kernel?

Sure :-)

Would the latest features introduced in perf/libbpf supposed to be available in the latest kernel only? What may be the reason that the

Quoted 6 lines

Yes, then the new perf binary should try to use the new stuff, if it fails, use the old one, there is no requirement that one uses perf 4.14 in lockstep with the kernel 4.14 (or any other version), perf 4.15 should work with the 4.14 kernel as well as with 4.15 (or any other future kernel), only limited by what it can grok up to when it was released.

So, see the patch below, that makes a 'perf test bpf' and my other test cases, including that one for probe_read_str() work again, it just fallbacks to a behaviour the older kernels can accept.

We can improve it so that that EINVAL fallback happens only for MAP_CREATE, and probably we don't need to change the size arg, just zero the unused fields, but I haven't checked that.

- Arnaldo

tools/lib/bpf/bpf.c +1 -0

index 5128677e4117..3084f07c7c33 100644
@@ -19,6 +19,7 @@
  * License along with this program; if not,  see <http://www.gnu.org/licenses>
  */
 
+#include <errno.h>
 #include <stdlib.h>
 #include <memory.h>
 #include <unistd.h>

tools/lib/bpf/bpf.c +4 -0

+
+		err = syscall(__NR_bpf, cmd, attr, old_union_size);
+	}
+	return err;
 }
 
 int bpf_create_map_node(enum bpf_map_type map_type, const char *name,

Quoted 33 lines

Em Thu, Nov 30, 2017 at 12:01:10AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 25 lines

Em Wed, Nov 29, 2017 at 02:31:36PM -0800, Martin KaFai Lau escreveu:

Quoted 17 lines · Arnaldo Carvalho de Melo

Quoted 15 lines

Em Wed, Nov 29, 2017 at 01:07:34PM -0800, Martin KaFai Lau escreveu:

Quoted 14 lines · Arnaldo Carvalho de Melo

Quoted 4 lines

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
Kernel build dir is set to /lib/modules/4.14.0+/build

[ ... ]

Quoted 3 lines

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") is introduced in 4.15.

I think the [email protected] broke on older kernels like 4.14 because the new bpf prog/map name is only introduced since 4.15.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

The newer perf needs to be compatible with an older kernel?

Sure :-)

Would the latest features introduced in perf/libbpf supposed to be available in the latest kernel only? What may be the reason that the

Quoted 6 lines

Yes, then the new perf binary should try to use the new stuff, if it fails, use the old one, there is no requirement that one uses perf 4.14 in lockstep with the kernel 4.14 (or any other version), perf 4.15 should work with the 4.14 kernel as well as with 4.15 (or any other future kernel), only limited by what it can grok up to when it was released.

Quoted 3 lines

So, see the patch below, that makes a 'perf test bpf' and my other test cases, including that one for probe_read_str() work again, it just fallbacks to a behaviour the older kernels can accept.

Thanks for the patch.

Quoted 20 lines

We can improve it so that that EINVAL fallback happens only for MAP_CREATE, and probably we don't need to change the size arg, just zero the unused fields, but I haven't checked that.

tools/lib/bpf/bpf.c +1 -0

index 5128677e4117..3084f07c7c33 100644
@@ -19,6 +19,7 @@
  * License along with this program; if not,  see <http://www.gnu.org/licenses>
  */
 
+#include <errno.h>
 #include <stdlib.h>
 #include <memory.h>
 #include <unistd.h>

@@ -53,10 +54,26 @@ static inline __u64 ptr_to_u64(const void *ptr)
 	return (__u64) (unsigned long) ptr;
 }
 

tools/lib/bpf/bpf.c static inline __u64 ptr_to_u64(const void *ptr) · line 60

Quoted 7 lines

tools/lib/bpf/bpf.c +3 -3

-static inline int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
-			  unsigned int size)
+static int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr, unsigned int size)
 {
-	return syscall(__NR_bpf, cmd, attr, size);
+	int err = syscall(__NR_bpf, cmd, attr, size);
+	if (err == -1 && (errno == EINVAL || errno == E2BIG)) {

I would add a check to the length of map_name/prog_name.

Quoted 8 lines

tools/lib/bpf/bpf.c +8 -0

+		const unsigned int old_union_size = offsetof(union bpf_attr, prog_name);
+		/*
+		 * These were the ones that added fields after the old bpf_attr
+		 * layout in commit 88cda1c9da02 ("bpf: libbpf: Provide basic
+		 * API support to specify BPF obj name") so zero that out to
+		 * pass the CHECK_ATTR() test in kernel/bpf/syscall.c in older
+		 * kernels.
+		 */

tools/lib/bpf/bpf.c static inline __u64 ptr_to_u64(const void *ptr) · line 72

Quoted 4 lines

tools/lib/bpf/bpf.c +4 -0

+		if (cmd == BPF_MAP_CREATE)
+			memset(&attr->map_name, 0, size - offsetof(union bpf_attr, map_name));
+		else
+			memset(&attr->prog_name, 0, size - old_union_size);

If bpf_attr is extended in the future, map_name/prog_name will still be used as the anchor for backward compatibility instead of trial and error attribute by attribute?

Instead of sinking all future bpf_attr's backward compatibility requirements to sys_bpf, I would push it up to its own BPF_* command helper which has a better sense of its bpf_attr, i.e. push it up to bpf_create_map_node() and bpf_load_program_name() in this case.

Quoted 6 lines

tools/lib/bpf/bpf.c +3 -0

+		err = syscall(__NR_bpf, cmd, attr, old_union_size);
+	}
+	return err;
 }
 
 int bpf_create_map_node(enum bpf_map_type map_type, const char *name,

Quoted 4 lines

Instead of sinking all future bpf_attr's backward compatibility requirements to sys_bpf, I would push it up to its own BPF_* command helper which has a better sense of its bpf_attr, i.e. push it up to bpf_create_map_node() and bpf_load_program_name() in this case.

Humm, we could try that approach, but the one in this patch seemed good enough.

And after all if the first syscall() invokation, with the latest kernel and latest tooling will work, right?

Quoted 4 lines

Quoted 4 lines

Instead of sinking all future bpf_attr's backward compatibility requirements to sys_bpf, I would push it up to its own BPF_* command helper which has a better sense of its bpf_attr, i.e. push it up to bpf_create_map_node() and bpf_load_program_name() in this case.

Quoted 5 lines

Humm, we could try that approach, but the one in this patch seemed good enough.

And after all if the first syscall() invokation, with the latest kernel and latest tooling will work, right?

I agree with Martin and I also don't think it will work to push logic of all bpf commands into single sys_bpf syscall wrapper.

This logic will become more and more complex over time. Like this case really belongs in bpf_create_map() which is a wrapper on top of single BPF_CREATE_MAP command.

Note it's the first time we're facing this 'new libbpf.a running on top of old kernel' issue and should be very careful adding such fallback code to the generic bpf library, since all the selftests/bpf/ are using this lib and relying on excepted behavior.

We don't want tests that want to test the latest kernel feature all of a sudden pass on old kernel that doesn't have it.

To some degree perf and selftests/bpf needs are diverging here, so adding #ifdef to libbpf.a to match testcase expectations may be necessary.

But this is not just testcase expectations, the usecase is someone wanting to use a newer tool, with perhaps some new features of interest that don't depend on changes in the kernel, in an older kernel on a system where updating it is not possible or desirable.

Em Thu, Nov 30, 2017 at 10:28:08AM -0800, Martin KaFai Lau escreveu:

Quoted 60 lines · Arnaldo Carvalho de Melo

Quoted 33 lines

Em Thu, Nov 30, 2017 at 12:01:10AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 25 lines

Em Wed, Nov 29, 2017 at 02:31:36PM -0800, Martin KaFai Lau escreveu:

Quoted 17 lines · Arnaldo Carvalho de Melo

Quoted 15 lines

Em Wed, Nov 29, 2017 at 01:07:34PM -0800, Martin KaFai Lau escreveu:

Quoted 14 lines · Arnaldo Carvalho de Melo

Quoted 4 lines

[root@jouet ~]# perf test -v bpf
39: BPF filter :
39.1: Basic BPF filtering :
Kernel build dir is set to /lib/modules/4.14.0+/build

[ ... ]

Quoted 3 lines

libbpf: failed to create map (name: 'flip_table'): Invalid argument
libbpf: failed to load object '[basic_bpf_test]'
bpf: load objects failed

88cda1c9da02 ("bpf: libbpf: Provide basic API support to specify BPF obj name") is introduced in 4.15.

I think the [email protected] broke on older kernels like 4.14 because the new bpf prog/map name is only introduced since 4.15.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

The newer perf needs to be compatible with an older kernel?

Sure :-)

Would the latest features introduced in perf/libbpf supposed to be available in the latest kernel only? What may be the reason that the

Quoted 6 lines

Yes, then the new perf binary should try to use the new stuff, if it fails, use the old one, there is no requirement that one uses perf 4.14 in lockstep with the kernel 4.14 (or any other version), perf 4.15 should work with the 4.14 kernel as well as with 4.15 (or any other future kernel), only limited by what it can grok up to when it was released.

Quoted 3 lines

So, see the patch below, that makes a 'perf test bpf' and my other test cases, including that one for probe_read_str() work again, it just fallbacks to a behaviour the older kernels can accept.

Thanks for the patch.

Quoted 20 lines

We can improve it so that that EINVAL fallback happens only for MAP_CREATE, and probably we don't need to change the size arg, just zero the unused fields, but I haven't checked that.

tools/lib/bpf/bpf.c +1 -0

index 5128677e4117..3084f07c7c33 100644
@@ -19,6 +19,7 @@
  * License along with this program; if not,  see <http://www.gnu.org/licenses>
  */
 
+#include <errno.h>
 #include <stdlib.h>
 #include <memory.h>
 #include <unistd.h>

@@ -53,10 +54,26 @@ static inline __u64 ptr_to_u64(const void *ptr)
 	return (__u64) (unsigned long) ptr;
 }
 

Quoted 8 lines

tools/lib/bpf/bpf.c static inline __u64 ptr_to_u64(const void *ptr) · line 60

Quoted 7 lines

tools/lib/bpf/bpf.c +3 -3

-static inline int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
-			  unsigned int size)
+static int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr, unsigned int size)
 {
-	return syscall(__NR_bpf, cmd, attr, size);
+	int err = syscall(__NR_bpf, cmd, attr, size);
+	if (err == -1 && (errno == EINVAL || errno == E2BIG)) {

I would add a check to the length of map_name/prog_name.

Can you elaborate? What kind of check?

Quoted 17 lines

tools/lib/bpf/bpf.c static inline __u64 ptr_to_u64(const void *ptr) · line 72

Quoted 13 lines

tools/lib/bpf/bpf.c +12 -0

+		const unsigned int old_union_size = offsetof(union bpf_attr, prog_name);
+		/*
+		 * These were the ones that added fields after the old bpf_attr
+		 * layout in commit 88cda1c9da02 ("bpf: libbpf: Provide basic
+		 * API support to specify BPF obj name") so zero that out to
+		 * pass the CHECK_ATTR() test in kernel/bpf/syscall.c in older
+		 * kernels.
+		 */
+		if (cmd == BPF_MAP_CREATE)
+			memset(&attr->map_name, 0, size - offsetof(union bpf_attr, map_name));
+		else
+			memset(&attr->prog_name, 0, size - old_union_size);

If bpf_attr is extended in the future, map_name/prog_name will still be used as the anchor for backward compatibility instead of trial and error attribute by attribute?

Then you will first try the latest and greatest, if it fails, go the previous (like here), if it fails, etc.

That or some sort of versioning to make sure the kernel and the tools can agree on a common set of functionality supported by both.

Again, this is how perf_evsel__open() works in the perf case, try the latest and go fallbacking to the most recent set of features that could somehow service what is needed or disable some feature and warn the user, i.e. do the best you can with what you have.

With this patch in place I was able to have what was working before 88cda1c9da02 working again.

Quoted 4 lines

Instead of sinking all future bpf_attr's backward compatibility requirements to sys_bpf, I would push it up to its own BPF_* command helper which has a better sense of its bpf_attr, i.e. push it up to bpf_create_map_node() and bpf_load_program_name() in this case.

Humm, we could try that approach, but the one in this patch seemed good enough.

And after all if the first syscall() invokation, with the latest kernel and latest tooling will work, right?

I think that we need as well bpf__strerror_METHOD() that can make sense of error returns per method (aka "command helper"), to tell the user when something can't be done why is that so: "you need a kernel >= 4.X", "this needs root privs", "too many maps, etc"

Right now we have a generic, libbpf wide one:

static const char *libbpf_strerror_table[NR_ERRNO] = {

        [ERRCODE_OFFSET(LIBELF)]        = "Something wrong in libelf",
        [ERRCODE_OFFSET(FORMAT)]        = "BPF object format invalid",
        [ERRCODE_OFFSET(KVERSION)]      = "'version' section incorrect or lost",
        [ERRCODE_OFFSET(ENDIAN)]        = "Endian mismatch",
        [ERRCODE_OFFSET(INTERNAL)]      = "Internal error in libbpf",
        [ERRCODE_OFFSET(RELOC)]         = "Relocation failed",
        [ERRCODE_OFFSET(VERIFY)]        = "Kernel verifier blocks program loading",
        [ERRCODE_OFFSET(PROG2BIG)]      = "Program too big",
        [ERRCODE_OFFSET(KVER)]          = "Incorrect kernel version",
        [ERRCODE_OFFSET(PROGTYPE)]      = "Kernel doesn't support this program type",

};

int libbpf_strerror(int err, char *buf, size_t size) {

	/* use the above or return strerror_r() for the errno usual range */
}

But as time goes by, features get added, ABI gets extended, and without a better kernel/user error reporting mechanism... Making sense of errno in the context of a specific method and looking at the system state looks like the best we can do to provide not-so-cryptic messages to the poor users, and we're all users :-)

- Arnaldo

Quoted 6 lines

Quoted 6 lines

tools/lib/bpf/bpf.c +3 -0

+		err = syscall(__NR_bpf, cmd, attr, old_union_size);
+	}
+	return err;
 }
 
 int bpf_create_map_node(enum bpf_map_type map_type, const char *name,