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]

Date:   Wed, 9 Sep 2020 09:19:37 +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 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.

>> +       if (map_is_map_of_maps(map_info->type) ||
>> +           map_is_map_of_progs(map_info->type))
>> +               return;
>> +
>> +       if (json_output) {
>> +               jsonw_start_object(json_wtr);   /* entry */
>> +               jsonw_name(json_wtr, "key");
>> +               print_hex_data_json(key, map_info->key_size);
>> +               jsonw_name(json_wtr, "value");
>> +               jsonw_start_object(json_wtr);   /* error */
>> +               jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
>> +               jsonw_end_object(json_wtr);     /* error */
>> +               jsonw_end_object(json_wtr);     /* entry */
>> +       } else {
>> +               const char *msg = NULL;
>> +
>> +               if (lookup_errno == ENOENT)
>> +                       msg = "<no entry>";
>> +               else if (lookup_errno == ENOSPC &&
>> +                        map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
>> +                       msg = "<cannot read>";
>> +
>> +               print_entry_error_msg(map_info, key,
>> +                                     msg ? : strerror(lookup_errno));
>> +       }
>> +}
>> +
> 
> [...]
>