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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <292BB732-5974-48E4-91EC-9482EF0E4600@redhat.com>
Date:   Wed, 06 May 2020 10:57:24 +0200
From:   "Eelco Chaudron" <echaudro@...hat.com>
To:     "Andrii Nakryiko" <andrii.nakryiko@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>,
        "Alexei Starovoitov" <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        "Martin Lau" <kafai@...com>, "Song Liu" <songliubraving@...com>,
        "Yonghong Song" <yhs@...com>, "Andrii Nakryiko" <andriin@...com>,
        "Toke Høiland-Jørgensen" <toke@...hat.com>
Subject: Re: [PATCH bpf-next v2] libbpf: fix probe code to return EPERM if
 encountered



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

> On Mon, May 4, 2020 at 2:13 AM Eelco Chaudron <echaudro@...hat.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@...hat.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.

> [...]