netdev - Re: [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects

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] [day] [month] [year] [list]

Date:   Sat, 23 Oct 2021 21:51:19 +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>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 3/5] bpftool: Switch to libbpf's hashmap for
 pinned paths of BPF objects

2021-10-22 17:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> On Fri, Oct 22, 2021 at 10:16 AM Quentin Monnet <quentin@...valent.com> wrote:
>>
>> In order to show pinned paths for BPF programs, maps, or links when
>> listing them with the "-f" option, bpftool creates hash maps to store
>> all relevant paths under the bpffs. So far, it would rely on the
>> kernel implementation (from tools/include/linux/hashtable.h).
>>
>> We can make bpftool rely on libbpf's implementation instead. The
>> motivation is to make bpftool less dependent of kernel headers, to ease
>> the path to a potential out-of-tree mirror, like libbpf has.
>>
>> This commit is the first step of the conversion: the hash maps for
>> pinned paths for programs, maps, and links are converted to libbpf's
>> hashmap.{c,h}. Other hash maps used for the PIDs of process holding
>> references to BPF objects are left unchanged for now. On the build side,
>> this requires adding a dependency to a second header internal to libbpf,
>> and making it a dependency for the bootstrap bpftool version as well.
>> The rest of the changes are a rather straightforward conversion.
>>
>> Signed-off-by: Quentin Monnet <quentin@...valent.com>
>> ---
>>  tools/bpf/bpftool/Makefile |  8 +++---
>>  tools/bpf/bpftool/common.c | 50 ++++++++++++++++++++------------------
>>  tools/bpf/bpftool/link.c   | 35 ++++++++++++++------------
>>  tools/bpf/bpftool/main.h   | 29 +++++++++++++---------
>>  tools/bpf/bpftool/map.c    | 35 ++++++++++++++------------
>>  tools/bpf/bpftool/prog.c   | 35 ++++++++++++++------------
>>  6 files changed, 105 insertions(+), 87 deletions(-)
>>
> 
> [...]
> 
>> @@ -420,28 +421,20 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb,
>>         if (bpf_obj_get_info_by_fd(fd, &pinned_info, &len))
>>                 goto out_close;
>>
>> -       obj_node = calloc(1, sizeof(*obj_node));
>> -       if (!obj_node) {
>> +       path = strdup(fpath);
>> +       if (!path) {
>>                 err = -1;
>>                 goto out_close;
>>         }
>>
>> -       obj_node->id = pinned_info.id;
>> -       obj_node->path = strdup(fpath);
>> -       if (!obj_node->path) {
>> -               err = -1;
>> -               free(obj_node);
>> -               goto out_close;
>> -       }
>> -
>> -       hash_add(build_fn_table->table, &obj_node->hash, obj_node->id);
>> +       hashmap__append(build_fn_table, u32_as_hash_field(pinned_info.id), path);
> 
> handle errors? operation can fail

Right, I missed it for hashmap__append(). I'll address everywhere.

> 
>>  out_close:
>>         close(fd);
>>  out_ret:
>>         return err;
>>  }
> 
> [...]
> 
>>
>>  unsigned int get_page_size(void)
>> @@ -962,3 +956,13 @@ int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len)
>>
>>         return fd;
>>  }
>> +
>> +size_t bpftool_hash_fn(const void *key, void *ctx)
>> +{
>> +       return (size_t)key;
>> +}
>> +
>> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx)
> 
> kind of too generic and too assuming function (hash_fn and
> equal_fn)... Maybe either use static functions near each hashmap use
> case, or name it to specify that it works when keys are ids?

Makes sense. I'll probably just rename them in that case, because the
functions are the same for all hash maps and I don't really feel like
having five copies of it.

> 
>> +{
>> +       return k1 == k2;
>> +}
> 
> [...]
> 
>> @@ -256,4 +247,18 @@ int do_filter_dump(struct tcmsg *ifinfo, struct nlattr **tb, const char *kind,
>>
>>  int print_all_levels(__maybe_unused enum libbpf_print_level level,
>>                      const char *format, va_list args);
>> +
>> +size_t bpftool_hash_fn(const void *key, void *ctx);
>> +bool bpftool_equal_fn(const void *k1, const void *k2, void *ctx);
>> +
>> +static inline void *u32_as_hash_field(__u32 x)
> 
> it's used for keys only, right? so u32_as_hash_key?

Yes for this patch, but we're calling it on a value in the following
patch, in build_btf_type_table(), to store the ID of a program or a map
associated to a given BTF object ID.

So I'll keep the current name here for v2, but I'll address all your
other comments.

Thanks for the review,
Quentin