netdev - Re: [PATCH bpf-next v3] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <625c343a-6554-a6e9-fb04-ca0a80554a91@iogearbox.net>
Date:   Mon, 11 May 2020 23:08:27 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Yonghong Song <yhs@...com>, Eelco Chaudron <echaudro@...hat.com>,
        bpf@...r.kernel.org
Cc:     davem@...emloft.net, netdev@...r.kernel.org, ast@...nel.org,
        kafai@...com, songliubraving@...com, andriin@...com,
        toke@...hat.com
Subject: Re: [PATCH bpf-next v3] libbpf: fix probe code to return EPERM if
 encountered

On 5/11/20 10:43 PM, Yonghong Song wrote:
> On 5/11/20 5:40 AM, Eelco Chaudron 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>
>> ---
>> v3: Updated error message to be more specific as suggested by Andrii
>> v2: Split bpf_object__probe_name() in two functions as suggested by Andrii
>>
>>   tools/lib/bpf/libbpf.c |   31 ++++++++++++++++++++++++++-----
>>   1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 8f480e29a6b0..ad3043c5db13 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];
>> @@ -3170,14 +3170,34 @@ bpf_object__probe_name(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;
> 
> Just curious. Did "errno" always survive pr_warn() here? pr_warn() may call user supplied print function which it outside libbpf control.
> Maybe should cache errno before calling pr_warn()?

+1, I think right now it's a bit of a mess in libbpf. Plenty of cases where we cache errno
before pr_warn() and plenty of cases where we don't. I think we should avoid any surprises
and do cache it on these occasions everywhere. Maybe a cocci script would help to fix the
remaining sites for good.

Thanks,
Daniel