Re: [patch net-next] tc: add BPF based action

Re: [patch net-next] tc: add BPF based action

[Alexei Starovoitov]

On Wed, Jan 7, 2015 at 11:26 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>>On the other hand, I would understand if it's at some point in
>>time eBPF which would f.e. mangle the packet, but the API you
>>propose is clearly classic BPF. ;)
>
> Exactly. I would like to extend cls_bpf and act_bpf to handle eBPF right
> after. That is the point.

I say that connecting it with classic BPF is not a prerequisite
to use eBPF in there. Invocation place may be the same,
but the way to pass the program will be different.
For classic with just pass the whole program, whereas
for eBPF we'll be likely passing fd.
Theoretically we can pass eBPF as vanilla program
as well that doesn't have map access, but verifier check
will only be binary (accept or reject). Which is not user
friendly. Even rejection of classic BPF is hard to decipher.
Especially when only language for classic is assembler
and poor users have no easy way to know what was
wrong with the program. Therefore I like bpf syscall
as a main and only interface to load the programs
and pass prog_fd to places where they suppose to run.
Having syscall as center place to load programs
also helps with accounting, since root will be able
to do something like 'lsmod' to see all loaded programs.
Anyway, that's a conversion for later...

As Daniel pointed out I think some better articulation
on what classic bpf programs will do here is needed.
It seems they will work as pre-filter on an action?
Few examples would help to understand use cases...

Re: [patch net-next] tc: add BPF based action

[Jiri Pirko]

Thu, Jan 08, 2015 at 08:04:31PM CET, ast@plumgrid.com wrote:
>On Wed, Jan 7, 2015 at 11:26 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>
>>>On the other hand, I would understand if it's at some point in
>>>time eBPF which would f.e. mangle the packet, but the API you
>>>propose is clearly classic BPF. ;)
>>
>> Exactly. I would like to extend cls_bpf and act_bpf to handle eBPF right
>> after. That is the point.
>
>I say that connecting it with classic BPF is not a prerequisite
>to use eBPF in there. Invocation place may be the same,
>but the way to pass the program will be different.
>For classic with just pass the whole program, whereas
>for eBPF we'll be likely passing fd.
>Theoretically we can pass eBPF as vanilla program
>as well that doesn't have map access, but verifier check
>will only be binary (accept or reject). Which is not user
>friendly. Even rejection of classic BPF is hard to decipher.
>Especially when only language for classic is assembler
>and poor users have no easy way to know what was
>wrong with the program. Therefore I like bpf syscall
>as a main and only interface to load the programs
>and pass prog_fd to places where they suppose to run.
>Having syscall as center place to load programs
>also helps with accounting, since root will be able
>to do something like 'lsmod' to see all loaded programs.
>Anyway, that's a conversion for later...
>
>As Daniel pointed out I think some better articulation
>on what classic bpf programs will do here is needed.
>It seems they will work as pre-filter on an action?
>Few examples would help to understand use cases...

Well, one can define bpf action to do final policy in tc pipeline if
skb should be dropped or not. I'm aware that this is in theory doable by
cls_bpf, but oftentimes one likes to use different cls.

And also, the plan is to extend this for ebpf in near future, as well
as cls_bpf. That will provide many more possibilities for user.

The intention here is to keep cls_bpf and act_bpf feature-consistent.

Thanks.

Jiri

Re: [patch net-next] tc: add BPF based action

[Jiri Pirko]

Thu, Jan 08, 2015 at 03:55:37PM CET, hannes@redhat.com wrote:
>On Mi, 2015-01-07 at 17:43 +0100, Jiri Pirko wrote:
>> +static int tcf_bpf(struct sk_buff *skb, const struct tc_action *a,
>> +		   struct tcf_result *res)
>> +{
>> +	struct tcf_bpf *b = a->priv;
>> +	int action;
>> +	int filter_res;
>> +
>> +	spin_lock(&b->tcf_lock);
>> +	b->tcf_tm.lastuse = jiffies;
>> +	bstats_update(&b->tcf_bstats, skb);
>> +	action = b->tcf_action;
>> +
>> +	filter_res = BPF_PROG_RUN(b->filter, skb);
>> +	if (filter_res == -1)
>> +		goto drop;
>> +
>> +	goto unlock;
>> +
>> +drop:
>> +	action = TC_ACT_SHOT;
>> +	b->tcf_qstats.drops++;
>> +unlock:
>> +	spin_unlock(&b->tcf_lock);
>> +	return action;
>> +}
>
>In theory this could be like:
>
>filter_res = BPF_PROG_RUN(b->filter, skb);
>
>spin_lock(&b->tcf_lock);
><update stats...>
>
>if (filter_res == -1)
>  goto drop;
>
>action = b->tcf_action;
>...
>
>to keep BPF_PROG_RUN out of the spin_lock?

Okay. That makes sense. Will include this change into v2.

Thanks.

>
>Bye,
>Hannes
>
>

Re: [patch net-next] tc: add BPF based action

[Hannes Frederic Sowa]

On Mi, 2015-01-07 at 17:43 +0100, Jiri Pirko wrote:
> +static int tcf_bpf(struct sk_buff *skb, const struct tc_action *a,
> +		   struct tcf_result *res)
> +{
> +	struct tcf_bpf *b = a->priv;
> +	int action;
> +	int filter_res;
> +
> +	spin_lock(&b->tcf_lock);
> +	b->tcf_tm.lastuse = jiffies;
> +	bstats_update(&b->tcf_bstats, skb);
> +	action = b->tcf_action;
> +
> +	filter_res = BPF_PROG_RUN(b->filter, skb);
> +	if (filter_res == -1)
> +		goto drop;
> +
> +	goto unlock;
> +
> +drop:
> +	action = TC_ACT_SHOT;
> +	b->tcf_qstats.drops++;
> +unlock:
> +	spin_unlock(&b->tcf_lock);
> +	return action;
> +}

In theory this could be like:

filter_res = BPF_PROG_RUN(b->filter, skb);

spin_lock(&b->tcf_lock);
<update stats...>

if (filter_res == -1)
  goto drop;

action = b->tcf_action;
...

to keep BPF_PROG_RUN out of the spin_lock?

Bye,
Hannes

Re: [patch net-next] tc: add BPF based action

[Jiri Pirko]

Wed, Jan 07, 2015 at 07:33:39PM CET, dborkman@redhat.com wrote:
>On 01/07/2015 05:43 PM, Jiri Pirko wrote:
>>This action provides a possibility to exec custom BPF code.
>
>Can you elaborate a bit more on the particular use-case, and
>what scenarios are unsolveable with the BPF filter we already
>have in tc? Just wondering, since you're using BPF for the
>purpose of classifying (but just from the context of actions)
>what about a possibility of a generic container for reusing
>(any) classifier from the framework, so we would not need to
>duplicate code?
>
>On the other hand, I would understand if it's at some point in
>time eBPF which would f.e. mangle the packet, but the API you
>propose is clearly classic BPF. ;)

Exactly. I would like to extend cls_bpf and act_bpf to handle eBPF right
after. That is the point.

>
>Thanks,
>Daniel

Re: [patch net-next] tc: add BPF based action

[Cong Wang]

On Wed, Jan 7, 2015 at 10:33 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 01/07/2015 05:43 PM, Jiri Pirko wrote:
>>
>> This action provides a possibility to exec custom BPF code.
>
>
> Can you elaborate a bit more on the particular use-case, and
> what scenarios are unsolveable with the BPF filter we already
> have in tc? Just wondering, since you're using BPF for the
> purpose of classifying (but just from the context of actions)
> what about a possibility of a generic container for reusing
> (any) classifier from the framework, so we would not need to
> duplicate code?
>

+1

Also as its name tells, BPF is a filter, why it could be used
as an action here? (I don't follow eBPF though.)

Re: [patch net-next] tc: add BPF based action

[Daniel Borkmann]

On 01/07/2015 05:43 PM, Jiri Pirko wrote:
> This action provides a possibility to exec custom BPF code.

Can you elaborate a bit more on the particular use-case, and
what scenarios are unsolveable with the BPF filter we already
have in tc? Just wondering, since you're using BPF for the
purpose of classifying (but just from the context of actions)
what about a possibility of a generic container for reusing
(any) classifier from the framework, so we would not need to
duplicate code?

On the other hand, I would understand if it's at some point in
time eBPF which would f.e. mangle the packet, but the API you
propose is clearly classic BPF. ;)

Thanks,
Daniel

[patch net-next] tc: add BPF based action

[Jiri Pirko]

This action provides a possibility to exec custom BPF code.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/net/tc_act/tc_bpf.h        |  25 +++++
 include/uapi/linux/tc_act/Kbuild   |   1 +
 include/uapi/linux/tc_act/tc_bpf.h |  31 ++++++
 net/sched/Kconfig                  |  11 +++
 net/sched/Makefile                 |   1 +
 net/sched/act_bpf.c                | 196 +++++++++++++++++++++++++++++++++++++
 6 files changed, 265 insertions(+)
 create mode 100644 include/net/tc_act/tc_bpf.h
 create mode 100644 include/uapi/linux/tc_act/tc_bpf.h
 create mode 100644 net/sched/act_bpf.c

diff --git a/include/net/tc_act/tc_bpf.h b/include/net/tc_act/tc_bpf.h
new file mode 100644
index 0000000..95e11da
--- /dev/null
+++ b/include/net/tc_act/tc_bpf.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2015 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __NET_TC_BPF_H
+#define __NET_TC_BPF_H
+
+#include <linux/filter.h>
+#include <net/act_api.h>
+
+struct tcf_bpf {
+	struct tcf_common	common;
+	struct bpf_prog		*filter;
+	struct sock_filter	*bpf_ops;
+	u16			bpf_len;
+};
+#define to_bpf(a) \
+	container_of(a->priv, struct tcf_bpf, common)
+
+#endif /* __NET_TC_BPF_H */
diff --git a/include/uapi/linux/tc_act/Kbuild b/include/uapi/linux/tc_act/Kbuild
index b057da2..19d5219 100644
--- a/include/uapi/linux/tc_act/Kbuild
+++ b/include/uapi/linux/tc_act/Kbuild
@@ -8,3 +8,4 @@ header-y += tc_nat.h
 header-y += tc_pedit.h
 header-y += tc_skbedit.h
 header-y += tc_vlan.h
+header-y += tc_bpf.h
diff --git a/include/uapi/linux/tc_act/tc_bpf.h b/include/uapi/linux/tc_act/tc_bpf.h
new file mode 100644
index 0000000..5288bd77
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_bpf.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) 2015 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_BPF_H
+#define __LINUX_TC_BPF_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_BPF 13
+
+struct tc_act_bpf {
+	tc_gen;
+};
+
+enum {
+	TCA_ACT_BPF_UNSPEC,
+	TCA_ACT_BPF_TM,
+	TCA_ACT_BPF_PARMS,
+	TCA_ACT_BPF_OPS_LEN,
+	TCA_ACT_BPF_OPS,
+	__TCA_ACT_BPF_MAX,
+};
+#define TCA_ACT_BPF_MAX (__TCA_ACT_BPF_MAX - 1)
+
+#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index c54c9d9..cc311e9 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -698,6 +698,17 @@ config NET_ACT_VLAN
 	  To compile this code as a module, choose M here: the
 	  module will be called act_vlan.
 
+config NET_ACT_BPF
+        tristate "BPF based action"
+        depends on NET_CLS_ACT
+        ---help---
+	  Say Y here to execute BFP code on packets.
+
+	  If unsure, say N.
+
+	  To compile this code as a module, choose M here: the
+	  module will be called act_bpf.
+
 config NET_CLS_IND
 	bool "Incoming device classification"
 	depends on NET_CLS_U32 || NET_CLS_FW
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 679f24a..7ca2b4e 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_NET_ACT_SIMP)	+= act_simple.o
 obj-$(CONFIG_NET_ACT_SKBEDIT)	+= act_skbedit.o
 obj-$(CONFIG_NET_ACT_CSUM)	+= act_csum.o
 obj-$(CONFIG_NET_ACT_VLAN)	+= act_vlan.o
+obj-$(CONFIG_NET_ACT_BPF)	+= act_bpf.o
 obj-$(CONFIG_NET_SCH_FIFO)	+= sch_fifo.o
 obj-$(CONFIG_NET_SCH_CBQ)	+= sch_cbq.o
 obj-$(CONFIG_NET_SCH_HTB)	+= sch_htb.o
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
new file mode 100644
index 0000000..43f5f9d
--- /dev/null
+++ b/net/sched/act_bpf.c
@@ -0,0 +1,196 @@
+/*
+ * Copyright (c) 2015 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <linux/filter.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+
+#include <linux/tc_act/tc_bpf.h>
+#include <net/tc_act/tc_bpf.h>
+
+#define BPF_TAB_MASK     15
+
+static int tcf_bpf(struct sk_buff *skb, const struct tc_action *a,
+		   struct tcf_result *res)
+{
+	struct tcf_bpf *b = a->priv;
+	int action;
+	int filter_res;
+
+	spin_lock(&b->tcf_lock);
+	b->tcf_tm.lastuse = jiffies;
+	bstats_update(&b->tcf_bstats, skb);
+	action = b->tcf_action;
+
+	filter_res = BPF_PROG_RUN(b->filter, skb);
+	if (filter_res == -1)
+		goto drop;
+
+	goto unlock;
+
+drop:
+	action = TC_ACT_SHOT;
+	b->tcf_qstats.drops++;
+unlock:
+	spin_unlock(&b->tcf_lock);
+	return action;
+}
+
+static const struct nla_policy act_bpf_policy[TCA_ACT_BPF_MAX + 1] = {
+	[TCA_ACT_BPF_PARMS]	= { .len = sizeof(struct tc_act_bpf) },
+	[TCA_ACT_BPF_OPS_LEN]	= { .type = NLA_U16 },
+	[TCA_ACT_BPF_OPS]	= { .type = NLA_BINARY,
+				    .len = sizeof(struct sock_filter) * BPF_MAXINSNS },
+};
+
+static int tcf_bpf_init(struct net *net, struct nlattr *nla,
+			struct nlattr *est, struct tc_action *a,
+			int ovr, int bind)
+{
+	struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
+	struct tc_act_bpf *parm;
+	struct tcf_bpf *b;
+	u16 bpf_size, bpf_len;
+	struct sock_filter *bpf_ops;
+	struct sock_fprog_kern tmp;
+	struct bpf_prog *fp;
+	int ret;
+
+	if (!nla)
+		return -EINVAL;
+
+	ret = nla_parse_nested(tb, TCA_ACT_BPF_MAX, nla, act_bpf_policy);
+	if (ret < 0)
+		return ret;
+
+	if (!tb[TCA_ACT_BPF_PARMS] ||
+	    !tb[TCA_ACT_BPF_OPS_LEN] || !tb[TCA_ACT_BPF_OPS])
+		return -EINVAL;
+	parm = nla_data(tb[TCA_ACT_BPF_PARMS]);
+
+	bpf_len = nla_get_u16(tb[TCA_ACT_BPF_OPS_LEN]);
+	if (bpf_len > BPF_MAXINSNS || bpf_len == 0)
+		return -EINVAL;
+
+	bpf_size = bpf_len * sizeof(*bpf_ops);
+	bpf_ops = kzalloc(bpf_size, GFP_KERNEL);
+	if (!bpf_ops)
+		return -ENOMEM;
+
+	memcpy(bpf_ops, nla_data(tb[TCA_ACT_BPF_OPS]), bpf_size);
+
+	tmp.len = bpf_len;
+	tmp.filter = bpf_ops;
+
+	ret = bpf_prog_create(&fp, &tmp);
+	if (ret)
+		goto free_bpf_ops;
+
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*b), bind);
+		if (ret)
+			goto free_bpf_ops;
+
+		ret = ACT_P_CREATED;
+	} else {
+		if (bind)
+			goto free_bpf_ops;
+		tcf_hash_release(a, bind);
+		if (!ovr) {
+			ret = -EEXIST;
+			goto free_bpf_ops;
+		}
+	}
+
+	b = to_bpf(a);
+	spin_lock_bh(&b->tcf_lock);
+	b->tcf_action = parm->action;
+	b->bpf_len = bpf_len;
+	b->bpf_ops = bpf_ops;
+	b->filter = fp;
+	spin_unlock_bh(&b->tcf_lock);
+
+	if (ret == ACT_P_CREATED)
+		tcf_hash_insert(a);
+	return ret;
+
+free_bpf_ops:
+	kfree(bpf_ops);
+	return ret;
+}
+
+static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *a,
+			int bind, int ref)
+{
+	unsigned char *tp = skb_tail_pointer(skb);
+	struct tcf_bpf *b = a->priv;
+	struct tc_act_bpf opt = {
+		.index    = b->tcf_index,
+		.refcnt   = b->tcf_refcnt - ref,
+		.bindcnt  = b->tcf_bindcnt - bind,
+		.action   = b->tcf_action,
+	};
+	struct tcf_t t;
+	struct nlattr *nla;
+
+	if (nla_put(skb, TCA_ACT_BPF_PARMS, sizeof(opt), &opt))
+		goto nla_put_failure;
+
+	if (nla_put_u16(skb, TCA_ACT_BPF_OPS_LEN, b->bpf_len))
+		goto nla_put_failure;
+
+	nla = nla_reserve(skb, TCA_ACT_BPF_OPS, b->bpf_len *
+			  sizeof(struct sock_filter));
+	if (!nla)
+		goto nla_put_failure;
+
+	memcpy(nla_data(nla), b->bpf_ops, nla_len(nla));
+
+	t.install = jiffies_to_clock_t(jiffies - b->tcf_tm.install);
+	t.lastuse = jiffies_to_clock_t(jiffies - b->tcf_tm.lastuse);
+	t.expires = jiffies_to_clock_t(b->tcf_tm.expires);
+	if (nla_put(skb, TCA_ACT_BPF_TM, sizeof(t), &t))
+		goto nla_put_failure;
+	return skb->len;
+
+nla_put_failure:
+	nlmsg_trim(skb, tp);
+	return -1;
+}
+
+static struct tc_action_ops act_bpf_ops = {
+	.kind		=	"bpf",
+	.type		=	TCA_ACT_BPF,
+	.owner		=	THIS_MODULE,
+	.act		=	tcf_bpf,
+	.dump		=	tcf_bpf_dump,
+	.init		=	tcf_bpf_init,
+};
+
+static int __init bpf_init_module(void)
+{
+	return tcf_register_action(&act_bpf_ops, BPF_TAB_MASK);
+}
+
+static void __exit bpf_cleanup_module(void)
+{
+	tcf_unregister_action(&act_bpf_ops);
+}
+
+module_init(bpf_init_module);
+module_exit(bpf_cleanup_module);
+
+MODULE_AUTHOR("Jiri Pirko <jiri@resnulli.us>");
+MODULE_DESCRIPTION("TC BPF based action");
+MODULE_LICENSE("GPL v2");
-- 
1.9.3