On Mon, Oct 11, 2021 at 1:20 AM Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
>
> To prepare for impending deprecation of libbpf's
> bpf_program__get_prog_info_linear, migrate uses of this function to use
> bpf_obj_get_info_by_fd.
>
> Since the profile_target_name and dump_prog_id_as_func_ptr helpers were
> only looking at the first func_info, avoid grabbing the rest to save a
> malloc. For do_dump, add a more full-featured helper, but avoid
> free/realloc of buffer when possible for multi-prog dumps.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> ---
> tools/bpf/bpftool/btf_dumper.c | 40 +++++----
> tools/bpf/bpftool/prog.c | 154 +++++++++++++++++++++++++--------
> 2 files changed, 144 insertions(+), 50 deletions(-)
>
> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> index 9c25286a5c73..0f85704628bf 100644
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -32,14 +32,16 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d,
> const struct btf_type *func_proto,
> __u32 prog_id)
> {
> - struct bpf_prog_info_linear *prog_info = NULL;
> const struct btf_type *func_type;
> + int prog_fd = -1, func_sig_len;
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> const char *prog_name = NULL;
> - struct bpf_func_info *finfo;
> struct btf *prog_btf = NULL;
> - struct bpf_prog_info *info;
> - int prog_fd, func_sig_len;
> + struct bpf_func_info finfo;
> + __u32 finfo_rec_size;
> char prog_str[1024];
> + int err;
>
> /* Get the ptr's func_proto */
> func_sig_len = btf_dump_func(d->btf, prog_str, func_proto, NULL, 0,
> @@ -55,22 +57,27 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d,
> if (prog_fd == -1)
please change this to (prog_fd < 0), see [0] for why
we should check all the other places in bpftool to see if there are
any patterns like this that would break on libbpf 1.0 (cc Quentin as
well)
[0] https://github.com/libbpf/libbpf/wiki/Libbpf-1.0-migration-guide#direct-error-code-returning-libbpf_strict_direct_errs
> goto print;
>
> - prog_info = bpf_program__get_prog_info_linear(prog_fd,
> - 1UL << BPF_PROG_INFO_FUNC_INFO);
> - close(prog_fd);
> - if (IS_ERR(prog_info)) {
> - prog_info = NULL;
> + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> + if (err)
> goto print;
> - }
> - info = &prog_info->info;
>
> - if (!info->btf_id || !info->nr_func_info)
> + if (!info.btf_id || !info.nr_func_info)
> + goto print;
> +
> + finfo_rec_size = info.func_info_rec_size;
> + memset(&info, 0, sizeof(info));
> + info.nr_func_info = 1;
> + info.func_info_rec_size = finfo_rec_size;
> + info.func_info = ptr_to_u64(&finfo);
> +
> + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> + if (err)
> goto print;
> - prog_btf = btf__load_from_kernel_by_id(info->btf_id);
> +
> + prog_btf = btf__load_from_kernel_by_id(info.btf_id);
> if (libbpf_get_error(prog_btf))
> goto print;
> - finfo = u64_to_ptr(info->func_info);
> - func_type = btf__type_by_id(prog_btf, finfo->type_id);
> + func_type = btf__type_by_id(prog_btf, finfo.type_id);
> if (!func_type || !btf_is_func(func_type))
> goto print;
>
> @@ -92,7 +99,8 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d,
> prog_str[sizeof(prog_str) - 1] = '\0';
> jsonw_string(d->jw, prog_str);
> btf__free(prog_btf);
> - free(prog_info);
> + if (prog_fd != -1)
similarly, prog_fd >= 0 here; also, isn't this a fix? Can you add
Fixes: tag then?
> + close(prog_fd);
> return 0;
> }
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index a24ea7e26aa4..3b3ccc7b6dd4 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -98,6 +98,72 @@ static enum bpf_attach_type parse_attach_type(const char *str)
> return __MAX_BPF_ATTACH_TYPE;
> }
>
> +#define holder_prep_needed_rec_sz(nr, rec_size)\
> +({ \
> + holder.nr = info->nr; \
> + needed += holder.nr * rec_size; \
> +})
> +
> +#define holder_prep_needed(nr, rec_size) \
> +({ \
> + holder.nr = info->nr; \
> + holder.rec_size = info->rec_size; \
> + needed += holder.nr * holder.rec_size; \
> +})
> +
> +#define holder_set_ptr(field, nr, rec_size) \
> +({ \
> + holder.field = ptr_to_u64(ptr); \
> + ptr += nr * rec_size; \
> +})
> +
> +static int prep_prog_info(struct bpf_prog_info *const info, enum dump_mode mode,
> + void **info_data, size_t *const info_data_sz)
> +{
> + struct bpf_prog_info holder = {};
> + size_t needed = 0;
> + void *ptr;
> +
> + if (mode == DUMP_JITED)
> + holder_prep_needed_rec_sz(jited_prog_len, 1);
> + else
> + holder_prep_needed_rec_sz(xlated_prog_len, 1);
> +
> + holder_prep_needed_rec_sz(nr_jited_ksyms, sizeof(__u64));
> + holder_prep_needed_rec_sz(nr_jited_func_lens, sizeof(__u32));
> + holder_prep_needed(nr_func_info, func_info_rec_size);
> + holder_prep_needed(nr_line_info, line_info_rec_size);
> + holder_prep_needed(nr_jited_line_info, jited_line_info_rec_size);
> +
> + if (needed > *info_data_sz) {
> + *info_data = realloc(*info_data, needed);
never do `x = realloc(x);`, if realloc fails, original memory is
leaked. Temporary variable is necessary to work with realloc, see a
bunch of other places in libbpf where we use realloc for an example.
> + if (!*info_data)
> + return -1;
> + *info_data_sz = needed;
> + }
> +
> + ptr = *info_data;
> +
> + if (mode == DUMP_JITED)
> + holder_set_ptr(jited_prog_insns, holder.jited_prog_len, 1);
> + else
> + holder_set_ptr(xlated_prog_insns, holder.xlated_prog_len, 1);
> +
> + holder_set_ptr(jited_ksyms, holder.nr_jited_ksyms, sizeof(__u64));
> + holder_set_ptr(jited_func_lens, holder.nr_jited_func_lens, sizeof(__u32));
> + holder_set_ptr(func_info, holder.nr_func_info, holder.func_info_rec_size);
> + holder_set_ptr(line_info, holder.nr_line_info, holder.line_info_rec_size);
> + holder_set_ptr(jited_line_info, holder.nr_jited_line_info,
> + holder.jited_line_info_rec_size);
tbh, I completely lost track of what these holder_xxx() macro actually
do here... You saved few lines of code, but I think we lost a lot in
readability
> +
> + *info = holder;
instead of copying at the end, why not fill out the passed in
bpf_prog_info directly? Of course *info stuff is annoying, but that's
solved with a temporary variable, no?
> + return 0;
> +}
> +
> +#undef holder_prep_needed
> +#undef holder_prep_needed_rec_sz
> +#undef holder_set_ptr
> +
[...]
