Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1687819413.git.dxu@dxuuu.xyz> <d98692cd1451b35b0939718f5d4676444aa40069.1687819413.git.dxu@dxuuu.xyz>
In-Reply-To: <d98692cd1451b35b0939718f5d4676444aa40069.1687819413.git.dxu@dxuuu.xyz>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Mon, 26 Jun 2023 17:11:52 -0700
Message-ID: <CAEf4Bza-b8WhMofDrOZcUr59inZcX8GWXMJxuHmPxj7eChzr=w@mail.gmail.com>
Subject: Re: [PATCH bpf-next 1/7] tools: libbpf: add netfilter link attach helper
To:     Daniel Xu <dxu@dxuuu.xyz>
Cc:     daniel@iogearbox.net, ast@kernel.org, andrii@kernel.org,
        fw@strlen.de, martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, netfilter-devel@vger.kernel.org,
        dsahern@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 26, 2023 at 4:02=E2=80=AFPM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Add new api function: bpf_program__attach_netfilter.
>
> 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.
>
> Co-developed-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  tools/lib/bpf/bpf.c      |  8 +++++++
>  tools/lib/bpf/bpf.h      |  6 +++++
>  tools/lib/bpf/libbpf.c   | 47 ++++++++++++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.h   | 15 +++++++++++++
>  tools/lib/bpf/libbpf.map |  1 +
>  5 files changed, 77 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index ed86b37d8024..3b0da19715e1 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -741,6 +741,14 @@ int bpf_link_create(int prog_fd, int target_fd,
>                 if (!OPTS_ZEROED(opts, tracing))
>                         return libbpf_err(-EINVAL);
>                 break;
> +       case BPF_NETFILTER:
> +               attr.link_create.netfilter.pf =3D OPTS_GET(opts, netfilte=
r.pf, 0);
> +               attr.link_create.netfilter.hooknum =3D OPTS_GET(opts, net=
filter.hooknum, 0);
> +               attr.link_create.netfilter.priority =3D OPTS_GET(opts, ne=
tfilter.priority, 0);
> +               attr.link_create.netfilter.flags =3D OPTS_GET(opts, netfi=
lter.flags, 0);
> +               if (!OPTS_ZEROED(opts, netfilter))
> +                       return libbpf_err(-EINVAL);
> +               break;
>         default:
>                 if (!OPTS_ZEROED(opts, flags))
>                         return libbpf_err(-EINVAL);
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 9aa0ee473754..c676295ab9bf 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -349,6 +349,12 @@ struct bpf_link_create_opts {
>                 struct {
>                         __u64 cookie;
>                 } tracing;
> +               struct {
> +                       __u32 pf;
> +                       __u32 hooknum;
> +                       __s32 priority;
> +                       __u32 flags;
> +               } netfilter;
>         };
>         size_t :0;
>  };
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 214f828ece6b..a8b9d5abb55f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11811,6 +11811,53 @@ 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(const struct bpf_program =
*prog,
> +                                              const struct bpf_netfilter=
_opts *opts)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_create_opts);

nit: let's use shorter LIBBPF_OPTS() macro

> +       struct bpf_link *link;
> +       int prog_fd, link_fd;
> +
> +       if (!OPTS_VALID(opts, bpf_netfilter_opts))
> +               return libbpf_err_ptr(-EINVAL);
> +
> +       link_create_opts.netfilter.pf =3D OPTS_GET(opts, pf, 0);
> +       link_create_opts.netfilter.hooknum =3D OPTS_GET(opts, hooknum, 0)=
;
> +       link_create_opts.netfilter.priority =3D OPTS_GET(opts, priority, =
0);
> +       link_create_opts.netfilter.flags =3D OPTS_GET(opts, flags, 0);
> +
> +       prog_fd =3D 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 =3D calloc(1, sizeof(*link));
> +       if (!link)
> +               return libbpf_err_ptr(-ENOMEM);
> +       link->detach =3D &bpf_link__detach_fd;
> +
> +       link_fd =3D bpf_link_create(prog_fd, 0, BPF_NETFILTER, &link_crea=
te_opts);
> +
> +       link->fd =3D ensure_good_fd(link_fd);

bpf_link_create() does ensure_good_fd() already, no need to do it
here, just assign result directly


> +
> +       if (link->fd < 0) {
> +               char errmsg[STRERR_BUFSIZE];
> +
> +               link_fd =3D -errno;
> +               free(link);
> +               pr_warn("prog '%s': failed to attach to pf:%d,hooknum:%d:=
prio:%d: %s\n",

comma before prio? but also how necessary is to emit all these? what
if we add another argument to opts, would we add them here as well?

I'd just go with just "failed to attach netfilter" and keep it simple

> +                       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 =3D NULL;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 754da73c643b..10642ad69d76 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(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..d9ec4407befa 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;
>  } LIBBPF_1.2.0;
> --
> 2.40.1
>