netdev - Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by

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: <CAEf4BzatvJORZvkz37_XJxvk5+Amr8V8iHq=1_4k_uCz0fE-eQ@mail.gmail.com>
Date:   Thu, 22 Jul 2021 17:48:13 -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>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()

On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@...valent.com> wrote:
>
> Replace the calls to deprecated function btf__get_from_id() with calls
> to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
> Update the surrounding code accordingly (instead of passing a pointer to
> the btf struct, get it as a return value from the function). Also make
> sure that btf__free() is called on the pointer after use.
>
> v2:
> - Given that btf__load_from_kernel_by_id() has changed since v1, adapt
>   the code accordingly instead of just renaming the function. Also add a
>   few calls to btf__free() when necessary.
>
> Signed-off-by: Quentin Monnet <quentin@...valent.com>
> Acked-by: John Fastabend <john.fastabend@...il.com>
> ---
>  tools/bpf/bpftool/btf.c                      |  8 ++----
>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
>  tools/bpf/bpftool/map.c                      | 16 +++++------
>  tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
>  tools/perf/util/bpf-event.c                  | 11 ++++----
>  tools/perf/util/bpf_counter.c                | 12 ++++++--
>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
>  7 files changed, 51 insertions(+), 35 deletions(-)
>

[...]

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 09ae0381205b..12787758ce03 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
>                 }
>                 return btf_vmlinux;
>         } else if (info->btf_value_type_id) {
> -               int err;
> -
> -               err = btf__get_from_id(info->btf_id, &btf);
> -               if (err || !btf) {
> +               btf = btf__load_from_kernel_by_id(info->btf_id);
> +               if (libbpf_get_error(btf)) {
>                         p_err("failed to get btf");
> -                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
> +                       if (!btf)
> +                               btf = ERR_PTR(-ESRCH);

why not do a simpler (less conditionals)

err = libbpf_get_error(btf);
if (err) {
    btf = ERR_PTR(err);
}

?

>                 }
>         }
>
> @@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>                             void *value)
>  {
>         json_writer_t *btf_wtr;
> -       struct btf *btf = NULL;
> -       int err;
> +       struct btf *btf;
>
> -       err = btf__get_from_id(info->btf_id, &btf);
> -       if (err) {
> +       btf = btf__load_from_kernel_by_id(info->btf_id);
> +       if (libbpf_get_error(btf)) {
>                 p_err("failed to get btf");
>                 return;
>         }

[...]

>
>         func_info = u64_to_ptr(info->func_info);
> @@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>                 kernel_syms_destroy(&dd);
>         }
>
> +       btf__free(btf);
> +

warrants a Fixes: tag?

>         return 0;
>  }
>
> @@ -2002,8 +2007,8 @@ static char *profile_target_name(int tgt_fd)
>         struct bpf_prog_info_linear *info_linear;
>         struct bpf_func_info *func_info;
>         const struct btf_type *t;
> +       struct btf *btf = NULL;
>         char *name = NULL;
> -       struct btf *btf;
>
>         info_linear = bpf_program__get_prog_info_linear(
>                 tgt_fd, 1UL << BPF_PROG_INFO_FUNC_INFO);
> @@ -2012,12 +2017,17 @@ static char *profile_target_name(int tgt_fd)
>                 return NULL;
>         }
>
> -       if (info_linear->info.btf_id == 0 ||
> -           btf__get_from_id(info_linear->info.btf_id, &btf)) {
> +       if (info_linear->info.btf_id == 0) {
>                 p_err("prog FD %d doesn't have valid btf", tgt_fd);
>                 goto out;
>         }
>
> +       btf = btf__load_from_kernel_by_id(info_linear->info.btf_id);
> +       if (libbpf_get_error(btf)) {
> +               p_err("failed to load btf for prog FD %d", tgt_fd);
> +               goto out;
> +       }
> +
>         func_info = u64_to_ptr(info_linear->info.func_info);
>         t = btf__type_by_id(btf, func_info[0].type_id);
>         if (!t) {
> @@ -2027,6 +2037,7 @@ static char *profile_target_name(int tgt_fd)
>         }
>         name = strdup(btf__name_by_offset(btf, t->name_off));
>  out:
> +       btf__free(btf);

and another Fixes? :) and two more below

>         free(info_linear);
>         return name;
>  }

[...]