netdev - Re: [PATCH bpf-next 2/2] libbpf: add support for prog

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: <CAEf4BzatBeCW0mLWSuO_LFguwnT8RXr=WoUJFR_rjKHbyvukaQ@mail.gmail.com>
Date:   Wed, 30 Oct 2019 15:02:03 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <ast@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 2/2] libbpf: add support for prog_tracing

On Wed, Oct 30, 2019 at 12:36 PM Alexei Starovoitov <ast@...nel.org> wrote:
>
> Cleanup libbpf from expected_attach_type == attach_btf_id hack
> and introduce BPF_PROG_TYPE_TRACING.
>
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---

Looks good overall, but please replace all u32 to __u32 and add proper
LIBBPF_APIs for all the new exposed functions from libbpf.h, with
those changes:

Acked-by: Andrii Nakryiko <andriin@...com>

>  tools/include/uapi/linux/bpf.h |  2 +
>  tools/lib/bpf/bpf.c            |  8 ++--
>  tools/lib/bpf/bpf.h            |  5 +-
>  tools/lib/bpf/libbpf.c         | 88 +++++++++++++++++++++++++---------
>  tools/lib/bpf/libbpf.h         |  4 ++
>  tools/lib/bpf/libbpf_probes.c  |  1 +
>  6 files changed, 80 insertions(+), 28 deletions(-)
>

[...]

>
> +static int libbpf_attach_btf_id_by_name(const char *name, u32 *btf_id);

here and in few places below, u32 will break libbpf on Github, please use __u32

> +
>  static struct bpf_object *
>  __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>                    struct bpf_object_open_opts *opts)
> @@ -3656,6 +3660,7 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>         bpf_object__for_each_program(prog, obj) {
>                 enum bpf_prog_type prog_type;
>                 enum bpf_attach_type attach_type;
> +               u32 btf_id;
>
>                 err = libbpf_prog_type_by_name(prog->section_name, &prog_type,
>                                                &attach_type);
> @@ -3667,6 +3672,12 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>
>                 bpf_program__set_type(prog, prog_type);
>                 bpf_program__set_expected_attach_type(prog, attach_type);
> +               if (prog_type == BPF_PROG_TYPE_TRACING) {
> +                       err = libbpf_attach_btf_id_by_name(prog->section_name, &btf_id);
> +                       if (err)
> +                               goto out;
> +                       bpf_program__set_attach_btf_id(prog, btf_id);
> +               }
>         }
>
>         return obj;

[...]

>
> +#define BTF_PREFIX "btf_trace_"
> +static int libbpf_attach_btf_id_by_name(const char *name, u32 *btf_id)
> +{
> +       struct btf *btf = bpf_core_find_kernel_btf();
> +       char raw_tp_btf_name[128] = BTF_PREFIX;
> +       char *dst = raw_tp_btf_name + sizeof(BTF_PREFIX) - 1;
> +       int ret, i, err;
> +
> +       if (IS_ERR(btf)) {
> +               pr_warn("vmlinux BTF is not found\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!name) {
> +               err = -EINVAL;
> +               goto err;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(section_names); i++) {
> +               if (!section_names[i].is_attach_btf)
> +                       continue;
> +               if (strncmp(name, section_names[i].sec, section_names[i].len))
> +                       continue;
> +               /* prepend "btf_trace_" prefix per kernel convention */
> +               strncat(dst, name + section_names[i].len,
> +                       sizeof(raw_tp_btf_name) - sizeof(BTF_PREFIX));
> +               ret = btf__find_by_name(btf, raw_tp_btf_name);
> +               if (ret <= 0) {
> +                       pr_warn("%s is not found in vmlinux BTF\n", dst);
> +                       err = -EINVAL;
> +                       goto err;
> +               }
> +               *btf_id = ret;
> +               err = 0;

nit: I'd just initialize err to zero, it will be easy to miss this if
we need to extend this function a bit.

> +               goto err;
> +       }
> +       pr_warn("failed to identify btf_id based on ELF section name '%s'\n", name);
> +       err = -ESRCH;
> +err:

err is misleading, it's just exit, really

> +       btf__free(btf);
> +       return err;
> +}
> +
>  int libbpf_attach_type_by_name(const char *name,
>                                enum bpf_attach_type *attach_type)
>  {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index c63e2ff84abc..a3f5b8d3398d 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -307,6 +307,7 @@ LIBBPF_API int bpf_program__set_sched_cls(struct bpf_program *prog);
>  LIBBPF_API int bpf_program__set_sched_act(struct bpf_program *prog);
>  LIBBPF_API int bpf_program__set_xdp(struct bpf_program *prog);
>  LIBBPF_API int bpf_program__set_perf_event(struct bpf_program *prog);
> +int bpf_program__set_tracing(struct bpf_program *prog);

LIBBPF_API? same few places below

>
>  LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
>  LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> @@ -317,6 +318,8 @@ bpf_program__get_expected_attach_type(struct bpf_program *prog);
>  LIBBPF_API void
>  bpf_program__set_expected_attach_type(struct bpf_program *prog,
>                                       enum bpf_attach_type type);
> +void
> +bpf_program__set_attach_btf_id(struct bpf_program *prog, u32 btf_id);
>

[...]