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

[Daniel Borkmann <daniel@iogearbox.net>]

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@redhat.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