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

Message-ID: <CAEf4BzbgGSS6p5Xyx6Sp34hLZQ8XwQN7Fg6ykPZ5VHFw6doUJw@mail.gmail.com>
Date:   Fri, 22 Oct 2021 17:54:41 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Quentin Monnet <quentin@...valent.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

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

>  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?

> +{
> +       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?

> +{
> +       return (void *)(uintptr_t)x;
> +}
> +
> +static inline bool hashmap__empty(struct hashmap *map)
> +{
> +       return map ? hashmap__size(map) == 0 : true;
> +}
> +
>  #endif

[...]