Re: [PATCH bpf] libbpf: fix reuse of pinned map on older kernel

[Martynas Pumputis <m@lambda.lt>]

On 7/8/21 12:58 AM, Andrii Nakryiko wrote:
> On Tue, Jul 6, 2021 at 10:24 AM Martynas Pumputis <m@lambda.lt> wrote:
>>
>> When loading a BPF program with a pinned map, the loader checks whether
>> the pinned map can be reused, i.e. their properties match. To derive
>> such of the pinned map, the loader invokes BPF_OBJ_GET_INFO_BY_FD and
>> then does the comparison.
>>
>> Unfortunately, on < 4.12 kernels the BPF_OBJ_GET_INFO_BY_FD is not
>> available, so loading the program fails with the following error:
>>
>>          libbpf: failed to get map info for map FD 5: Invalid argument
>>          libbpf: couldn't reuse pinned map at
>>                  '/sys/fs/bpf/tc/globals/cilium_call_policy': parameter
>>                  mismatch"
>>          libbpf: map 'cilium_call_policy': error reusing pinned map
>>          libbpf: map 'cilium_call_policy': failed to create:
>>                  Invalid argument(-22)
>>          libbpf: failed to load object 'bpf_overlay.o'
>>
>> To fix this, probe the kernel for BPF_OBJ_GET_INFO_BY_FD support. If it
>> doesn't support, then fallback to derivation of the map properties via
>> /proc/$PID/fdinfo/$MAP_FD.
>>
>> Signed-off-by: Martynas Pumputis <m@lambda.lt>
>> ---
>>   tools/lib/bpf/libbpf.c | 103 +++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 92 insertions(+), 11 deletions(-)
>>
> 
> [...]
> 
>> @@ -4406,10 +4478,19 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
>>
>>          map_info_len = sizeof(map_info);
>>
>> -       if (bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len)) {
>> -               pr_warn("failed to get map info for map FD %d: %s\n",
>> -                       map_fd, libbpf_strerror_r(errno, msg, sizeof(msg)));
>> -               return false;
>> +       if (kernel_supports(obj, FEAT_OBJ_GET_INFO_BY_FD)) {
> 
> why not just try bpf_obj_get_info_by_fd() first, and if it fails
> always fallback to bpf_get_map_info_from_fdinfo(). No need to do
> feature detection. This will cut down on the amount of code without
> any regression in behavior. More so, it will actually now be
> consistent and good behavior in case of bpf_map__reuse_fd() where we
> don't have obj. WDYT?

I was thinking about it, but then decided to use the kernel probing 
instead. The reasoning was the following:

1) For programs with many pinned maps we would issue many failing 
BPF_OBJ_GET_INFO_BY_FD calls (instead of a single one) which might 
hinder the performance.
2) A canonical way in libbpf to detect features is via kernel_supports() 
and friends, so I didn't want to diverge there.

Re bpf_map__reuse_fd(), if we are OK to break the API before libbpf 
v1.0, then we could extend bpf_map__reuse_fd() to accept the obj. 
However, this would break some consumers of the lib, e.g., iproute2 [1].

Anyway, if you think that we can ignore 1) and 2), then I'm happy to 
change. Also, I'm going to submit to bpf-next.

[1]: 
https://github.com/shemminger/iproute2/blob/v5.11.0/lib/bpf_libbpf.c#L98

> 
> 
>> +               if (bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len)) {
>> +                       pr_warn("failed to get map info for map FD %d: %s\n",
>> +                               map_fd,
>> +                               libbpf_strerror_r(errno, msg, sizeof(msg)));
>> +                       return false;
>> +               }
> 
> [...]
>