Date:   Wed, 26 Apr 2023 21:51:01 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Florian Westphal <fw@...len.de>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, dxu@...uu.xyz, qde@...cy.de
Subject: Re: [PATCH bpf-next v5 1/7] bpf: add bpf_link support for
 BPF_NETFILTER programs

On Fri, Apr 21, 2023 at 10:07 AM Florian Westphal <fw@...len.de> wrote:
>
> Add bpf_link support skeleton.  To keep this reviewable, no bpf program
> can be invoked yet, if a program is attached only a c-stub is called and
> not the actual bpf program.
>
> Defaults to 'y' if both netfilter and bpf syscall are enabled in kconfig.
>
> Uapi example usage:
>         union bpf_attr attr = { };
>
>         attr.link_create.prog_fd = progfd;
>         attr.link_create.attach_type = 0; /* unused */
>         attr.link_create.netfilter.pf = PF_INET;
>         attr.link_create.netfilter.hooknum = NF_INET_LOCAL_IN;
>         attr.link_create.netfilter.priority = -128;
>
>         err = bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
>
> ... this would attach progfd to ipv4:input hook.
>
> Such hook gets removed automatically if the calling program exits.
>
> BPF_NETFILTER program invocation is added in followup change.
>
> NF_HOOK_OP_BPF enum will eventually be read from nfnetlink_hook, it
> allows to tell userspace which program is attached at the given hook
> when user runs 'nft hook list' command rather than just the priority
> and not-very-helpful 'this hook runs a bpf prog but I can't tell which
> one'.
>
> Will also be used to disallow registration of two bpf programs with
> same priority in a followup patch.
>
> v4: arm32 cmpxchg only supports 32bit operand
>     s/prio/priority/
> v3: restrict prog attachment to ip/ip6 for now, lets lift restrictions if
>     more use cases pop up (arptables, ebtables, netdev ingress/egress etc).
>
> Signed-off-by: Florian Westphal <fw@...len.de>
> ---
>  no changes since last version
>
>  include/linux/netfilter.h           |   1 +
>  include/net/netfilter/nf_bpf_link.h |  10 ++
>  include/uapi/linux/bpf.h            |  14 +++
>  kernel/bpf/syscall.c                |   6 ++
>  net/netfilter/Kconfig               |   3 +
>  net/netfilter/Makefile              |   1 +
>  net/netfilter/nf_bpf_link.c         | 159 ++++++++++++++++++++++++++++
>  7 files changed, 194 insertions(+)
>  create mode 100644 include/net/netfilter/nf_bpf_link.h
>  create mode 100644 net/netfilter/nf_bpf_link.c
>
> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
> index c8e03bcaecaa..0762444e3767 100644
> --- a/include/linux/netfilter.h
> +++ b/include/linux/netfilter.h
> @@ -80,6 +80,7 @@ typedef unsigned int nf_hookfn(void *priv,
>  enum nf_hook_ops_type {
>         NF_HOOK_OP_UNDEFINED,
>         NF_HOOK_OP_NF_TABLES,
> +       NF_HOOK_OP_BPF,
>  };
>
>  struct nf_hook_ops {
> diff --git a/include/net/netfilter/nf_bpf_link.h b/include/net/netfilter/nf_bpf_link.h
> new file mode 100644
> index 000000000000..eeaeaf3d15de
> --- /dev/null
> +++ b/include/net/netfilter/nf_bpf_link.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#if IS_ENABLED(CONFIG_NETFILTER_BPF_LINK)
> +int bpf_nf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
> +#else
> +static inline int bpf_nf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> +{
> +       return -EOPNOTSUPP;
> +}
> +#endif
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 4b20a7269bee..1bb11a6ee667 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -986,6 +986,7 @@ enum bpf_prog_type {
>         BPF_PROG_TYPE_LSM,
>         BPF_PROG_TYPE_SK_LOOKUP,
>         BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */
> +       BPF_PROG_TYPE_NETFILTER,
>  };
>
>  enum bpf_attach_type {
> @@ -1050,6 +1051,7 @@ enum bpf_link_type {
>         BPF_LINK_TYPE_PERF_EVENT = 7,
>         BPF_LINK_TYPE_KPROBE_MULTI = 8,
>         BPF_LINK_TYPE_STRUCT_OPS = 9,
> +       BPF_LINK_TYPE_NETFILTER = 10,
>
>         MAX_BPF_LINK_TYPE,
>  };
> @@ -1560,6 +1562,12 @@ union bpf_attr {
>                                  */
>                                 __u64           cookie;
>                         } tracing;
> +                       struct {
> +                               __u32           pf;
> +                               __u32           hooknum;

catching up on stuff a bit...

enum nf_inet_hooks {
        NF_INET_PRE_ROUTING,
        NF_INET_LOCAL_IN,
        NF_INET_FORWARD,
        NF_INET_LOCAL_OUT,
        NF_INET_POST_ROUTING,
        NF_INET_NUMHOOKS,
        NF_INET_INGRESS = NF_INET_NUMHOOKS,
};

So it seems like this "hook number" is more like "hook type", is my
understanding correct? If so, wouldn't it be cleaner and more uniform
with, say, cgroup network hooks to provide hook type as
expected_attach_type? It would also allow to have a nicer interface in
libbpf, by specifying that as part of SEC():

SEC("netfilter/pre_routing"), SEC("netfilter/local_in"), etc...

Also, it seems like you actually didn't wire NETFILTER link support in
libbpf completely. See bpf_link_create under tools/lib/bpf/bpf.c, it
has to handle this new type of link as well. Existing tests seem a bit
bare-bones for SEC("netfilter"), would it be possible to add something
that will demonstrate it a bit better and will be actually executed at
runtime and validated?


> +                               __s32           priority;
> +                               __u32           flags;
> +                       } netfilter;
>                 };
>         } link_create;
>

[...]

Powered by blists - more mailing lists