All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/2] bpf: introduce bpf_skb_vlan_push/pop() helpers
@ 2015-07-17  2:58 Alexei Starovoitov
  2015-07-17  2:58 ` [PATCH net-next 1/2] " Alexei Starovoitov
  2015-07-17  2:58 ` [PATCH net-next 2/2] test_bpf: add bpf_skb_vlan_push/pop() tests Alexei Starovoitov
  0 siblings, 2 replies; 5+ messages in thread
From: Alexei Starovoitov @ 2015-07-17  2:58 UTC (permalink / raw)
  To: David S. Miller; +Cc: Michael Holzheu, Daniel Borkmann, netdev

Let eBPF programs call skb_vlan_push/pop via helpers.
JIT changes to accomodate of re-caching of skb->data and headerlen
turned out to be pretty simple.

This patch is on top of Daniel's bpf_get_cgroup_classid():
http://patchwork.ozlabs.org/patch/495826/
to avoid merge conflicts.

Alexei Starovoitov (2):
  bpf: introduce bpf_skb_vlan_push/pop() helpers
  test_bpf: add bpf_skb_vlan_push/pop() tests

 arch/s390/net/bpf_jit_comp.c |    4 ++
 arch/x86/net/bpf_jit_comp.c  |   80 ++++++++++++++++++----------------
 include/linux/bpf.h          |    2 +
 include/linux/filter.h       |    1 +
 include/uapi/linux/bpf.h     |    2 +
 kernel/bpf/core.c            |    1 +
 lib/test_bpf.c               |   98 ++++++++++++++++++++++++++++++++++++++++--
 net/core/filter.c            |   49 +++++++++++++++++++++
 8 files changed, 196 insertions(+), 41 deletions(-)

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net-next 1/2] bpf: introduce bpf_skb_vlan_push/pop() helpers
  2015-07-17  2:58 [PATCH net-next 0/2] bpf: introduce bpf_skb_vlan_push/pop() helpers Alexei Starovoitov
@ 2015-07-17  2:58 ` Alexei Starovoitov
  2015-07-17  8:12   ` Eric Dumazet
  2015-07-17  2:58 ` [PATCH net-next 2/2] test_bpf: add bpf_skb_vlan_push/pop() tests Alexei Starovoitov
  1 sibling, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2015-07-17  2:58 UTC (permalink / raw)
  To: David S. Miller; +Cc: Michael Holzheu, Daniel Borkmann, netdev

In order to let eBPF programs call skb_vlan_push/pop via helper functions
eBPF JITs need to recognize helpers that change skb->data, since
skb->data and hlen are cached as part of JIT code generation.
- arm64 JIT is using bpf_load_pointer() without caching, so it's ok as-is.
- x64 JIT recognizes bpf_skb_vlan_push/pop() calls and re-caches skb->data/hlen
  after such calls (experiments showed that conditional re-caching is slower).
- s390 JIT falls back to interpreter for now when bpf_skb_vlan_push() is present
  in the program (re-caching is tbd).

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
with these helpers the phys2virt gateway I showed at nfws can be done
in scalable way without creating a ton of vlan netdevs.

 arch/s390/net/bpf_jit_comp.c |    4 +++
 arch/x86/net/bpf_jit_comp.c  |   80 ++++++++++++++++++++++--------------------
 include/linux/bpf.h          |    2 ++
 include/linux/filter.h       |    1 +
 include/uapi/linux/bpf.h     |    2 ++
 net/core/filter.c            |   47 +++++++++++++++++++++++++
 6 files changed, 98 insertions(+), 38 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index fee782acc2ee..79c731e8d178 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -973,6 +973,10 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp, int i
 		 */
 		const u64 func = (u64)__bpf_call_base + imm;
 
+		if (bpf_helper_changes_skb_data((void *)func))
+			/* TODO reload skb->data, hlen */
+			return -1;
+
 		REG_SET_SEEN(BPF_REG_5);
 		jit->seen |= SEEN_FUNC;
 		/* lg %w1,<d(imm)>(%l) */
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 579a8fd74be0..6c335a8fc086 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -315,6 +315,26 @@ static void emit_bpf_tail_call(u8 **pprog)
 	*pprog = prog;
 }
 
+
+static void emit_load_skb_data_hlen(u8 **pprog)
+{
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	/* r9d = skb->len - skb->data_len (headlen)
+	 * r10 = skb->data
+	 */
+	/* mov %r9d, off32(%rdi) */
+	EMIT3_off32(0x44, 0x8b, 0x8f, offsetof(struct sk_buff, len));
+
+	/* sub %r9d, off32(%rdi) */
+	EMIT3_off32(0x44, 0x2b, 0x8f, offsetof(struct sk_buff, data_len));
+
+	/* mov %r10, off32(%rdi) */
+	EMIT3_off32(0x4c, 0x8b, 0x97, offsetof(struct sk_buff, data));
+	*pprog = prog;
+}
+
 static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		  int oldproglen, struct jit_context *ctx)
 {
@@ -329,36 +349,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 
 	emit_prologue(&prog);
 
-	if (seen_ld_abs) {
-		/* r9d : skb->len - skb->data_len (headlen)
-		 * r10 : skb->data
-		 */
-		if (is_imm8(offsetof(struct sk_buff, len)))
-			/* mov %r9d, off8(%rdi) */
-			EMIT4(0x44, 0x8b, 0x4f,
-			      offsetof(struct sk_buff, len));
-		else
-			/* mov %r9d, off32(%rdi) */
-			EMIT3_off32(0x44, 0x8b, 0x8f,
-				    offsetof(struct sk_buff, len));
-
-		if (is_imm8(offsetof(struct sk_buff, data_len)))
-			/* sub %r9d, off8(%rdi) */
-			EMIT4(0x44, 0x2b, 0x4f,
-			      offsetof(struct sk_buff, data_len));
-		else
-			EMIT3_off32(0x44, 0x2b, 0x8f,
-				    offsetof(struct sk_buff, data_len));
-
-		if (is_imm8(offsetof(struct sk_buff, data)))
-			/* mov %r10, off8(%rdi) */
-			EMIT4(0x4c, 0x8b, 0x57,
-			      offsetof(struct sk_buff, data));
-		else
-			/* mov %r10, off32(%rdi) */
-			EMIT3_off32(0x4c, 0x8b, 0x97,
-				    offsetof(struct sk_buff, data));
-	}
+	if (seen_ld_abs)
+		emit_load_skb_data_hlen(&prog);
 
 	for (i = 0; i < insn_cnt; i++, insn++) {
 		const s32 imm32 = insn->imm;
@@ -367,6 +359,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		u8 b1 = 0, b2 = 0, b3 = 0;
 		s64 jmp_offset;
 		u8 jmp_cond;
+		bool reload_skb_data;
 		int ilen;
 		u8 *func;
 
@@ -818,12 +811,18 @@ xadd:			if (is_imm8(insn->off))
 			func = (u8 *) __bpf_call_base + imm32;
 			jmp_offset = func - (image + addrs[i]);
 			if (seen_ld_abs) {
-				EMIT2(0x41, 0x52); /* push %r10 */
-				EMIT2(0x41, 0x51); /* push %r9 */
-				/* need to adjust jmp offset, since
-				 * pop %r9, pop %r10 take 4 bytes after call insn
-				 */
-				jmp_offset += 4;
+				reload_skb_data = bpf_helper_changes_skb_data(func);
+				if (reload_skb_data) {
+					EMIT1(0x57); /* push %rdi */
+					jmp_offset += 22; /* pop, mov, sub, mov */
+				} else {
+					EMIT2(0x41, 0x52); /* push %r10 */
+					EMIT2(0x41, 0x51); /* push %r9 */
+					/* need to adjust jmp offset, since
+					 * pop %r9, pop %r10 take 4 bytes after call insn
+					 */
+					jmp_offset += 4;
+				}
 			}
 			if (!imm32 || !is_simm32(jmp_offset)) {
 				pr_err("unsupported bpf func %d addr %p image %p\n",
@@ -832,8 +831,13 @@ xadd:			if (is_imm8(insn->off))
 			}
 			EMIT1_off32(0xE8, jmp_offset);
 			if (seen_ld_abs) {
-				EMIT2(0x41, 0x59); /* pop %r9 */
-				EMIT2(0x41, 0x5A); /* pop %r10 */
+				if (reload_skb_data) {
+					EMIT1(0x5F); /* pop %rdi */
+					emit_load_skb_data_hlen(&prog);
+				} else {
+					EMIT2(0x41, 0x59); /* pop %r9 */
+					EMIT2(0x41, 0x5A); /* pop %r10 */
+				}
 			}
 			break;
 
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4383476a0d48..139d6d2e123f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -192,5 +192,7 @@ extern const struct bpf_func_proto bpf_ktime_get_ns_proto;
 extern const struct bpf_func_proto bpf_get_current_pid_tgid_proto;
 extern const struct bpf_func_proto bpf_get_current_uid_gid_proto;
 extern const struct bpf_func_proto bpf_get_current_comm_proto;
+extern const struct bpf_func_proto bpf_skb_vlan_push_proto;
+extern const struct bpf_func_proto bpf_skb_vlan_pop_proto;
 
 #endif /* _LINUX_BPF_H */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 17724f6ea983..69d00555ce35 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -411,6 +411,7 @@ void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp);
 
 u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
 void bpf_int_jit_compile(struct bpf_prog *fp);
+bool bpf_helper_changes_skb_data(void *func);
 
 #ifdef CONFIG_BPF_JIT
 typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 2de87e58b12b..2f6c83d714e9 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -256,6 +256,8 @@ enum bpf_func_id {
 	 * Return: classid if != 0
 	 */
 	BPF_FUNC_get_cgroup_classid,
+	BPF_FUNC_skb_vlan_push, /* bpf_skb_vlan_push(skb, vlan_proto, vlan_tci) */
+	BPF_FUNC_skb_vlan_pop,  /* bpf_skb_vlan_pop(skb) */
 	__BPF_FUNC_MAX_ID,
 };
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 247450a5e387..3683ab09a48a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1437,6 +1437,49 @@ static const struct bpf_func_proto bpf_get_cgroup_classid_proto = {
 	.arg1_type      = ARG_PTR_TO_CTX,
 };
 
+static u64 bpf_skb_vlan_push(u64 r1, u64 vlan_proto, u64 vlan_tci, u64 r4, u64 r5)
+{
+	struct sk_buff *skb = (struct sk_buff *) (long) r1;
+
+	if (unlikely(vlan_proto != htons(ETH_P_8021Q) &&
+		     vlan_proto != htons(ETH_P_8021AD)))
+		vlan_proto = htons(ETH_P_8021Q);
+
+	return skb_vlan_push(skb, vlan_proto, vlan_tci);
+}
+
+const struct bpf_func_proto bpf_skb_vlan_push_proto = {
+	.func           = bpf_skb_vlan_push,
+	.gpl_only       = false,
+	.ret_type       = RET_INTEGER,
+	.arg1_type      = ARG_PTR_TO_CTX,
+	.arg2_type      = ARG_ANYTHING,
+	.arg3_type      = ARG_ANYTHING,
+};
+
+static u64 bpf_skb_vlan_pop(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
+{
+	struct sk_buff *skb = (struct sk_buff *) (long) r1;
+
+	return skb_vlan_pop(skb);
+}
+
+const struct bpf_func_proto bpf_skb_vlan_pop_proto = {
+	.func           = bpf_skb_vlan_pop,
+	.gpl_only       = false,
+	.ret_type       = RET_INTEGER,
+	.arg1_type      = ARG_PTR_TO_CTX,
+};
+
+bool bpf_helper_changes_skb_data(void *func)
+{
+	if (func == bpf_skb_vlan_push)
+		return true;
+	if (func == bpf_skb_vlan_pop)
+		return true;
+	return false;
+}
+
 static const struct bpf_func_proto *
 sk_filter_func_proto(enum bpf_func_id func_id)
 {
@@ -1476,6 +1519,10 @@ tc_cls_act_func_proto(enum bpf_func_id func_id)
 		return &bpf_clone_redirect_proto;
 	case BPF_FUNC_get_cgroup_classid:
 		return &bpf_get_cgroup_classid_proto;
+	case BPF_FUNC_skb_vlan_push:
+		return &bpf_skb_vlan_push_proto;
+	case BPF_FUNC_skb_vlan_pop:
+		return &bpf_skb_vlan_pop_proto;
 	default:
 		return sk_filter_func_proto(func_id);
 	}
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH net-next 2/2] test_bpf: add bpf_skb_vlan_push/pop() tests
  2015-07-17  2:58 [PATCH net-next 0/2] bpf: introduce bpf_skb_vlan_push/pop() helpers Alexei Starovoitov
  2015-07-17  2:58 ` [PATCH net-next 1/2] " Alexei Starovoitov
@ 2015-07-17  2:58 ` Alexei Starovoitov
  1 sibling, 0 replies; 5+ messages in thread
From: Alexei Starovoitov @ 2015-07-17  2:58 UTC (permalink / raw)
  To: David S. Miller; +Cc: Michael Holzheu, Daniel Borkmann, netdev

improve accuracy of timing in test_bpf and add two stress tests:
- {skb->data[0], get_smp_processor_id} repeated 2k times
- {skb->data[0], vlan_push} x 68 followed by {skb->data[0], vlan_pop} x 68

1st test is useful to test performance of JIT implementation of BPF_LD_ABS
together with BPF_CALL instructions.
2nd test is stressing skb_vlan_push/pop logic together with skb->data access
via BPF_LD_ABS insn which checks that re-caching of skb->data is done correctly.

In order to call bpf_skb_vlan_push() from test_bpf.ko have to add
three export_symbol_gpl.

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
 kernel/bpf/core.c |    1 +
 lib/test_bpf.c    |   98 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 net/core/filter.c |    2 ++
 3 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index bf38f5e8196c..fafa74161445 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -177,6 +177,7 @@ noinline u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
 {
 	return 0;
 }
+EXPORT_SYMBOL_GPL(__bpf_call_base);
 
 /**
  *	__bpf_prog_run - run eBPF program on a given context
diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index 9198f28a5528..8b5e66f008b0 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/filter.h>
+#include <linux/bpf.h>
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 #include <linux/if_vlan.h>
@@ -355,6 +356,81 @@ static int bpf_fill_ja(struct bpf_test *self)
 	return __bpf_fill_ja(self, 12, 9);
 }
 
+static int bpf_fill_ld_abs_get_processor_id(struct bpf_test *self)
+{
+	unsigned int len = BPF_MAXINSNS;
+	struct sock_filter *insn;
+	int i;
+
+	insn = kmalloc_array(len, sizeof(*insn), GFP_KERNEL);
+	if (!insn)
+		return -ENOMEM;
+
+	for (i = 0; i < len - 1; i += 2) {
+		insn[i] = __BPF_STMT(BPF_LD | BPF_B | BPF_ABS, 0);
+		insn[i + 1] = __BPF_STMT(BPF_LD | BPF_W | BPF_ABS,
+					 SKF_AD_OFF + SKF_AD_CPU);
+	}
+
+	insn[len - 1] = __BPF_STMT(BPF_RET | BPF_K, 0xbee);
+
+	self->u.ptr.insns = insn;
+	self->u.ptr.len = len;
+
+	return 0;
+}
+
+#define PUSH_CNT 68
+/* test: {skb->data[0], vlan_push} x 68 + {skb->data[0], vlan_pop} x 68 */
+static int bpf_fill_ld_abs_vlan_push_pop(struct bpf_test *self)
+{
+	unsigned int len = BPF_MAXINSNS;
+	struct bpf_insn *insn;
+	int i = 0, j, k = 0;
+
+	insn = kmalloc_array(len, sizeof(*insn), GFP_KERNEL);
+	if (!insn)
+		return -ENOMEM;
+
+	insn[i++] = BPF_MOV64_REG(R6, R1);
+loop:
+	for (j = 0; j < PUSH_CNT; j++) {
+		insn[i++] = BPF_LD_ABS(BPF_B, 0);
+		insn[i] = BPF_JMP_IMM(BPF_JNE, R0, 0x34, len - i - 2);
+		i++;
+		insn[i++] = BPF_MOV64_REG(R1, R6);
+		insn[i++] = BPF_MOV64_IMM(R2, 1);
+		insn[i++] = BPF_MOV64_IMM(R3, 2);
+		insn[i++] = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+					 bpf_skb_vlan_push_proto.func - __bpf_call_base);
+		insn[i] = BPF_JMP_IMM(BPF_JNE, R0, 0, len - i - 2);
+		i++;
+	}
+
+	for (j = 0; j < PUSH_CNT; j++) {
+		insn[i++] = BPF_LD_ABS(BPF_B, 0);
+		insn[i] = BPF_JMP_IMM(BPF_JNE, R0, 0x34, len - i - 2);
+		i++;
+		insn[i++] = BPF_MOV64_REG(R1, R6);
+		insn[i++] = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+					 bpf_skb_vlan_pop_proto.func - __bpf_call_base);
+		insn[i] = BPF_JMP_IMM(BPF_JNE, R0, 0, len - i - 2);
+		i++;
+	}
+	if (++k < 5)
+		goto loop;
+
+	for (; i < len - 1; i++)
+		insn[i] = BPF_ALU32_IMM(BPF_MOV, R0, 0xbef);
+
+	insn[len - 1] = BPF_EXIT_INSN();
+
+	self->u.ptr.insns = insn;
+	self->u.ptr.len = len;
+
+	return 0;
+}
+
 static struct bpf_test tests[] = {
 	{
 		"TAX",
@@ -4398,6 +4474,22 @@ static struct bpf_test tests[] = {
 		{ { 0, 0xababcbac } },
 		.fill_helper = bpf_fill_maxinsns11,
 	},
+	{
+		"BPF_MAXINSNS: ld_abs+get_processor_id",
+		{ },
+		CLASSIC,
+		{ },
+		{ { 1, 0xbee } },
+		.fill_helper = bpf_fill_ld_abs_get_processor_id,
+	},
+	{
+		"BPF_MAXINSNS: ld_abs+vlan_push/pop",
+		{ },
+		INTERNAL,
+		{ 0x34 },
+		{ { 1, 0xbef } },
+		.fill_helper = bpf_fill_ld_abs_vlan_push_pop,
+	},
 };
 
 static struct net_device dev;
@@ -4551,14 +4643,14 @@ static int __run_one(const struct bpf_prog *fp, const void *data,
 	u64 start, finish;
 	int ret = 0, i;
 
-	start = ktime_to_us(ktime_get());
+	start = ktime_get_ns();
 
 	for (i = 0; i < runs; i++)
 		ret = BPF_PROG_RUN(fp, data);
 
-	finish = ktime_to_us(ktime_get());
+	finish = ktime_get_ns();
 
-	*duration = (finish - start) * 1000ULL;
+	*duration = finish - start;
 	do_div(*duration, runs);
 
 	return ret;
diff --git a/net/core/filter.c b/net/core/filter.c
index 3683ab09a48a..d8eb7e811aa3 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1456,6 +1456,7 @@ const struct bpf_func_proto bpf_skb_vlan_push_proto = {
 	.arg2_type      = ARG_ANYTHING,
 	.arg3_type      = ARG_ANYTHING,
 };
+EXPORT_SYMBOL_GPL(bpf_skb_vlan_push_proto);
 
 static u64 bpf_skb_vlan_pop(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
 {
@@ -1470,6 +1471,7 @@ const struct bpf_func_proto bpf_skb_vlan_pop_proto = {
 	.ret_type       = RET_INTEGER,
 	.arg1_type      = ARG_PTR_TO_CTX,
 };
+EXPORT_SYMBOL_GPL(bpf_skb_vlan_pop_proto);
 
 bool bpf_helper_changes_skb_data(void *func)
 {
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next 1/2] bpf: introduce bpf_skb_vlan_push/pop() helpers
  2015-07-17  2:58 ` [PATCH net-next 1/2] " Alexei Starovoitov
@ 2015-07-17  8:12   ` Eric Dumazet
  2015-07-17 18:17     ` Alexei Starovoitov
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2015-07-17  8:12 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Michael Holzheu, Daniel Borkmann, netdev

On Thu, 2015-07-16 at 19:58 -0700, Alexei Starovoitov wrote:
> In order to let eBPF programs call skb_vlan_push/pop via helper functions

Why should eBPF program do such thing ?

Are BPF users in the kernel expecting skb being changed, and are we sure
they reload all cached values when/if needed ?

> eBPF JITs need to recognize helpers that change skb->data, since
> skb->data and hlen are cached as part of JIT code generation.
> - arm64 JIT is using bpf_load_pointer() without caching, so it's ok as-is.
> - x64 JIT recognizes bpf_skb_vlan_push/pop() calls and re-caches skb->data/hlen
>   after such calls (experiments showed that conditional re-caching is slower).
> - s390 JIT falls back to interpreter for now when bpf_skb_vlan_push() is present
>   in the program (re-caching is tbd).



> +static u64 bpf_skb_vlan_push(u64 r1, u64 vlan_proto, u64 vlan_tci, u64 r4, u64 r5)
> +{
> +	struct sk_buff *skb = (struct sk_buff *) (long) r1;
> +
> +	if (unlikely(vlan_proto != htons(ETH_P_8021Q) &&
> +		     vlan_proto != htons(ETH_P_8021AD)))
> +		vlan_proto = htons(ETH_P_8021Q);


This would raise sparse error, as vlan_proto is u64, and htons() __be16

make C=2 CF=-D__CHECK_ENDIAN__ net/core/filter.o

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next 1/2] bpf: introduce bpf_skb_vlan_push/pop() helpers
  2015-07-17  8:12   ` Eric Dumazet
@ 2015-07-17 18:17     ` Alexei Starovoitov
  0 siblings, 0 replies; 5+ messages in thread
From: Alexei Starovoitov @ 2015-07-17 18:17 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David S. Miller, Michael Holzheu, Daniel Borkmann, netdev

On 7/17/15 1:12 AM, Eric Dumazet wrote:
> On Thu, 2015-07-16 at 19:58 -0700, Alexei Starovoitov wrote:
>> In order to let eBPF programs call skb_vlan_push/pop via helper functions
>
> Why should eBPF program do such thing ?
>
> Are BPF users in the kernel expecting skb being changed, and are we sure
> they reload all cached values when/if needed ?

well, classic BPF and even extended BPF with socket filters cannot use
these helpers. They are for TC ingress/egress only. There different
actions can already change skb->data while classifiers/actions are
running. btw, before we started discussing this topic at nfws,
I thought that bpf programs will never be able to change skb->data from
inside the programs, but turned out it's only JITs that needed
re-caching. Programs cannot see skb->data. They can access packet
only via ld_abs/ld_ind instructions and helper functions.
So any changes to internal fields of skb are invisible to programs.
skb->data/hlen cache that is part of JIT is also invisible to the
programs. It's an optimization that some JITs do. arm64 JIT doesn't
do this optimization, for example.
I'll reword commit log to better explain this.

One of the use cases is this phys2virt gateway I presented. There I
need to do vlan-learning and src mac forwarding. Currently I'm
creating as many as I can vlan netdevs on top of regular eth0
and attach tc+bpf to all of them. That's very inefficient.
With these helpers I'll attach tc+bpf to eth0 only and skip creation
of thousands of vlan netdevs.

>> eBPF JITs need to recognize helpers that change skb->data, since
>> skb->data and hlen are cached as part of JIT code generation.
>> - arm64 JIT is using bpf_load_pointer() without caching, so it's ok as-is.
>> - x64 JIT recognizes bpf_skb_vlan_push/pop() calls and re-caches skb->data/hlen
>>    after such calls (experiments showed that conditional re-caching is slower).
>> - s390 JIT falls back to interpreter for now when bpf_skb_vlan_push() is present
>>    in the program (re-caching is tbd).
>
>
>
>> +static u64 bpf_skb_vlan_push(u64 r1, u64 vlan_proto, u64 vlan_tci, u64 r4, u64 r5)
>> +{
>> +	struct sk_buff *skb = (struct sk_buff *) (long) r1;
>> +
>> +	if (unlikely(vlan_proto != htons(ETH_P_8021Q) &&
>> +		     vlan_proto != htons(ETH_P_8021AD)))
>> +		vlan_proto = htons(ETH_P_8021Q);
>
>
> This would raise sparse error, as vlan_proto is u64, and htons() __be16
>
> make C=2 CF=-D__CHECK_ENDIAN__ net/core/filter.o

yes.
When I wrote these lines I thought of the same, so I did run the sparse
and it spewed a lot of false positives and stopped on 'too many errors'
before reaching these lines.
So I downloaded the latest sparse, hacked it a little and tried again.
Still it didn't complain about the endianness. That was puzzling,
so I left the above lines as-is.
but since your eagle eyes caught it, I will add casts :)

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-07-17 18:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-17  2:58 [PATCH net-next 0/2] bpf: introduce bpf_skb_vlan_push/pop() helpers Alexei Starovoitov
2015-07-17  2:58 ` [PATCH net-next 1/2] " Alexei Starovoitov
2015-07-17  8:12   ` Eric Dumazet
2015-07-17 18:17     ` Alexei Starovoitov
2015-07-17  2:58 ` [PATCH net-next 2/2] test_bpf: add bpf_skb_vlan_push/pop() tests Alexei Starovoitov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.