Re: [PATCH net-next 6/7] ebpf: remove CONFIG_BPF_SYSCALL ifdefs in socket filter code

Re: [PATCH net-next 6/7] ebpf: remove CONFIG_BPF_SYSCALL ifdefs in socket filter code

[Alexei Starovoitov]

On Tue, Feb 10, 2015 at 4:15 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> Socket filter code and other subsystems with upcoming eBPF support
> should not need to deal with the fact that we have CONFIG_BPF_SYSCALL
> defined or not. Having the bpf syscall as a config option is a nice
> thing and I'd expect it to stay that way for expert users (I presume
> one day the default setting of it might change, though), but code
> making use of it should not care if it's actually enabled or not.
> Instead, hide this via header files and let the rest deal with it.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  include/linux/bpf.h |  27 +++++++--
>  net/core/filter.c   | 166 ++++++++++++++++++++++++----------------------------
>  2 files changed, 100 insertions(+), 93 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 4fe1bd3..def0103 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -114,9 +114,6 @@ struct bpf_prog_type_list {
>         enum bpf_prog_type type;
>  };
>
> -int bpf_register_prog_type(struct bpf_prog_type_list *tl);
> -void bpf_unregister_prog_type(struct bpf_prog_type_list *tl);
> -
>  struct bpf_prog;
>
>  struct bpf_prog_aux {
> @@ -130,11 +127,31 @@ struct bpf_prog_aux {
>  };
>
>  #ifdef CONFIG_BPF_SYSCALL
> +int bpf_register_prog_type(struct bpf_prog_type_list *tl);
> +void bpf_unregister_prog_type(struct bpf_prog_type_list *tl);
> +
>  void bpf_prog_put(struct bpf_prog *prog);
> +struct bpf_prog *bpf_prog_get(u32 ufd);
>  #else
> -static inline void bpf_prog_put(struct bpf_prog *prog) {}
> +static inline int bpf_register_prog_type(struct bpf_prog_type_list *tl)
> +{
> +       return 0;
> +}
> +
> +static inline void bpf_unregister_prog_type(struct bpf_prog_type_list *tl)
> +{
> +}
> +
> +static inline struct bpf_prog *bpf_prog_get(u32 ufd)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline void bpf_prog_put(struct bpf_prog *prog)
> +{
> +}
>  #endif
> -struct bpf_prog *bpf_prog_get(u32 ufd);

makes sense. I have similar change for bpf+tracing.

> -#ifdef CONFIG_BPF_SYSCALL
> -int sk_attach_bpf(u32 ufd, struct sock *sk)
> -{
> -       struct sk_filter *fp, *old_fp;
> -       struct bpf_prog *prog;

why move the functions inside filter.c ?
couldn't we just remove two lines with #ifdef/endif ?

Re: [PATCH net-next 6/7] ebpf: remove CONFIG_BPF_SYSCALL ifdefs in socket filter code

[Daniel Borkmann]

On 02/11/2015 02:51 AM, Alexei Starovoitov wrote:
...
> why move the functions inside filter.c ?
> couldn't we just remove two lines with #ifdef/endif ?

Yep, will do in the non-RFC. That should make it shorter.

[RFC PATCH net-next 0/7] eBPF support for cls_bpf

[Daniel Borkmann]

I'm sending this out only as RFC as the merge window is open
anyway and Dave's pull request pending.

My plan would be to get this work fully refined and remove
some rough edges until net-next opens up again, submitting
it as non-RFC.

So for the time being similarly as the recent OVS eBPF action
patchset this may serve as a discussion ground, also wrt to
the BPF tutorial at netdev01 [1].

I presume on top of this e.g. Jiri might want to follow-up
with act_bpf, I'm also open/happy to contribute to it.

The series starts with a couple of cleanups and making the
prog type infrastructure pluggable in eBPF. Most interesting
is probably the last patch that adds actual support and the
iproute2 bits from the link below.

With regards to accessing fields from the context (here: skb),
I leave that for future work just as we currently do in socket
filters, also with regard to the currently ongoing ABI discussion
from tracing side. Nevertheless, the state with these patches
still allows for interesting functionality to be implemented as
complex classifiers, e.g. such as a fully fledged C-like flow
dissector as shown in bpf samples directory and the like.

iproute2 part:

   http://git.breakpoint.cc/cgit/dborkman/iproute2.git/log/?h=ebpf

I have configured and built LLVM with: --enable-experimental-targets=BPF

  [1] https://www.netdev01.org/sessions/15

Thanks !

Daniel Borkmann (7):
  ebpf: remove kernel test stubs
  ebpf: constify various function pointer structs
  ebpf: check first for MAXINSNS in bpf_prog_load
  ebpf: extend program type/subsystem registration
  ebpf: export BPF_PSEUDO_MAP_FD to uapi
  ebpf: remove CONFIG_BPF_SYSCALL ifdefs in socket filter code
  cls_bpf: add initial eBPF support for programmable classifiers

 include/linux/bpf.h          |  46 +++++++---
 include/linux/filter.h       |   2 -
 include/uapi/linux/bpf.h     |   3 +
 include/uapi/linux/pkt_cls.h |   1 +
 kernel/bpf/Makefile          |   3 -
 kernel/bpf/arraymap.c        |   6 +-
 kernel/bpf/hashtab.c         |   6 +-
 kernel/bpf/helpers.c         |   9 +-
 kernel/bpf/syscall.c         |  96 +++++++++++++++------
 kernel/bpf/test_stub.c       |  78 -----------------
 kernel/bpf/verifier.c        |  28 ++++--
 net/core/filter.c            | 169 +++++++++++++++++-------------------
 net/sched/cls_bpf.c          | 200 ++++++++++++++++++++++++++++++++-----------
 samples/bpf/libbpf.h         |   4 +-
 samples/bpf/test_verifier.c  |   5 +-
 15 files changed, 374 insertions(+), 282 deletions(-)
 delete mode 100644 kernel/bpf/test_stub.c

-- 
1.9.3

[PATCH net-next 6/7] ebpf: remove CONFIG_BPF_SYSCALL ifdefs in socket filter code

[Daniel Borkmann]

Socket filter code and other subsystems with upcoming eBPF support
should not need to deal with the fact that we have CONFIG_BPF_SYSCALL
defined or not. Having the bpf syscall as a config option is a nice
thing and I'd expect it to stay that way for expert users (I presume
one day the default setting of it might change, though), but code
making use of it should not care if it's actually enabled or not.
Instead, hide this via header files and let the rest deal with it.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 include/linux/bpf.h |  27 +++++++--
 net/core/filter.c   | 166 ++++++++++++++++++++++++----------------------------
 2 files changed, 100 insertions(+), 93 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4fe1bd3..def0103 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -114,9 +114,6 @@ struct bpf_prog_type_list {
 	enum bpf_prog_type type;
 };
 
-int bpf_register_prog_type(struct bpf_prog_type_list *tl);
-void bpf_unregister_prog_type(struct bpf_prog_type_list *tl);
-
 struct bpf_prog;
 
 struct bpf_prog_aux {
@@ -130,11 +127,31 @@ struct bpf_prog_aux {
 };
 
 #ifdef CONFIG_BPF_SYSCALL
+int bpf_register_prog_type(struct bpf_prog_type_list *tl);
+void bpf_unregister_prog_type(struct bpf_prog_type_list *tl);
+
 void bpf_prog_put(struct bpf_prog *prog);
+struct bpf_prog *bpf_prog_get(u32 ufd);
 #else
-static inline void bpf_prog_put(struct bpf_prog *prog) {}
+static inline int bpf_register_prog_type(struct bpf_prog_type_list *tl)
+{
+	return 0;
+}
+
+static inline void bpf_unregister_prog_type(struct bpf_prog_type_list *tl)
+{
+}
+
+static inline struct bpf_prog *bpf_prog_get(u32 ufd)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void bpf_prog_put(struct bpf_prog *prog)
+{
+}
 #endif
-struct bpf_prog *bpf_prog_get(u32 ufd);
+
 /* verify correctness of eBPF program */
 int bpf_check(struct bpf_prog *fp, union bpf_attr *attr);
 
diff --git a/net/core/filter.c b/net/core/filter.c
index d76560f..306b860 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1020,6 +1020,46 @@ void bpf_prog_destroy(struct bpf_prog *fp)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_destroy);
 
+int sk_attach_bpf(u32 ufd, struct sock *sk)
+{
+	struct sk_filter *fp, *old_fp;
+	struct bpf_prog *prog;
+
+	if (sock_flag(sk, SOCK_FILTER_LOCKED))
+		return -EPERM;
+
+	prog = bpf_prog_get(ufd);
+	if (IS_ERR(prog))
+		return PTR_ERR(prog);
+
+	if (prog->aux->tl->type != BPF_PROG_TYPE_SOCKET_FILTER) {
+		bpf_prog_put(prog);
+		return -EINVAL;
+	}
+
+	fp = kmalloc(sizeof(*fp), GFP_KERNEL);
+	if (!fp) {
+		bpf_prog_put(prog);
+		return -ENOMEM;
+	}
+
+	fp->prog = prog;
+	atomic_set(&fp->refcnt, 0);
+
+	if (!sk_filter_charge(sk, fp)) {
+		__sk_filter_release(fp);
+		return -ENOMEM;
+	}
+
+	old_fp = rcu_dereference_protected(sk->sk_filter,
+					   sock_owned_by_user(sk));
+	rcu_assign_pointer(sk->sk_filter, fp);
+	if (old_fp)
+		sk_filter_uncharge(sk, old_fp);
+
+	return 0;
+}
+
 /**
  *	sk_attach_filter - attach a socket filter
  *	@fprog: the filter program
@@ -1094,94 +1134,6 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(sk_attach_filter);
 
-#ifdef CONFIG_BPF_SYSCALL
-int sk_attach_bpf(u32 ufd, struct sock *sk)
-{
-	struct sk_filter *fp, *old_fp;
-	struct bpf_prog *prog;
-
-	if (sock_flag(sk, SOCK_FILTER_LOCKED))
-		return -EPERM;
-
-	prog = bpf_prog_get(ufd);
-	if (IS_ERR(prog))
-		return PTR_ERR(prog);
-
-	if (prog->aux->tl->type != BPF_PROG_TYPE_SOCKET_FILTER) {
-		/* valid fd, but invalid program type */
-		bpf_prog_put(prog);
-		return -EINVAL;
-	}
-
-	fp = kmalloc(sizeof(*fp), GFP_KERNEL);
-	if (!fp) {
-		bpf_prog_put(prog);
-		return -ENOMEM;
-	}
-	fp->prog = prog;
-
-	atomic_set(&fp->refcnt, 0);
-
-	if (!sk_filter_charge(sk, fp)) {
-		__sk_filter_release(fp);
-		return -ENOMEM;
-	}
-
-	old_fp = rcu_dereference_protected(sk->sk_filter,
-					   sock_owned_by_user(sk));
-	rcu_assign_pointer(sk->sk_filter, fp);
-
-	if (old_fp)
-		sk_filter_uncharge(sk, old_fp);
-
-	return 0;
-}
-
-/* allow socket filters to call
- * bpf_map_lookup_elem(), bpf_map_update_elem(), bpf_map_delete_elem()
- */
-static const struct bpf_func_proto *sock_filter_func_proto(enum bpf_func_id func_id)
-{
-	switch (func_id) {
-	case BPF_FUNC_map_lookup_elem:
-		return &bpf_map_lookup_elem_proto;
-	case BPF_FUNC_map_update_elem:
-		return &bpf_map_update_elem_proto;
-	case BPF_FUNC_map_delete_elem:
-		return &bpf_map_delete_elem_proto;
-	default:
-		return NULL;
-	}
-}
-
-static bool sock_filter_is_valid_access(int off, int size, enum bpf_access_type type)
-{
-	/* skb fields cannot be accessed yet */
-	return false;
-}
-
-static const struct bpf_verifier_ops sock_filter_ops = {
-	.get_func_proto = sock_filter_func_proto,
-	.is_valid_access = sock_filter_is_valid_access,
-};
-
-static struct bpf_prog_type_list sock_filter_type __read_mostly = {
-	.ops = &sock_filter_ops,
-	.type = BPF_PROG_TYPE_SOCKET_FILTER,
-};
-
-static int __init register_sock_filter_ops(void)
-{
-	return bpf_register_prog_type(&sock_filter_type);
-}
-late_initcall(register_sock_filter_ops);
-#else
-int sk_attach_bpf(u32 ufd, struct sock *sk)
-{
-	return -EOPNOTSUPP;
-}
-#endif
-
 int sk_detach_filter(struct sock *sk)
 {
 	int ret = -ENOENT;
@@ -1241,3 +1193,41 @@ out:
 	release_sock(sk);
 	return ret;
 }
+
+static const struct bpf_func_proto *
+sock_filter_func_proto(enum bpf_func_id func_id)
+{
+	switch (func_id) {
+	case BPF_FUNC_map_lookup_elem:
+		return &bpf_map_lookup_elem_proto;
+	case BPF_FUNC_map_update_elem:
+		return &bpf_map_update_elem_proto;
+	case BPF_FUNC_map_delete_elem:
+		return &bpf_map_delete_elem_proto;
+	default:
+		return NULL;
+	}
+}
+
+static bool sock_filter_is_valid_access(int off, int size,
+					enum bpf_access_type type)
+{
+	/* skb fields cannot be accessed yet */
+	return false;
+}
+
+static const struct bpf_verifier_ops sock_filter_ops = {
+	.get_func_proto = sock_filter_func_proto,
+	.is_valid_access = sock_filter_is_valid_access,
+};
+
+static struct bpf_prog_type_list sock_filter_type __read_mostly = {
+	.ops = &sock_filter_ops,
+	.type = BPF_PROG_TYPE_SOCKET_FILTER,
+};
+
+static int __init register_sock_filter_ops(void)
+{
+	return bpf_register_prog_type(&sock_filter_type);
+}
+late_initcall(register_sock_filter_ops);
-- 
1.9.3