[bpf-next,1/2] tools: bpftool: dump outer maps content

Although user space can lookup and dump the content of an outer map
(hash-of-maps or array-of-maps), bpftool does not allow to do so.

It seems that the only reason for that is historical. Lookups for outer
maps was added in commit 14dc6f04f49d ("bpf: Add syscall lookup support
for fd array and htab"), and although the relevant code in bpftool had
not been merged yet, I suspect it had already been written with the
assumption that user space could not read outer maps.

Let's remove the restriction, dump for outer maps works with no further
change.

Reported-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/map.c | 4 ----
 1 file changed, 4 deletions(-)

On Fri, Sep 4, 2020 at 9:14 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Although user space can lookup and dump the content of an outer map
> (hash-of-maps or array-of-maps), bpftool does not allow to do so.
>
> It seems that the only reason for that is historical. Lookups for outer
> maps was added in commit 14dc6f04f49d ("bpf: Add syscall lookup support
> for fd array and htab"), and although the relevant code in bpftool had
> not been merged yet, I suspect it had already been written with the
> assumption that user space could not read outer maps.
>
> Let's remove the restriction, dump for outer maps works with no further
> change.
>
> Reported-by: Martynas Pumputis <m@lambda.lt>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/map.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index bc0071228f88..cb3a75eb5531 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -739,10 +739,6 @@ static int dump_map_elem(int fd, void *key, void *value,
>         /* lookup error handling */
>         lookup_errno = errno;
>
> -       if (map_is_map_of_maps(map_info->type) ||
> -           map_is_map_of_progs(map_info->type))
> -               return 0;
> -

this code path handles the error case when lookup fails, or am I
misreading it? It's fine to remove this restriction, but the commit
message is completely misleading. That whole dump_map_elem() code is a
bit confusing. E.g., what's the purpose of num_elems there?..

Also, can you please update the commit message with how the output
looks like for map-of-maps with your change?

>         if (json_output) {
>                 jsonw_start_object(json_wtr);
>                 jsonw_name(json_wtr, "key");
> --
> 2.25.1
>

On 04/09/2020 23:03, Andrii Nakryiko wrote:
> On Fri, Sep 4, 2020 at 9:14 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> Although user space can lookup and dump the content of an outer map
>> (hash-of-maps or array-of-maps), bpftool does not allow to do so.
>>
>> It seems that the only reason for that is historical. Lookups for outer
>> maps was added in commit 14dc6f04f49d ("bpf: Add syscall lookup support
>> for fd array and htab"), and although the relevant code in bpftool had
>> not been merged yet, I suspect it had already been written with the
>> assumption that user space could not read outer maps.
>>
>> Let's remove the restriction, dump for outer maps works with no further
>> change.
>>
>> Reported-by: Martynas Pumputis <m@lambda.lt>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> ---
>>  tools/bpf/bpftool/map.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index bc0071228f88..cb3a75eb5531 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -739,10 +739,6 @@ static int dump_map_elem(int fd, void *key, void *value,
>>         /* lookup error handling */
>>         lookup_errno = errno;
>>
>> -       if (map_is_map_of_maps(map_info->type) ||
>> -           map_is_map_of_progs(map_info->type))
>> -               return 0;
>> -
> 
> this code path handles the error case when lookup fails, or am I
> misreading it? It's fine to remove this restriction, but the commit
> message is completely misleading. That whole dump_map_elem() code is a
> bit confusing. E.g., what's the purpose of num_elems there?..
> 
> Also, can you please update the commit message with how the output
> looks like for map-of-maps with your change?

Ok, the function _is_ confusing and _I_ totally got confused. Dumping
outer maps is already supported and this patch is not needed (and as you
mentioned, it does not do what I expected and wrote). Sorry about that.

I'll send a clean-up for that function instead in v2, to avoid confusion
in the future. Thanks for the review!

Quentin