netdev - Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <997410b6-7428-173c-8197-ac9eae036e34@isovalent.com>
Date:   Wed, 9 Sep 2020 17:59:29 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump
 map entry

On 09/09/2020 17:46, Andrii Nakryiko wrote:
> On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@...valent.com> wrote:
>>
>> On 09/09/2020 17:30, Andrii Nakryiko wrote:
>>> On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@...valent.com> wrote:
>>>>
>>>> On 09/09/2020 04:25, Andrii Nakryiko wrote:
>>>>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@...valent.com> wrote:
>>>>>>
>>>>>> The function used to dump a map entry in bpftool is a bit difficult to
>>>>>> follow, as a consequence to earlier refactorings. There is a variable
>>>>>> ("num_elems") which does not appear to be necessary, and the error
>>>>>> handling would look cleaner if moved to its own function. Let's clean it
>>>>>> up. No functional change.
>>>>>>
>>>>>> v2:
>>>>>> - v1 was erroneously removing the check on fd maps in an attempt to get
>>>>>>   support for outer map dumps. This is already working. Instead, v2
>>>>>>   focuses on cleaning up the dump_map_elem() function, to avoid
>>>>>>   similar confusion in the future.
>>>>>>
>>>>>> Signed-off-by: Quentin Monnet <quentin@...valent.com>
>>>>>> ---
>>>>>>  tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++-------------------
>>>>>>  1 file changed, 52 insertions(+), 49 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>>>> index bc0071228f88..c8159cb4fb1e 100644
>>>>>> --- a/tools/bpf/bpftool/map.c
>>>>>> +++ b/tools/bpf/bpftool/map.c
>>>>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>>>>>>         jsonw_end_object(json_wtr);
>>>>>>  }
>>>>>>
>>>>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>>>>>> -                             const char *error_msg)
>>>>>> +static void
>>>>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key,
>>>>>> +                     const char *error_msg)
>>>>>>  {
>>>>>>         int msg_size = strlen(error_msg);
>>>>>>         bool single_line, break_names;
>>>>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>>>>>>         printf("\n");
>>>>>>  }
>>>>>>
>>>>>> +static void
>>>>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
>>>>>> +{
>>>>>> +       /* For prog_array maps or arrays of maps, failure to lookup the value
>>>>>> +        * means there is no entry for that key. Do not print an error message
>>>>>> +        * in that case.
>>>>>> +        */
>>>>>
>>>>> this is the case when error is ENOENT, all the other ones should be
>>>>> treated the same, no?
>>>>
>>>> Do you mean all map types should be treated the same? If so, I can
>>>> remove the check below, as in v1. Or do you mean there is a missing
>>>> check on the error value? In which case I can extend this check to
>>>> verify we have ENOENT.
>>>
>>> The former, probably. I don't see how map-in-map is different for
>>> lookups and why it needs special handling.
>>
>> I didn't find a particular reason in the logs. My guess is that they may
>> be more likely to have "empty" entries than other types, and that it
>> might be more difficult to spot the existing entries in the middle of a
>> list of "<no entry>" messages.
>>
>> But I agree, let's get rid of this special case and have the same output
>> for all types. I'll respin.
> 
> Oh, wait, I think what I had in mind is to special case ENOENT for
> map-in-map and just skip those. So yeah, sorry, there is still a bit
> of a special handling, but **only** for -ENOENT. When I was replying I
> forgot bpftool emits "<no entry>" for each -ENOENT by default.

So do you prefer me to extend the check with errno == -ENOENT? Or shall
I remove it entirely and just have the "<no entry>" messages like for
the other map types?

Quentin