netdev - Re: [PATCH bpf-next 2/2] libbpf: support new uapi for map element bpf iterator

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: <CAEf4BzaeT1HULBE0dQULSF62Wm6=t49Dc8jjHVJ9Nt1noxeCtQ@mail.gmail.com>
Date:   Sun, 2 Aug 2020 18:35:30 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 2/2] libbpf: support new uapi for map element bpf iterator

On Sat, Aug 1, 2020 at 9:22 PM Yonghong Song <yhs@...com> wrote:
>
> Previous commit adjusted kernel uapi for map
> element bpf iterator. This patch adjusted libbpf API
> due to uapi change.
>
> Signed-off-by: Yonghong Song <yhs@...com>
> ---
>  tools/lib/bpf/bpf.c    | 4 +++-
>  tools/lib/bpf/bpf.h    | 5 +++--
>  tools/lib/bpf/libbpf.c | 7 +++++--
>  3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index eab14c97c15d..c75a84398d51 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -598,7 +598,9 @@ int bpf_link_create(int prog_fd, int target_fd,
>         attr.link_create.prog_fd = prog_fd;
>         attr.link_create.target_fd = target_fd;
>         attr.link_create.attach_type = attach_type;
> -       attr.link_create.flags = OPTS_GET(opts, flags, 0);
> +       attr.link_create.iter_info =
> +               ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
> +       attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
>
>         return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
>  }
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 28855fd5b5f4..c9895f191305 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -170,9 +170,10 @@ LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
>
>  struct bpf_link_create_opts {
>         size_t sz; /* size of this struct for forward/backward compatibility */
> -       __u32 flags;

I'd actually keep flags in link_create_ops, as it's part of the kernel
UAPI anyways, we won't have to add it later. Just pass it through into
bpf_attr.

> +       union bpf_iter_link_info *iter_info;
> +       __u32 iter_info_len;
>  };
> -#define bpf_link_create_opts__last_field flags
> +#define bpf_link_create_opts__last_field iter_info_len
>
>  LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
>                                enum bpf_attach_type attach_type,
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 7be04e45d29c..dc8fabf9d30d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8298,6 +8298,7 @@ bpf_program__attach_iter(struct bpf_program *prog,
>                          const struct bpf_iter_attach_opts *opts)
>  {
>         DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_create_opts);
> +       union bpf_iter_link_info linfo;
>         char errmsg[STRERR_BUFSIZE];
>         struct bpf_link *link;
>         int prog_fd, link_fd;
> @@ -8307,8 +8308,10 @@ bpf_program__attach_iter(struct bpf_program *prog,
>                 return ERR_PTR(-EINVAL);
>
>         if (OPTS_HAS(opts, map_fd)) {
> -               target_fd = opts->map_fd;
> -               link_create_opts.flags = BPF_ITER_LINK_MAP_FD;
> +               memset(&linfo, 0, sizeof(linfo));
> +               linfo.map.map_fd = opts->map_fd;
> +               link_create_opts.iter_info = &linfo;
> +               link_create_opts.iter_info_len = sizeof(linfo);

Maybe instead of having map_fd directly in bpf_iter_attach_opts, let's
just accept bpf_iter_link_info and its len directly from the user?
Right now kernel UAPI and libbpf API for customizing iterator
attachment differ. It would be simpler to keep them in sync and we
won't have to discuss how to evolve bpf_iter_attach_opts as we add
more customization for different types of iterators. Thoughts?

>         }
>
>         prog_fd = bpf_program__fd(prog);
> --
> 2.24.1
>