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

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

[Martynas Pumputis]

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(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ac882e1..f3daed3 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -193,6 +193,8 @@ enum kern_feature_id {
 	FEAT_MODULE_BTF,
 	/* BTF_KIND_FLOAT support */
 	FEAT_BTF_FLOAT,
+	/* BPF_OBJ_GET_INFO_BY_FD support */
+	FEAT_OBJ_GET_INFO_BY_FD,
 	__FEAT_CNT,
 };
 
@@ -3920,14 +3922,54 @@ static int bpf_map_find_btf_info(struct bpf_object *obj, struct bpf_map *map)
 	return 0;
 }
 
-int bpf_map__reuse_fd(struct bpf_map *map, int fd)
+static int bpf_get_map_info_from_fdinfo(int fd, struct bpf_map_info *info)
+{
+	char file[PATH_MAX], buff[4096];
+	FILE *fp;
+	__u32 val;
+	int err;
+
+	snprintf(file, sizeof(file), "/proc/%d/fdinfo/%d", getpid(), fd);
+	memset(info, 0, sizeof(*info));
+
+	fp = fopen(file, "r");
+	if (!fp) {
+		err = -errno;
+		pr_warn("failed to open %s: %d. No procfs support?\n", file,
+			err);
+		return err;
+	}
+
+	while (fgets(buff, sizeof(buff), fp)) {
+		if (sscanf(buff, "map_type:\t%u", &val) == 1)
+			info->type = val;
+		else if (sscanf(buff, "key_size:\t%u", &val) == 1)
+			info->key_size = val;
+		else if (sscanf(buff, "value_size:\t%u", &val) == 1)
+			info->value_size = val;
+		else if (sscanf(buff, "max_entries:\t%u", &val) == 1)
+			info->max_entries = val;
+		else if (sscanf(buff, "map_flags:\t%i", &val) == 1)
+			info->map_flags = val;
+	}
+
+	fclose(fp);
+
+	return 0;
+}
+
+static int bpf_map__reuse_fd_safe(struct bpf_object *obj, struct bpf_map *map,
+				  int fd)
 {
 	struct bpf_map_info info = {};
 	__u32 len = sizeof(info);
 	int new_fd, err;
 	char *new_name;
 
-	err = bpf_obj_get_info_by_fd(fd, &info, &len);
+	if (obj == NULL || kernel_supports(obj, FEAT_OBJ_GET_INFO_BY_FD))
+		err = bpf_obj_get_info_by_fd(fd, &info, &len);
+	else
+		err = bpf_get_map_info_from_fdinfo(fd, &info);
 	if (err)
 		return libbpf_err(err);
 
@@ -3974,6 +4016,11 @@ err_free_new_name:
 	return libbpf_err(err);
 }
 
+int bpf_map__reuse_fd(struct bpf_map *map, int fd)
+{
+	return bpf_map__reuse_fd_safe(NULL, map, fd);
+}
+
 __u32 bpf_map__max_entries(const struct bpf_map *map)
 {
 	return map->def.max_entries;
@@ -4320,6 +4367,27 @@ static int probe_module_btf(void)
 	return !err;
 }
 
+static int probe_kern_bpf_get_info_by_fd(void)
+{
+	int fd, err;
+	__u32 len;
+	struct bpf_map_info info;
+	struct bpf_create_map_attr attr = {
+		.map_type = BPF_MAP_TYPE_ARRAY,
+		.key_size = sizeof(int),
+		.value_size = sizeof(int),
+		.max_entries = 1,
+	};
+
+	fd = bpf_create_map_xattr(&attr);
+	if (fd < 0)
+		return 0;
+
+	err = bpf_obj_get_info_by_fd(fd, &info, &len);
+	close(fd);
+	return !err;
+}
+
 enum kern_feature_result {
 	FEAT_UNKNOWN = 0,
 	FEAT_SUPPORTED = 1,
@@ -4370,6 +4438,9 @@ static struct kern_feature_desc {
 	[FEAT_BTF_FLOAT] = {
 		"BTF_KIND_FLOAT support", probe_kern_btf_float,
 	},
+	[FEAT_OBJ_GET_INFO_BY_FD] = {
+		"BPF_OBJ_GET_INFO_BY_FD support", probe_kern_bpf_get_info_by_fd,
+	},
 };
 
 static bool kernel_supports(const struct bpf_object *obj, enum kern_feature_id feat_id)
@@ -4398,7 +4469,8 @@ static bool kernel_supports(const struct bpf_object *obj, enum kern_feature_id f
 	return READ_ONCE(feat->res) == FEAT_SUPPORTED;
 }
 
-static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
+static bool map_is_reuse_compat(struct bpf_object *obj,
+				const struct bpf_map *map, int map_fd)
 {
 	struct bpf_map_info map_info = {};
 	char msg[STRERR_BUFSIZE];
@@ -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)) {
+		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;
+		}
+	} else {
+		if (bpf_get_map_info_from_fdinfo(map_fd, &map_info)) {
+			pr_warn("failed to get map info for fdinfo: %s\n",
+				libbpf_strerror_r(errno, msg, sizeof(msg)));
+			return false;
+		}
 	}
 
 	return (map_info.type == map->def.type &&
@@ -4420,7 +4501,7 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
 }
 
 static int
-bpf_object__reuse_map(struct bpf_map *map)
+bpf_object__reuse_map(struct bpf_object *obj, struct bpf_map *map)
 {
 	char *cp, errmsg[STRERR_BUFSIZE];
 	int err, pin_fd;
@@ -4440,14 +4521,14 @@ bpf_object__reuse_map(struct bpf_map *map)
 		return err;
 	}
 
-	if (!map_is_reuse_compat(map, pin_fd)) {
+	if (!map_is_reuse_compat(obj, map, pin_fd)) {
 		pr_warn("couldn't reuse pinned map at '%s': parameter mismatch\n",
 			map->pin_path);
 		close(pin_fd);
 		return -EINVAL;
 	}
 
-	err = bpf_map__reuse_fd(map, pin_fd);
+	err = bpf_map__reuse_fd_safe(obj, map, pin_fd);
 	if (err) {
 		close(pin_fd);
 		return err;
@@ -4643,7 +4724,7 @@ bpf_object__create_maps(struct bpf_object *obj)
 		map = &obj->maps[i];
 
 		if (map->pin_path) {
-			err = bpf_object__reuse_map(map);
+			err = bpf_object__reuse_map(obj, map);
 			if (err) {
 				pr_warn("map '%s': error reusing pinned map\n",
 					map->name);
-- 
2.32.0

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

[Song Liu]

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>

The code looks good to me. Except a checkpatch CHECK:

CHECK: Comparison to NULL could be written "!obj"
#96: FILE: tools/lib/bpf/libbpf.c:3943:
+ if (obj == NULL || kernel_supports(obj, FEAT_OBJ_GET_INFO_BY_FD))

Also, I think this should target bpf-next tree?

Thanks,
Song

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

[Martynas Pumputis]

On 7/7/21 1:32 AM, Song Liu 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>
> 
> The code looks good to me. Except a checkpatch CHECK:
> 
> CHECK: Comparison to NULL could be written "!obj"
> #96: FILE: tools/lib/bpf/libbpf.c:3943:
> + if (obj == NULL || kernel_supports(obj, FEAT_OBJ_GET_INFO_BY_FD))

Thanks for the review. I will send v2 with the fix.

> 
> Also, I think this should target bpf-next tree?

Considering that libbpf is supported on older kernels, w/o this patch it 
is impossible to use it on < 4.12 kernels for programs with pinned maps. 
Therefore, I think that this is a fix and thus it should target the bpf 
tree instead.

> 
> Thanks,
> Song
> 

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

[Andrii Nakryiko]

On Wed, Jul 7, 2021 at 3:36 AM Martynas Pumputis <m@lambda.lt> wrote:
>
>
>
> On 7/7/21 1:32 AM, Song Liu 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>
> >
> > The code looks good to me. Except a checkpatch CHECK:
> >
> > CHECK: Comparison to NULL could be written "!obj"
> > #96: FILE: tools/lib/bpf/libbpf.c:3943:
> > + if (obj == NULL || kernel_supports(obj, FEAT_OBJ_GET_INFO_BY_FD))
>
> Thanks for the review. I will send v2 with the fix.
>
> >
> > Also, I think this should target bpf-next tree?
>
> Considering that libbpf is supported on older kernels, w/o this patch it
> is impossible to use it on < 4.12 kernels for programs with pinned maps.
> Therefore, I think that this is a fix and thus it should target the bpf
> tree instead.

It does feel like a feature of supporting even older kernels. There
was nothing broken before, libbpf just didn't know how to get
information to validate map info. Now it knows how to do it for older
kernels. So I'd still route it through bpf-next tree.

>
> >
> > Thanks,
> > Song
> >

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

[Andrii Nakryiko]

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?


> +               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;
> +               }

[...]

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

[Martynas Pumputis]

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;
>> +               }
> 
> [...]
> 

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

[Toke Høiland-Jørgensen]

Martynas Pumputis <m@lambda.lt> writes:

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

"Might" hinder the performance? Did you measure this? We're usually
talking (at most) dozens of maps per object file, not thousands; also,
this would only be incurred if the initial call actually fails (i.e., on
old kernels).

> 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].

IMO, this does not sound like something worth breaking the API
compatibility for...

-Toke

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

[Andrii Nakryiko]

On Thu, Jul 8, 2021 at 4:43 AM Martynas Pumputis <m@lambda.lt> wrote:
>
>
>
> 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.

you can't have so many maps per BPF program to really notice
performance drop for doing an almost no-op bpf() syscall, so I find
this a weak argument

> 2) A canonical way in libbpf to detect features is via kernel_supports()
> and friends, so I didn't want to diverge there.

There are places where we just gracefully handle missing features.
E.g., when loading map with BTF and it fails, we'll retry without BTF.
Or in __perf_buffer__new we allow BPF_OBJ_GET_INFO_BY_FD to fail.

I'd start with a simple fallback and do explicit feature detection
later if it turns out to be problematic.

>
> 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].

No, we are not ok to just arbitrarily break API. And especially that
passing obj to bpf_map API seems weird. We could solve this problem by
remembering the bpf_object pointer inside struct bpf_map, just like we
do for struct bpf_program, but again, I'm not sure it's worth it in
this case. But strategy doesn't help perf_buffer__new() case.

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

I'd like bpf_map__reuse_fd() to have consistent behavior. And avoid
extra code if possible, so let's try a simple fallback for now?

>
> [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;
> >> +               }
> >
> > [...]
> >