Message-ID: <CAEf4BzZ69YgrQW7DHCJUT_X+GqMq_ZQQPBwopaJJVGFD5=d5Vg@mail.gmail.com>
Date: Thu, 25 May 2023 14:01:46 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Florian Westphal <fw@...len.de>
Cc: bpf@...r.kernel.org, ast@...nel.org, netdev@...r.kernel.org, dxu@...uu.xyz, 
	qde@...cy.de
Subject: Re: [PATCH bpf-next 1/2] tools: libbpf: add netfilter link attach helper

On Thu, May 25, 2023 at 4:01 AM Florian Westphal <fw@...len.de> wrote:
>
> Add new api function: bpf_program__attach_netfilter_opts.
>
> It takes a bpf program (netfilter type), and a pointer to a option struct
> that contains the desired attachment (protocol family, priority, hook
> location, ...).
>
> It returns a pointer to a 'bpf_link' structure or NULL on error.
>
> Next patch adds new netfilter_basic test that uses this function to
> attach a program to a few pf/hook/priority combinations.
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@...il.com>
> Signed-off-by: Florian Westphal <fw@...len.de>
> ---
>  tools/lib/bpf/libbpf.c   | 51 ++++++++++++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.h   | 15 ++++++++++++
>  tools/lib/bpf/libbpf.map |  1 +
>  3 files changed, 67 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 5cca00979aae..033447aa0773 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11811,6 +11811,57 @@ static int attach_iter(const struct bpf_program *prog, long cookie, struct bpf_l
>         return libbpf_get_error(*link);
>  }
>
> +struct bpf_link *bpf_program__attach_netfilter_opts(const struct bpf_program *prog,
> +                                                   const struct bpf_netfilter_opts *opts)

let's just call it `bpf_program__attach_netfilter`. We add "_opts" if
we had variant without opts. This doesn't apply here, so a shorter
name is preferable.

> +{
> +       const size_t attr_sz = offsetofend(union bpf_attr, link_create);
> +       struct bpf_link *link;
> +       int prog_fd, link_fd;
> +       union bpf_attr attr;
> +
> +       if (!OPTS_VALID(opts, bpf_netfilter_opts))
> +               return libbpf_err_ptr(-EINVAL);
> +
> +       prog_fd = bpf_program__fd(prog);
> +       if (prog_fd < 0) {
> +               pr_warn("prog '%s': can't attach before loaded\n", prog->name);
> +               return libbpf_err_ptr(-EINVAL);
> +       }
> +
> +       link = calloc(1, sizeof(*link));
> +       if (!link)
> +               return libbpf_err_ptr(-ENOMEM);
> +       link->detach = &bpf_link__detach_fd;
> +
> +       memset(&attr, 0, attr_sz);
> +
> +       attr.link_create.prog_fd = prog_fd;
> +       attr.link_create.netfilter.pf = OPTS_GET(opts, pf, 0);
> +       attr.link_create.netfilter.hooknum = OPTS_GET(opts, hooknum, 0);
> +       attr.link_create.netfilter.priority = OPTS_GET(opts, priority, 0);
> +       attr.link_create.netfilter.flags = OPTS_GET(opts, flags, 0);
> +
> +       link_fd = syscall(__NR_bpf, BPF_LINK_CREATE, &attr, attr_sz);

this code shouldn't do direct syscall, these high-level APIs should go
through libbpf low-level API. In this case, you need to call
bpf_link_create().

Except bpf_link_create() doesn't really support NETLINK links yet,
which is what we'll need to fix first. bpf_link_create() determines
what kind of parameters to pass to kernel based on bpf_attach_type.
And we currently don't have an attach type for NETLINK BPF link.
Thankfully it's not too late to add it. I see that link_create() in
kernel/bpf/syscall.c just bypasses attach_type check. We shouldn't
have done that. Instead we need to add BPF_NETLINK attach type to enum
bpf_attach_type. And wire all that properly throughout the kernel and
libbpf itself. Thankfully kernel release is not finalized and we can
still fix that up, but please prioritize it before we get too far into
rc releases.

> +
> +       link->fd = ensure_good_fd(link_fd);
> +
> +       if (link->fd < 0) {
> +               char errmsg[STRERR_BUFSIZE];
> +
> +               link_fd = -errno;
> +               free(link);
> +               pr_warn("prog '%s': failed to attach to pf:%d,hooknum:%d:prio:%d: %s\n",
> +                       prog->name,
> +                       OPTS_GET(opts, pf, 0),
> +                       OPTS_GET(opts, hooknum, 0),
> +                       OPTS_GET(opts, priority, 0),
> +                       libbpf_strerror_r(link_fd, errmsg, sizeof(errmsg)));
> +               return libbpf_err_ptr(link_fd);
> +       }
> +
> +       return link;
> +}
> +
>  struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
>  {
>         struct bpf_link *link = NULL;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 754da73c643b..081beb95a097 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -718,6 +718,21 @@ LIBBPF_API struct bpf_link *
>  bpf_program__attach_freplace(const struct bpf_program *prog,
>                              int target_fd, const char *attach_func_name);
>
> +struct bpf_netfilter_opts {
> +       /* size of this struct, for forward/backward compatibility */
> +       size_t sz;
> +
> +       __u32 pf;
> +       __u32 hooknum;
> +       __s32 priority;
> +       __u32 flags;
> +};
> +#define bpf_netfilter_opts__last_field flags
> +
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_netfilter_opts(const struct bpf_program *prog,
> +                                  const struct bpf_netfilter_opts *opts);
> +
>  struct bpf_map;
>
>  LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 7521a2fb7626..e13d60608bf3 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -395,4 +395,5 @@ LIBBPF_1.2.0 {
>  LIBBPF_1.3.0 {
>         global:
>                 bpf_obj_pin_opts;
> +               bpf_program__attach_netfilter_opts;

opts and the rest looks good, thanks

>  } LIBBPF_1.2.0;
> --
> 2.39.3
>

Powered by blists - more mailing lists