netdev - Re: [PATCH bpf-next v7 07/10] libbpf: add support for freplace attachment in bpf_link

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: <CAEf4BzZHAgEn3dodbqZVp==7d_KsC4nuS-AwgFvAjDFiF4L4BA@mail.gmail.com>
Date:   Mon, 21 Sep 2020 16:18:48 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v7 07/10] libbpf: add support for freplace
 attachment in bpf_link_create

On Sat, Sep 19, 2020 at 4:51 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@...hat.com>
>
> This adds support for supplying a target btf ID for the bpf_link_create()
> operation, and adds a new bpf_program__attach_freplace() high-level API for
> attaching freplace functions with a target.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---
>  tools/lib/bpf/bpf.c      |   18 +++++++++++++++---
>  tools/lib/bpf/bpf.h      |    3 ++-
>  tools/lib/bpf/libbpf.c   |   36 +++++++++++++++++++++++++++++++-----
>  tools/lib/bpf/libbpf.h   |    3 +++
>  tools/lib/bpf/libbpf.map |    1 +
>  5 files changed, 52 insertions(+), 9 deletions(-)
>

Looks good, but let's add meaningful error messages, especially that
it's so trivial to do and obvious what exactly is wrong.

> +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
> +                                             int target_fd,
> +                                             const char *attach_func_name)
> +{
> +       int btf_id;
> +
> +       if (!!target_fd != !!attach_func_name || prog->type != BPF_PROG_TYPE_EXT)
> +               return ERR_PTR(-EINVAL);

Can you please split those two and have separate error messages for
both, following libbpf's format, e.g.,:

prog '%s': should be freplace program type

and

prog '%s': either both or none of target FD and target function name
should be provided

> +
> +       if (target_fd) {
> +

nit: why empty line?

> +               btf_id = libbpf_find_prog_btf_id(attach_func_name, target_fd);
> +               if (btf_id < 0)
> +                       return ERR_PTR(btf_id);
> +
> +               return bpf_program__attach_fd(prog, target_fd, btf_id, "freplace");
> +       } else {
> +               /* no target, so use raw_tracepoint_open for compatibility
> +                * with old kernels
> +                */
> +               return bpf_program__attach_trace(prog);
> +       }
>  }
>

[...]