Skip to contentSkip to conversation

Two BPF smap related followups

PATCH In review Daniel Borkmann 4 participants 8 messages Aug 17, 2017

Subsystems (4)
Networking Network drivers BPF BPF networking
lore
Share & tools
lore

Daniel Borkmann

author maintainer
#0 lore raw

cover Two BPF smap related followups

Fixing preemption imbalance and consolidating prologue generation. Thanks!

Daniel Borkmann (2):
bpf: don't enable preemption twice in smap_do_verdict
bpf: reuse tc bpf prologue for sk skb progs

 kernel/bpf/sockmap.c |  3 ++-
 net/core/filter.c    | 47 ++++++++++-------------------------------------
 2 files changed, 12 insertions(+), 38 deletions(-)
Signature · 1 line
1.9.3
author maintainer
#1 lore raw

PATCH 1/2 bpf: don't enable preemption twice in smap_do_verdict

In smap_do_verdict(), the fall-through branch leads to call preempt_enable() twice for the SK_REDIRECT, which creates an imbalance. Only enable it for all remaining cases again.

Fixes 174a79ff9515 ("bpf: sockmap with sk redirect support")
Reported-by Alexei Starovoitov <[email protected]>
Signed-off-by Daniel Borkmann <[email protected]>
---
 kernel/bpf/sockmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/bpf/sockmap.c +2 -1
index f7e5e6c..39de541 100644
@@ -135,7 +135,8 @@ static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb)
 	/* Fall through and free skb otherwise */
 	case SK_DROP:
 	default:
-		preempt_enable();
+		if (rc != SK_REDIRECT)
+			preempt_enable();
 		kfree_skb(skb);
 	}
 }
Signature · 1 line
1.9.3
reviewer Acked-by on v1 1/2
#2 lore raw

Re: [PATCH net-next 1/2] bpf: don't enable preemption twice in smap_do_verdict

Quoted 17 lines · Daniel Borkmann

In smap_do_verdict(), the fall-through branch leads to call preempt_enable() twice for the SK_REDIRECT, which creates an imbalance. Only enable it for all remaining cases again.

Fixes 174a79ff9515 ("bpf: sockmap with sk redirect support")
Reported-by Alexei Starovoitov <[email protected]>
Signed-off-by Daniel Borkmann <[email protected]>
---
 kernel/bpf/sockmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/bpf/sockmap.c +0 -0
index f7e5e6c..39de541 100644

kernel/bpf/sockmap.c static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb) · line 142

Quoted 9 lines
kernel/bpf/sockmap.c +2 -1
@@ -135,7 +135,8 @@ static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb)
 	/* Fall through and free skb otherwise */
 	case SK_DROP:
 	default:
-		preempt_enable();
+		if (rc != SK_REDIRECT)
+			preempt_enable();
 		kfree_skb(skb);
 	}
 }

Yep looks good, nice catch Alexei. Thanks! I'll add a selftests entry in test_maps to catch this fall through case. Looks like we don't hit this case during selftests at the moment.

Acked-by John Fastabend <[email protected]>
Acked-by
#3 lore raw

Re: [PATCH net-next 1/2] bpf: don't enable preemption twice in smap_do_verdict

Quoted 32 lines · John Fastabend
Quoted 17 lines · Daniel Borkmann

In smap_do_verdict(), the fall-through branch leads to call preempt_enable() twice for the SK_REDIRECT, which creates an imbalance. Only enable it for all remaining cases again.

Fixes 174a79ff9515 ("bpf: sockmap with sk redirect support")
Reported-by Alexei Starovoitov <[email protected]>
Signed-off-by Daniel Borkmann <[email protected]>
---
 kernel/bpf/sockmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/bpf/sockmap.c +0 -0
index f7e5e6c..39de541 100644

kernel/bpf/sockmap.c static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb) · line 142

Quoted 9 lines
kernel/bpf/sockmap.c +2 -1
@@ -135,7 +135,8 @@ static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb)
 	/* Fall through and free skb otherwise */
 	case SK_DROP:
 	default:
-		preempt_enable();
+		if (rc != SK_REDIRECT)
+			preempt_enable();
 		kfree_skb(skb);
 	}
 }

Yep looks good, nice catch Alexei. Thanks! I'll add a selftests entry in test_maps to catch this fall through case. Looks like we don't hit this case during selftests at the moment.

Quoted 2 lines
Acked-by John Fastabend <[email protected]>
Acked-by Alexei Starovoitov <[email protected]>
author maintainer
#4 lore raw

PATCH 2/2 bpf: reuse tc bpf prologue for sk skb progs

In reply to Daniel Borkmann

Given both program types are effecitvely doing the same in the prologue, just reuse the one that we had for tc and only adapt to the corresponding drop verdict value. That way, we don't need to have the duplicate from 8a31db561566 ("bpf: add access to sock fields and pkt data from sk_skb programs") to maintain.

Reported-by Alexei Starovoitov <[email protected]>
Signed-off-by Daniel Borkmann <[email protected]>
---
 net/core/filter.c | 47 ++++++++++-------------------------------------
 1 file changed, 10 insertions(+), 37 deletions(-)
net/core/filter.c +10 -37
index e9f8dce..9865a98 100644
@@ -3456,8 +3456,8 @@ static bool sock_filter_is_valid_access(int off, int size,
 	return true;
 }
 
-static int tc_cls_act_prologue(struct bpf_insn *insn_buf, bool direct_write,
-			       const struct bpf_prog *prog)
+static int bpf_unclone_prologue(struct bpf_insn *insn_buf, bool direct_write,
+				const struct bpf_prog *prog, int drop_verdict)
 {
 	struct bpf_insn *insn = insn_buf;
 

@@ -3484,7 +3484,7 @@ static int tc_cls_act_prologue(struct bpf_insn *insn_buf, bool direct_write,
 	 * return TC_ACT_SHOT;
 	 */
 	*insn++ = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2);
-	*insn++ = BPF_ALU32_IMM(BPF_MOV, BPF_REG_0, TC_ACT_SHOT);
+	*insn++ = BPF_ALU32_IMM(BPF_MOV, BPF_REG_0, drop_verdict);
 	*insn++ = BPF_EXIT_INSN();
 
 	/* restore: */

@@ -3495,6 +3495,12 @@ static int tc_cls_act_prologue(struct bpf_insn *insn_buf, bool direct_write,
 	return insn - insn_buf;
 }
 
+static int tc_cls_act_prologue(struct bpf_insn *insn_buf, bool direct_write,
+			       const struct bpf_prog *prog)
+{
+	return bpf_unclone_prologue(insn_buf, direct_write, prog, TC_ACT_SHOT);
+}
+
 static bool tc_cls_act_is_valid_access(int off, int size,
 				       enum bpf_access_type type,
 				       struct bpf_insn_access_aux *info)

@@ -3601,40 +3607,7 @@ static bool sock_ops_is_valid_access(int off, int size,
 static int sk_skb_prologue(struct bpf_insn *insn_buf, bool direct_write,
 			   const struct bpf_prog *prog)
 {
-	struct bpf_insn *insn = insn_buf;
-
-	if (!direct_write)
-		return 0;
-
-	/* if (!skb->cloned)
-	 *       goto start;
-	 *
-	 * (Fast-path, otherwise approximation that we might be
-	 *  a clone, do the rest in helper.)
-	 */
-	*insn++ = BPF_LDX_MEM(BPF_B, BPF_REG_6, BPF_REG_1, CLONED_OFFSET());
-	*insn++ = BPF_ALU32_IMM(BPF_AND, BPF_REG_6, CLONED_MASK);
-	*insn++ = BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 7);
-
-	/* ret = bpf_skb_pull_data(skb, 0); */
-	*insn++ = BPF_MOV64_REG(BPF_REG_6, BPF_REG_1);
-	*insn++ = BPF_ALU64_REG(BPF_XOR, BPF_REG_2, BPF_REG_2);
-	*insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
-			       BPF_FUNC_skb_pull_data);
-	/* if (!ret)
-	 *      goto restore;
-	 * return SK_DROP;
-	 */
-	*insn++ = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2);
-	*insn++ = BPF_ALU32_IMM(BPF_MOV, BPF_REG_0, SK_DROP);
-	*insn++ = BPF_EXIT_INSN();
-
-	/* restore: */
-	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_6);
-	/* start: */
-	*insn++ = prog->insnsi[0];
-
-	return insn - insn_buf;
+	return bpf_unclone_prologue(insn_buf, direct_write, prog, SK_DROP);
 }
 
 static bool sk_skb_is_valid_access(int off, int size,
Signature · 1 line
1.9.3
reviewer Acked-by on v1 2/2
#5 lore raw

Re: [PATCH net-next 2/2] bpf: reuse tc bpf prologue for sk skb progs

Quoted 6 lines · Daniel Borkmann

Given both program types are effecitvely doing the same in the prologue, just reuse the one that we had for tc and only adapt to the corresponding drop verdict value. That way, we don't need to have the duplicate from 8a31db561566 ("bpf: add access to sock fields and pkt data from sk_skb programs") to maintain.

Quoted 6 lines
Reported-by Alexei Starovoitov <[email protected]>
Signed-off-by Daniel Borkmann <[email protected]>
---
 net/core/filter.c | 47 ++++++++++-------------------------------------
 1 file changed, 10 insertions(+), 37 deletions(-)

Nice clean-up, Thanks.

Acked-by John Fastabend <[email protected]>
Acked-by
#6 lore raw

Re: [PATCH net-next 2/2] bpf: reuse tc bpf prologue for sk skb progs

Quoted 14 lines · John Fastabend
Quoted 6 lines · Daniel Borkmann

Given both program types are effecitvely doing the same in the prologue, just reuse the one that we had for tc and only adapt to the corresponding drop verdict value. That way, we don't need to have the duplicate from 8a31db561566 ("bpf: add access to sock fields and pkt data from sk_skb programs") to maintain.

Quoted 6 lines
Reported-by Alexei Starovoitov <[email protected]>
Signed-off-by Daniel Borkmann <[email protected]>
---
 net/core/filter.c | 47 ++++++++++-------------------------------------
 1 file changed, 10 insertions(+), 37 deletions(-)
Quoted 4 lines

Nice clean-up, Thanks.

Acked-by John Fastabend <[email protected]>
Acked-by Alexei Starovoitov <[email protected]>