Re: [PATCH bpf-next v4 3/8] libbpf: Add opts-based attach/detach/query API for tcx

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 10, 2023 at 1:12 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> Extend libbpf attach opts and add a new detach opts API so this can be used
> to add/remove fd-based tcx BPF programs. The old-style bpf_prog_detach() and
> bpf_prog_detach2() APIs are refactored to reuse the new bpf_prog_detach_opts()
> internally.
>
> The bpf_prog_query_opts() API got extended to be able to handle the new
> link_ids, link_attach_flags and revision fields.
>
> For concrete usage examples, see the extensive selftests that have been
> developed as part of this series.
>
> Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> ---
>  tools/lib/bpf/bpf.c      | 105 +++++++++++++++++++++++++--------------
>  tools/lib/bpf/bpf.h      |  92 ++++++++++++++++++++++++++++------
>  tools/lib/bpf/libbpf.c   |  12 +++--
>  tools/lib/bpf/libbpf.map |   1 +
>  4 files changed, 157 insertions(+), 53 deletions(-)
>

Thanks for doc comments! Looks good, left a few nits with suggestions
for simplifying code, but it's minor.

Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 3b0da19715e1..3dfc43b477c3 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -629,55 +629,87 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
>         return bpf_prog_attach_opts(prog_fd, target_fd, type, &opts);
>  }
>
> -int bpf_prog_attach_opts(int prog_fd, int target_fd,
> -                         enum bpf_attach_type type,
> -                         const struct bpf_prog_attach_opts *opts)
> +int bpf_prog_attach_opts(int prog_fd, int target,
> +                        enum bpf_attach_type type,
> +                        const struct bpf_prog_attach_opts *opts)
>  {
> -       const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
> +       const size_t attr_sz = offsetofend(union bpf_attr, expected_revision);
> +       __u32 relative_id, flags;
>         union bpf_attr attr;
> -       int ret;
> +       int ret, relative;
>
>         if (!OPTS_VALID(opts, bpf_prog_attach_opts))
>                 return libbpf_err(-EINVAL);
>
> +       relative_id = OPTS_GET(opts, relative_id, 0);
> +       relative = OPTS_GET(opts, relative_fd, 0);
> +       flags = OPTS_GET(opts, flags, 0);
> +
> +       /* validate we don't have unexpected combinations of non-zero fields */
> +       if (relative > 0 && relative_id)
> +               return libbpf_err(-EINVAL);

I left a comment in the next patch about this, I think it should be
simple `if (relative_fd && relative_id) { /* bad */ }`. But see the
next patch for why.

> +       if (relative_id) {
> +               relative = relative_id;
> +               flags |= BPF_F_ID;
> +       }

it's a bit hard to follow as written (to me at least). How about a
slight variation that has less in-place state update


int relative_fd, relative_id;

relative_fd = OPTS_GET(opts, relative_fd, 0);
relative_id = OPTS_GET(opts, relative_id, 0);

/* only one of fd or id can be specified */
if (relative_fd && relative_id > 0)
    return libbpf_err(-EINVAL);

... then see further below

> +
>         memset(&attr, 0, attr_sz);
> -       attr.target_fd     = target_fd;
> -       attr.attach_bpf_fd = prog_fd;
> -       attr.attach_type   = type;
> -       attr.attach_flags  = OPTS_GET(opts, flags, 0);
> -       attr.replace_bpf_fd = OPTS_GET(opts, replace_prog_fd, 0);
> +       attr.target_fd          = target;
> +       attr.attach_bpf_fd      = prog_fd;
> +       attr.attach_type        = type;
> +       attr.attach_flags       = flags;
> +       attr.relative_fd        = relative;

instead of two lines above, have simple if/else

if (relative_if) {
    attr.relative_id = relative_id;
    attr.attach_flags = flags | BPF_F_ID;
} else {
    attr.relative_fd = relative_fd;
    attr.attach_flags = flags;
}

This combined with the piece above seems very straightforward in terms
of what is checked and what's passed into attr. WDYT?

> +       attr.replace_bpf_fd     = OPTS_GET(opts, replace_fd, 0);
> +       attr.expected_revision  = OPTS_GET(opts, expected_revision, 0);
>
>         ret = sys_bpf(BPF_PROG_ATTACH, &attr, attr_sz);
>         return libbpf_err_errno(ret);
>  }
>
> -int bpf_prog_detach(int target_fd, enum bpf_attach_type type)
> +int bpf_prog_detach_opts(int prog_fd, int target,
> +                        enum bpf_attach_type type,
> +                        const struct bpf_prog_detach_opts *opts)
>  {
> -       const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
> +       const size_t attr_sz = offsetofend(union bpf_attr, expected_revision);
> +       __u32 relative_id, flags;
>         union bpf_attr attr;
> -       int ret;
> +       int ret, relative;
> +
> +       if (!OPTS_VALID(opts, bpf_prog_detach_opts))
> +               return libbpf_err(-EINVAL);
> +
> +       relative_id = OPTS_GET(opts, relative_id, 0);
> +       relative = OPTS_GET(opts, relative_fd, 0);
> +       flags = OPTS_GET(opts, flags, 0);
> +
> +       /* validate we don't have unexpected combinations of non-zero fields */
> +       if (relative > 0 && relative_id)
> +               return libbpf_err(-EINVAL);
> +       if (relative_id) {
> +               relative = relative_id;
> +               flags |= BPF_F_ID;
> +       }

see above, I think the same data flow simplification can be done

>
>         memset(&attr, 0, attr_sz);
> -       attr.target_fd   = target_fd;
> -       attr.attach_type = type;
> +       attr.target_fd          = target;
> +       attr.attach_bpf_fd      = prog_fd;
> +       attr.attach_type        = type;
> +       attr.attach_flags       = flags;
> +       attr.relative_fd        = relative;
> +       attr.expected_revision  = OPTS_GET(opts, expected_revision, 0);
>
>         ret = sys_bpf(BPF_PROG_DETACH, &attr, attr_sz);
>         return libbpf_err_errno(ret);
>  }
>

[...]

> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index d9ec4407befa..a95d39bbef90 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -396,4 +396,5 @@ LIBBPF_1.3.0 {
>         global:
>                 bpf_obj_pin_opts;
>                 bpf_program__attach_netfilter;
> +               bpf_prog_detach_opts;

I think it sorts before bpf_program__attach_netfilter?

>  } LIBBPF_1.2.0;


> --
> 2.34.1
>





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux