How about adding a name for bpftool self created maps?

How about adding a name for bpftool self created maps?

[Hangbin Liu]

Hi Quentin,

When I using `bpftool map list` to show what maps user using. bpftool will
show maps including self created maps. Apart from the "pid_iter.rodata",
there are also array maps without name, which makes it hard to filter out
what maps user using. e.g.

# bpftool map list
63: array  flags 0x0
         key 4B  value 32B  max_entries 1  memlock 4096B
65: array  name pid_iter.rodata  flags 0x480
         key 4B  value 4B  max_entries 1  memlock 4096B
         btf_id 98  frozen
         pids bpftool(10572)
66: array  flags 0x0
         key 4B  value 32B  max_entries 1  memlock 4096B

So do you have plan to add a special name for the bpftool self created maps?
Or do you know if there is a way to filter out these maps?

Thanks
Hangbin

Re: How about adding a name for bpftool self created maps?

[Quentin Monnet]

On Tue, 21 Jun 2022 at 03:09, Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Hi Quentin,
>
> When I using `bpftool map list` to show what maps user using. bpftool will
> show maps including self created maps. Apart from the "pid_iter.rodata",
> there are also array maps without name, which makes it hard to filter out
> what maps user using. e.g.
>
> # bpftool map list
> 63: array  flags 0x0
>          key 4B  value 32B  max_entries 1  memlock 4096B
> 65: array  name pid_iter.rodata  flags 0x480
>          key 4B  value 4B  max_entries 1  memlock 4096B
>          btf_id 98  frozen
>          pids bpftool(10572)
> 66: array  flags 0x0
>          key 4B  value 32B  max_entries 1  memlock 4096B
>
> So do you have plan to add a special name for the bpftool self created maps?
> Or do you know if there is a way to filter out these maps?

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.

Other than this I'm not aware of a reliable way to filter out these
maps at the moment. This could probably be done in bpftool since we
should have the ids of the self-generated maps. But I think I'd rather
find a way to add map names, if possible. It would make it easier to
recognise/filter these maps on regular listing, whereas a new option
would be harder for users to discover.

Quentin

[0] https://lore.kernel.org/bpf/CAEf4BzY66WPKQbDe74AKZ6nFtZjq5e+G3Ji2egcVytB9R6_sGQ@mail.gmail.com/

Re: How about adding a name for bpftool self created maps?

[Hangbin Liu]

On Tue, Jun 21, 2022 at 10:28:27PM +0100, Quentin Monnet wrote:
> > When I using `bpftool map list` to show what maps user using. bpftool will
> > show maps including self created maps. Apart from the "pid_iter.rodata",
> > there are also array maps without name, which makes it hard to filter out
> > what maps user using. e.g.
> >
> > # bpftool map list
> > 63: array  flags 0x0
> >          key 4B  value 32B  max_entries 1  memlock 4096B
> > 65: array  name pid_iter.rodata  flags 0x480
> >          key 4B  value 4B  max_entries 1  memlock 4096B
> >          btf_id 98  frozen
> >          pids bpftool(10572)
> > 66: array  flags 0x0
> >          key 4B  value 32B  max_entries 1  memlock 4096B
> >
> > So do you have plan to add a special name for the bpftool self created maps?
> > Or do you know if there is a way to filter out these maps?
> 
> 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,

Thanks for the reply. Probe the kernel and add name if supported makes
sense to me.

Thanks
Hangbin

> 
> Other than this I'm not aware of a reliable way to filter out these
> maps at the moment. This could probably be done in bpftool since we
> should have the ids of the self-generated maps. But I think I'd rather
> find a way to add map names, if possible. It would make it easier to
> recognise/filter these maps on regular listing, whereas a new option
> would be harder for users to discover.
> 
> Quentin
> 
> [0] https://lore.kernel.org/bpf/CAEf4BzY66WPKQbDe74AKZ6nFtZjq5e+G3Ji2egcVytB9R6_sGQ@mail.gmail.com/

Re: How about adding a name for bpftool self created maps?

[Hangbin Liu]

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.

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?

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?

Thanks
Hangbin

Re: How about adding a name for bpftool self created maps?

[Quentin Monnet]

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

Re: How about adding a name for bpftool self created maps?

[Hangbin Liu]

On Thu, Aug 04, 2022 at 11:01:48AM +0100, Quentin Monnet wrote:
> 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.)

Ah, this looks more clean and easier.

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

Thanks for the explanation. I will try add a wrapper first.

Regards
Hangbin