netdev - Re: [PATCH bpf-next 07/10] libbpf: Add extended attach/detach opts

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: <CAEf4BzadsJWcCm4e=Bc40EaLj_8dMRy-MrfF7uwF5ysdZgGnkA@mail.gmail.com>
Date:   Wed, 5 Oct 2022 20:19:40 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     bpf@...r.kernel.org, razor@...ckwall.org, ast@...nel.org,
        andrii@...nel.org, martin.lau@...ux.dev, john.fastabend@...il.com,
        joannelkoong@...il.com, memxor@...il.com, toke@...hat.com,
        joe@...ium.io, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 07/10] libbpf: Add extended attach/detach opts

On Tue, Oct 4, 2022 at 4:12 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> Extend libbpf attach opts and add a new detach opts API so this can be used
> to add/remove fd-based tc BPF programs. For concrete usage examples, see the
> extensive selftests that have been developed as part of this series.
>
> Co-developed-by: Nikolay Aleksandrov <razor@...ckwall.org>
> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.org>
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> ---
>  tools/lib/bpf/bpf.c      | 21 +++++++++++++++++++++
>  tools/lib/bpf/bpf.h      | 17 +++++++++++++++--
>  tools/lib/bpf/libbpf.map |  1 +
>  3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 18b1e91cc469..d1e338ac9a62 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -670,6 +670,27 @@ int bpf_prog_detach2(int prog_fd, int target_fd, enum bpf_attach_type type)
>         return libbpf_err_errno(ret);
>  }
>
> +int bpf_prog_detach_opts(int prog_fd, int target_fd,
> +                        enum bpf_attach_type type,
> +                        const struct bpf_prog_detach_opts *opts)
> +{
> +       const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
> +       union bpf_attr attr;
> +       int ret;
> +
> +       if (!OPTS_VALID(opts, bpf_prog_detach_opts))
> +               return libbpf_err(-EINVAL);
> +
> +       memset(&attr, 0, attr_sz);
> +       attr.target_fd     = target_fd;
> +       attr.attach_bpf_fd = prog_fd;
> +       attr.attach_type   = type;
> +       attr.attach_priority = OPTS_GET(opts, attach_priority, 0);
> +
> +       ret = sys_bpf(BPF_PROG_DETACH, &attr, attr_sz);
> +       return libbpf_err_errno(ret);
> +}
> +
>  int bpf_link_create(int prog_fd, int target_fd,
>                     enum bpf_attach_type attach_type,
>                     const struct bpf_link_create_opts *opts)
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index bef7a5282188..96de58fecdbc 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -286,8 +286,11 @@ LIBBPF_API int bpf_obj_get_opts(const char *pathname,
>
>  struct bpf_prog_attach_opts {
>         size_t sz; /* size of this struct for forward/backward compatibility */
> -       unsigned int flags;
> -       int replace_prog_fd;
> +       __u32 flags;
> +       union {
> +               int replace_prog_fd;
> +               __u32 attach_priority;
> +       };

just add a new field, unions are very confusing in API structures.
It's ok if some unused fields stay zero.

>  };
>  #define bpf_prog_attach_opts__last_field replace_prog_fd
>
> @@ -296,9 +299,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
>  LIBBPF_API int bpf_prog_attach_opts(int prog_fd, int attachable_fd,
>                                      enum bpf_attach_type type,
>                                      const struct bpf_prog_attach_opts *opts);
> +
> +struct bpf_prog_detach_opts {
> +       size_t sz; /* size of this struct for forward/backward compatibility */
> +       __u32 attach_priority;

please add size_t: 0; at the end to ensure better zero-initialization
by compiler

> +};
> +#define bpf_prog_detach_opts__last_field attach_priority
> +
>  LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
>  LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
>                                 enum bpf_attach_type type);
> +LIBBPF_API int bpf_prog_detach_opts(int prog_fd, int target_fd,
> +                                   enum bpf_attach_type type,

given we add detach_opts API, is type something that always makes
sense? If not, let's move it into opts.

> +                                   const struct bpf_prog_detach_opts *opts);
>
>  union bpf_iter_link_info; /* defined in up-to-date linux/bpf.h */
>  struct bpf_link_create_opts {
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index c1d6aa7c82b6..0c94b4862ebb 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -377,4 +377,5 @@ LIBBPF_1.1.0 {
>                 user_ring_buffer__reserve;
>                 user_ring_buffer__reserve_blocking;
>                 user_ring_buffer__submit;
> +               bpf_prog_detach_opts;

let's keep this sorted

>  } LIBBPF_1.0.0;

> --
> 2.34.1
>