On 04/08/2022 07:30, Hangbin Liu wrote:
> On Tue, Jun 21, 2022 at 10:28:27PM +0100, Quentin Monnet wrote:
>> Hi Hangbin,
>>
>> No plan currently. Adding names has been suggested before, but it's
>> not compatible with some older kernels that don't support map names
>> [0]. Maybe one solution would be to probe the kernel for map name
>> support, and to add a name if supported.
>
> Hi Quentin,
>
> I looked into this issue this week. And I have some questions.
> Can we just use the probe_kern_prog_name() function directly? e.g.
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index e89cc9c885b3..f7d1580cd54e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4476,7 +4476,10 @@ static int probe_kern_global_data(void)
> };
> int ret, map, insn_cnt = ARRAY_SIZE(insns);
>
> - map = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), 32, 1, NULL);
> + if (probe_kern_prog_name() > 0)
> + map = bpf_map_create(BPF_MAP_TYPE_ARRAY, "global_data", sizeof(int), 32, 1, NULL);
> + else
> + map = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), 32, 1, NULL);
> if (map < 0) {
> ret = -errno;
> cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
>
> I know the map name and prog name supports are not in the same patch. But they are
> added to kernel in one patch series. I doubt any one will backport them separately.
Hi! It would look much cleaner to have something specific to map names.
It does not have to be a dedicated probe in my opinion, maybe we can
just try loading with a name and retry if this fails with -EINVAL (a bit
like we retry with another prog type in bpf_object__probe_loading(), if
the first one fails). Something like this (not tested):
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 50d41815f431..abcafdf8ae7e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4430,7 +4430,10 @@ static int probe_kern_global_data(void)
};
int ret, map, insn_cnt = ARRAY_SIZE(insns);
- map = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), 32, 1, NULL);
+ map = bpf_map_create(BPF_MAP_TYPE_ARRAY, "global_data", sizeof(int), 32, 1, NULL);
+ if (map < 0 && errno == EINVAL)
+ /* Retry without name */
+ map = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), 32, 1, NULL);
if (map < 0) {
ret = -errno;
cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
(Maybe with a small wrapper, given that we'd also need this in
probe_prog_bind_map() and probe_kern_array_mmap() as well.)
> And I also have a question about function probe_kern_prog_name(). I only
> saw it created a prog with name "test". But I didn't find the function check
> if the prog are really has name "test". If a old kernel doesn't support prog
> name, I think it will just ignore the name field. No?
No, "if (CHECK_ATTR(BPF_PROG_LOAD))" should fail in bpf_prog_load() in
kernel/bpf/syscall.c, and the syscall should fail with -EINVAL.
If older kernels simply ignored the "name" field for programs and maps,
we wouldn't have to probe or retry for the current case in the first
place :).
> Another way I think we can use to probe if kernel supports map name is try
> to attach a kprobe/bpf_obj_name_cpy. If attach success, the kernel should support
> the prog/map name. WDYT?
It's probably easier to try to load a map with a name. Also kprobes can be
disabled, if I remember correctly.
Thanks,
Quentin
