[PATCH bpf-next v2] libbpf: fix probe code to return EPERM if encountered

[PATCH bpf-next v2] libbpf: fix probe code to return EPERM if encountered

[Eelco Chaudron]

When the probe code was failing for any reason ENOTSUP was returned, even
if this was due to no having enough lock space. This patch fixes this by
returning EPERM to the user application, so it can respond and increase
the RLIMIT_MEMLOCK size.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v2: Split bpf_object__probe_name() in two functions as suggested by Andrii

 tools/lib/bpf/libbpf.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8f480e29a6b0..6838e6d431ce 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3149,7 +3149,7 @@ int bpf_map__resize(struct bpf_map *map, __u32 max_entries)
 }
 
 static int
-bpf_object__probe_name(struct bpf_object *obj)
+bpf_object__probe_loading(struct bpf_object *obj)
 {
 	struct bpf_load_program_attr attr;
 	char *cp, errmsg[STRERR_BUFSIZE];
@@ -3176,8 +3176,26 @@ bpf_object__probe_name(struct bpf_object *obj)
 	}
 	close(ret);
 
-	/* now try the same program, but with the name */
+	return 0;
+}
 
+static int
+bpf_object__probe_name(struct bpf_object *obj)
+{
+	struct bpf_load_program_attr attr;
+	struct bpf_insn insns[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+	int ret;
+
+	/* make sure loading with name works */
+
+	memset(&attr, 0, sizeof(attr));
+	attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+	attr.insns = insns;
+	attr.insns_cnt = ARRAY_SIZE(insns);
+	attr.license = "GPL";
 	attr.name = "test";
 	ret = bpf_load_program_xattr(&attr, NULL, 0);
 	if (ret >= 0) {
@@ -5386,7 +5404,8 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 
 	obj->loaded = true;
 
-	err = bpf_object__probe_caps(obj);
+	err = bpf_object__probe_loading(obj);
+	err = err ? : bpf_object__probe_caps(obj);
 	err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
 	err = err ? : bpf_object__sanitize_and_load_btf(obj);
 	err = err ? : bpf_object__sanitize_maps(obj);

Re: [PATCH bpf-next v2] libbpf: fix probe code to return EPERM if encountered

[Andrii Nakryiko]

On Mon, May 4, 2020 at 2:13 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
> When the probe code was failing for any reason ENOTSUP was returned, even
> if this was due to no having enough lock space. This patch fixes this by
> returning EPERM to the user application, so it can respond and increase
> the RLIMIT_MEMLOCK size.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> v2: Split bpf_object__probe_name() in two functions as suggested by Andrii

Yeah, looks good, and this is good enough, so consider you have my
ack. But I think we can further improve the experience by:

1. Changing existing "Couldn't load basic 'r0 = 0' BPF program."
message to be something more meaningful and actionable for user. E.g.,

"Couldn't load trivial BPF program. Make sure your kernel supports BPF
(CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is set to big enough
value."

Then even complete kernel newbies can search for CONFIG_BPF_SYSCALL or
RLIMIT_MEMLOCK and hopefully find useful discussions. We can/should
add RLIMIT_MEMLOCK examples to some FAQ, probably as well (if it's not
there already).

2. I'd do bpf_object__probe_loading() before obj->loaded is set, so
that user can have a loop of bpf_object__load() that bump
RLIMIT_MEMLOCK in steps. After setting obj->loaded = true, user won't
be able to attemp loading again and will get "object should not be
loaded twice\n".

[...]

Re: [PATCH bpf-next v2] libbpf: fix probe code to return EPERM if encountered

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Mon, May 4, 2020 at 2:13 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>>
>> When the probe code was failing for any reason ENOTSUP was returned, even
>> if this was due to no having enough lock space. This patch fixes this by
>> returning EPERM to the user application, so it can respond and increase
>> the RLIMIT_MEMLOCK size.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> v2: Split bpf_object__probe_name() in two functions as suggested by Andrii
>
> Yeah, looks good, and this is good enough, so consider you have my
> ack. But I think we can further improve the experience by:
>
> 1. Changing existing "Couldn't load basic 'r0 = 0' BPF program."
> message to be something more meaningful and actionable for user. E.g.,
>
> "Couldn't load trivial BPF program. Make sure your kernel supports BPF
> (CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is set to big enough
> value."
>
> Then even complete kernel newbies can search for CONFIG_BPF_SYSCALL or
> RLIMIT_MEMLOCK and hopefully find useful discussions. We can/should
> add RLIMIT_MEMLOCK examples to some FAQ, probably as well (if it's not
> there already).

Always on board with improving documentation; and yeah I agree that
"Couldn't load basic 'r0 = 0' BPF program." could be a bit friendlier ;)

> 2. I'd do bpf_object__probe_loading() before obj->loaded is set, so
> that user can have a loop of bpf_object__load() that bump
> RLIMIT_MEMLOCK in steps. After setting obj->loaded = true, user won't
> be able to attemp loading again and will get "object should not be
> loaded twice\n".

In practice this is not going to be enough, though. The memlock error
only triggers on initial load if the limit is already exceeded (by other
BPF programs); but often what will happen is that the program being
loaded will have a map definition that's big enough to exhaust the
memlimit by itself. In which case the memlock error will trigger while
creating maps, not on initial probe.

Since you can't predict where the error will happen, you need to be
prepared to close the bpf object and start over anyway, so I'm not sure
it adds much value to move bpf_object__probe_loading() earlier?

-Toke

Re: [PATCH bpf-next v2] libbpf: fix probe code to return EPERM if encountered

[Andrii Nakryiko]

On Tue, May 5, 2020 at 3:07 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Mon, May 4, 2020 at 2:13 AM Eelco Chaudron <echaudro@redhat.com> wrote:
> >>
> >> When the probe code was failing for any reason ENOTSUP was returned, even
> >> if this was due to no having enough lock space. This patch fixes this by
> >> returning EPERM to the user application, so it can respond and increase
> >> the RLIMIT_MEMLOCK size.
> >>
> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> >> ---
> >> v2: Split bpf_object__probe_name() in two functions as suggested by Andrii
> >
> > Yeah, looks good, and this is good enough, so consider you have my
> > ack. But I think we can further improve the experience by:
> >
> > 1. Changing existing "Couldn't load basic 'r0 = 0' BPF program."
> > message to be something more meaningful and actionable for user. E.g.,
> >
> > "Couldn't load trivial BPF program. Make sure your kernel supports BPF
> > (CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is set to big enough
> > value."
> >
> > Then even complete kernel newbies can search for CONFIG_BPF_SYSCALL or
> > RLIMIT_MEMLOCK and hopefully find useful discussions. We can/should
> > add RLIMIT_MEMLOCK examples to some FAQ, probably as well (if it's not
> > there already).
>
> Always on board with improving documentation; and yeah I agree that
> "Couldn't load basic 'r0 = 0' BPF program." could be a bit friendlier ;)
>
> > 2. I'd do bpf_object__probe_loading() before obj->loaded is set, so
> > that user can have a loop of bpf_object__load() that bump
> > RLIMIT_MEMLOCK in steps. After setting obj->loaded = true, user won't
> > be able to attemp loading again and will get "object should not be
> > loaded twice\n".
>
> In practice this is not going to be enough, though. The memlock error
> only triggers on initial load if the limit is already exceeded (by other
> BPF programs); but often what will happen is that the program being
> loaded will have a map definition that's big enough to exhaust the
> memlimit by itself. In which case the memlock error will trigger while
> creating maps, not on initial probe.
>
> Since you can't predict where the error will happen, you need to be
> prepared to close the bpf object and start over anyway, so I'm not sure
> it adds much value to move bpf_object__probe_loading() earlier?
>

True that. Ok, sounds fine to me (error message would be still nice to
improve, though).

> -Toke
>

Re: [PATCH bpf-next v2] libbpf: fix probe code to return EPERM if encountered

[Eelco Chaudron]

On 4 May 2020, at 20:43, Andrii Nakryiko wrote:

> On Mon, May 4, 2020 at 2:13 AM Eelco Chaudron <echaudro@redhat.com> 
> wrote:
>>
>> When the probe code was failing for any reason ENOTSUP was returned, 
>> even
>> if this was due to no having enough lock space. This patch fixes this 
>> by
>> returning EPERM to the user application, so it can respond and 
>> increase
>> the RLIMIT_MEMLOCK size.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> v2: Split bpf_object__probe_name() in two functions as suggested by 
>> Andrii
>
> Yeah, looks good, and this is good enough, so consider you have my
> ack. But I think we can further improve the experience by:
>
> 1. Changing existing "Couldn't load basic 'r0 = 0' BPF program."
> message to be something more meaningful and actionable for user. E.g.,
>
> "Couldn't load trivial BPF program. Make sure your kernel supports BPF
> (CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is set to big enough
> value."

I had pr_perm_msg() in the previous patch, but I forgot to put it back 
in :(
However your message looks way better, so I plan to send a v3 with the 
following:

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6838e6d431ce..ad3043c5db13 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3170,8 +3170,10 @@ bpf_object__probe_loading(struct bpf_object *obj)
         ret = bpf_load_program_xattr(&attr, NULL, 0);
         if (ret < 0) {
                 cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
-               pr_warn("Error in %s():%s(%d). Couldn't load basic 'r0 = 
0' BPF program.\n",
-                       __func__, cp, errno);
+               pr_warn("Error in %s():%s(%d). Couldn't load trivial BPF 
"
+                       "program. Make sure your kernel supports BPF "
+                       "(CONFIG_BPF_SYSCALL=y) and/or that 
RLIMIT_MEMLOCK is "
+                       "set to big enough value.\n", __func__, cp, 
errno);
                 return -errno;
         }
         close(ret);

> Then even complete kernel newbies can search for CONFIG_BPF_SYSCALL or
> RLIMIT_MEMLOCK and hopefully find useful discussions. We can/should
> add RLIMIT_MEMLOCK examples to some FAQ, probably as well (if it's not
> there already).

The xdp-tutorial repo has examples on how to set it to unlimited ;)
Also, the xdp-tool’s repo has some examples on how to dynamically try 
to increase it, for examples:

http://172.16.1.201:8080/source/xref/xdp-tools/xdp-loader/xdp-loader.c?r=1926fc3d#198

> 2. I'd do bpf_object__probe_loading() before obj->loaded is set, so
> that user can have a loop of bpf_object__load() that bump
> RLIMIT_MEMLOCK in steps. After setting obj->loaded = true, user won't
> be able to attemp loading again and will get "object should not be
> loaded twice\n".

Guess this was acked in Toke’s thread.

> [...]