* [PATCH net-next v3 0/3] BPF cleanups
@ 2014-05-01 16:16 Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table Daniel Borkmann
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Daniel Borkmann @ 2014-05-01 16:16 UTC (permalink / raw)
To: davem; +Cc: ast, netdev
v2->v3:
- Included Dave's feedback for unsigned long type in patch 3
- Patch 1 and patch 2 unchanged since v1, dropped other
two for now
v1->v2:
- Only changed patch 5 as to suggestion from Alexei
- Rest is the same
Daniel Borkmann (3):
net: filter: simplify label names from jump-table
net: filter: make register namings more comprehensible
net: filter: misc/various cleanups
include/linux/filter.h | 47 ++++-
net/core/filter.c | 538 +++++++++++++++++++++++++------------------------
2 files changed, 309 insertions(+), 276 deletions(-)
--
1.7.11.7
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table
2014-05-01 16:16 [PATCH net-next v3 0/3] BPF cleanups Daniel Borkmann
@ 2014-05-01 16:16 ` Daniel Borkmann
2014-05-01 17:20 ` Eric Dumazet
2014-05-01 16:16 ` [PATCH net-next v3 2/3] net: filter: make register namings more comprehensible Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 3/3] net: filter: misc/various cleanups Daniel Borkmann
2 siblings, 1 reply; 8+ messages in thread
From: Daniel Borkmann @ 2014-05-01 16:16 UTC (permalink / raw)
To: davem; +Cc: ast, netdev
This patch simplifies label naming for the BPF jump-table.
When we define labels via DL(), we just concatenate/textify
the combination of instruction opcode which consists of the
class, subclass, word size, target register and so on. Each
time we leave BPF_ prefix intact, so that e.g. the preprocessor
generates a label BPF_ALU_BPF_ADD_BPF_X for DL(BPF_ALU, BPF_ADD,
BPF_X) whereas a label name of ALU_ADD_X is much more easy
to grasp. Pure cleanup only.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
include/linux/filter.h | 3 +
net/core/filter.c | 308 ++++++++++++++++++++++++-------------------------
2 files changed, 157 insertions(+), 154 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 759abf7..b042d1d 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -37,6 +37,9 @@
#define BPF_CALL 0x80 /* function call */
#define BPF_EXIT 0x90 /* function return */
+/* Placeholder/dummy for 0 */
+#define BPF_0 0
+
/* BPF has 10 general purpose 64-bit registers and stack frame. */
#define MAX_BPF_REG 11
diff --git a/net/core/filter.c b/net/core/filter.c
index 7c4db3d..a1784e9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -156,94 +156,94 @@ unsigned int __sk_run_filter(void *ctx, const struct sock_filter_int *insn)
static const void *jumptable[256] = {
[0 ... 255] = &&default_label,
/* Now overwrite non-defaults ... */
-#define DL(A, B, C) [A|B|C] = &&A##_##B##_##C
- DL(BPF_ALU, BPF_ADD, BPF_X),
- DL(BPF_ALU, BPF_ADD, BPF_K),
- DL(BPF_ALU, BPF_SUB, BPF_X),
- DL(BPF_ALU, BPF_SUB, BPF_K),
- DL(BPF_ALU, BPF_AND, BPF_X),
- DL(BPF_ALU, BPF_AND, BPF_K),
- DL(BPF_ALU, BPF_OR, BPF_X),
- DL(BPF_ALU, BPF_OR, BPF_K),
- DL(BPF_ALU, BPF_LSH, BPF_X),
- DL(BPF_ALU, BPF_LSH, BPF_K),
- DL(BPF_ALU, BPF_RSH, BPF_X),
- DL(BPF_ALU, BPF_RSH, BPF_K),
- DL(BPF_ALU, BPF_XOR, BPF_X),
- DL(BPF_ALU, BPF_XOR, BPF_K),
- DL(BPF_ALU, BPF_MUL, BPF_X),
- DL(BPF_ALU, BPF_MUL, BPF_K),
- DL(BPF_ALU, BPF_MOV, BPF_X),
- DL(BPF_ALU, BPF_MOV, BPF_K),
- DL(BPF_ALU, BPF_DIV, BPF_X),
- DL(BPF_ALU, BPF_DIV, BPF_K),
- DL(BPF_ALU, BPF_MOD, BPF_X),
- DL(BPF_ALU, BPF_MOD, BPF_K),
- DL(BPF_ALU, BPF_NEG, 0),
- DL(BPF_ALU, BPF_END, BPF_TO_BE),
- DL(BPF_ALU, BPF_END, BPF_TO_LE),
- DL(BPF_ALU64, BPF_ADD, BPF_X),
- DL(BPF_ALU64, BPF_ADD, BPF_K),
- DL(BPF_ALU64, BPF_SUB, BPF_X),
- DL(BPF_ALU64, BPF_SUB, BPF_K),
- DL(BPF_ALU64, BPF_AND, BPF_X),
- DL(BPF_ALU64, BPF_AND, BPF_K),
- DL(BPF_ALU64, BPF_OR, BPF_X),
- DL(BPF_ALU64, BPF_OR, BPF_K),
- DL(BPF_ALU64, BPF_LSH, BPF_X),
- DL(BPF_ALU64, BPF_LSH, BPF_K),
- DL(BPF_ALU64, BPF_RSH, BPF_X),
- DL(BPF_ALU64, BPF_RSH, BPF_K),
- DL(BPF_ALU64, BPF_XOR, BPF_X),
- DL(BPF_ALU64, BPF_XOR, BPF_K),
- DL(BPF_ALU64, BPF_MUL, BPF_X),
- DL(BPF_ALU64, BPF_MUL, BPF_K),
- DL(BPF_ALU64, BPF_MOV, BPF_X),
- DL(BPF_ALU64, BPF_MOV, BPF_K),
- DL(BPF_ALU64, BPF_ARSH, BPF_X),
- DL(BPF_ALU64, BPF_ARSH, BPF_K),
- DL(BPF_ALU64, BPF_DIV, BPF_X),
- DL(BPF_ALU64, BPF_DIV, BPF_K),
- DL(BPF_ALU64, BPF_MOD, BPF_X),
- DL(BPF_ALU64, BPF_MOD, BPF_K),
- DL(BPF_ALU64, BPF_NEG, 0),
- DL(BPF_JMP, BPF_CALL, 0),
- DL(BPF_JMP, BPF_JA, 0),
- DL(BPF_JMP, BPF_JEQ, BPF_X),
- DL(BPF_JMP, BPF_JEQ, BPF_K),
- DL(BPF_JMP, BPF_JNE, BPF_X),
- DL(BPF_JMP, BPF_JNE, BPF_K),
- DL(BPF_JMP, BPF_JGT, BPF_X),
- DL(BPF_JMP, BPF_JGT, BPF_K),
- DL(BPF_JMP, BPF_JGE, BPF_X),
- DL(BPF_JMP, BPF_JGE, BPF_K),
- DL(BPF_JMP, BPF_JSGT, BPF_X),
- DL(BPF_JMP, BPF_JSGT, BPF_K),
- DL(BPF_JMP, BPF_JSGE, BPF_X),
- DL(BPF_JMP, BPF_JSGE, BPF_K),
- DL(BPF_JMP, BPF_JSET, BPF_X),
- DL(BPF_JMP, BPF_JSET, BPF_K),
- DL(BPF_JMP, BPF_EXIT, 0),
- DL(BPF_STX, BPF_MEM, BPF_B),
- DL(BPF_STX, BPF_MEM, BPF_H),
- DL(BPF_STX, BPF_MEM, BPF_W),
- DL(BPF_STX, BPF_MEM, BPF_DW),
- DL(BPF_STX, BPF_XADD, BPF_W),
- DL(BPF_STX, BPF_XADD, BPF_DW),
- DL(BPF_ST, BPF_MEM, BPF_B),
- DL(BPF_ST, BPF_MEM, BPF_H),
- DL(BPF_ST, BPF_MEM, BPF_W),
- DL(BPF_ST, BPF_MEM, BPF_DW),
- DL(BPF_LDX, BPF_MEM, BPF_B),
- DL(BPF_LDX, BPF_MEM, BPF_H),
- DL(BPF_LDX, BPF_MEM, BPF_W),
- DL(BPF_LDX, BPF_MEM, BPF_DW),
- DL(BPF_LD, BPF_ABS, BPF_W),
- DL(BPF_LD, BPF_ABS, BPF_H),
- DL(BPF_LD, BPF_ABS, BPF_B),
- DL(BPF_LD, BPF_IND, BPF_W),
- DL(BPF_LD, BPF_IND, BPF_H),
- DL(BPF_LD, BPF_IND, BPF_B),
+#define DL(A, B, C) [BPF_##A|BPF_##B|BPF_##C] = &&A##_##B##_##C
+ DL(ALU, ADD, X),
+ DL(ALU, ADD, K),
+ DL(ALU, SUB, X),
+ DL(ALU, SUB, K),
+ DL(ALU, AND, X),
+ DL(ALU, AND, K),
+ DL(ALU, OR, X),
+ DL(ALU, OR, K),
+ DL(ALU, LSH, X),
+ DL(ALU, LSH, K),
+ DL(ALU, RSH, X),
+ DL(ALU, RSH, K),
+ DL(ALU, XOR, X),
+ DL(ALU, XOR, K),
+ DL(ALU, MUL, X),
+ DL(ALU, MUL, K),
+ DL(ALU, MOV, X),
+ DL(ALU, MOV, K),
+ DL(ALU, DIV, X),
+ DL(ALU, DIV, K),
+ DL(ALU, MOD, X),
+ DL(ALU, MOD, K),
+ DL(ALU, NEG, 0),
+ DL(ALU, END, TO_BE),
+ DL(ALU, END, TO_LE),
+ DL(ALU64, ADD, X),
+ DL(ALU64, ADD, K),
+ DL(ALU64, SUB, X),
+ DL(ALU64, SUB, K),
+ DL(ALU64, AND, X),
+ DL(ALU64, AND, K),
+ DL(ALU64, OR, X),
+ DL(ALU64, OR, K),
+ DL(ALU64, LSH, X),
+ DL(ALU64, LSH, K),
+ DL(ALU64, RSH, X),
+ DL(ALU64, RSH, K),
+ DL(ALU64, XOR, X),
+ DL(ALU64, XOR, K),
+ DL(ALU64, MUL, X),
+ DL(ALU64, MUL, K),
+ DL(ALU64, MOV, X),
+ DL(ALU64, MOV, K),
+ DL(ALU64, ARSH, X),
+ DL(ALU64, ARSH, K),
+ DL(ALU64, DIV, X),
+ DL(ALU64, DIV, K),
+ DL(ALU64, MOD, X),
+ DL(ALU64, MOD, K),
+ DL(ALU64, NEG, 0),
+ DL(JMP, CALL, 0),
+ DL(JMP, JA, 0),
+ DL(JMP, JEQ, X),
+ DL(JMP, JEQ, K),
+ DL(JMP, JNE, X),
+ DL(JMP, JNE, K),
+ DL(JMP, JGT, X),
+ DL(JMP, JGT, K),
+ DL(JMP, JGE, X),
+ DL(JMP, JGE, K),
+ DL(JMP, JSGT, X),
+ DL(JMP, JSGT, K),
+ DL(JMP, JSGE, X),
+ DL(JMP, JSGE, K),
+ DL(JMP, JSET, X),
+ DL(JMP, JSET, K),
+ DL(JMP, EXIT, 0),
+ DL(STX, MEM, B),
+ DL(STX, MEM, H),
+ DL(STX, MEM, W),
+ DL(STX, MEM, DW),
+ DL(STX, XADD, W),
+ DL(STX, XADD, DW),
+ DL(ST, MEM, B),
+ DL(ST, MEM, H),
+ DL(ST, MEM, W),
+ DL(ST, MEM, DW),
+ DL(LDX, MEM, B),
+ DL(LDX, MEM, H),
+ DL(LDX, MEM, W),
+ DL(LDX, MEM, DW),
+ DL(LD, ABS, W),
+ DL(LD, ABS, H),
+ DL(LD, ABS, B),
+ DL(LD, IND, W),
+ DL(LD, IND, H),
+ DL(LD, IND, B),
#undef DL
};
@@ -257,93 +257,93 @@ select_insn:
/* ALU */
#define ALU(OPCODE, OP) \
- BPF_ALU64_##OPCODE##_BPF_X: \
+ ALU64_##OPCODE##_X: \
A = A OP X; \
CONT; \
- BPF_ALU_##OPCODE##_BPF_X: \
+ ALU_##OPCODE##_X: \
A = (u32) A OP (u32) X; \
CONT; \
- BPF_ALU64_##OPCODE##_BPF_K: \
+ ALU64_##OPCODE##_K: \
A = A OP K; \
CONT; \
- BPF_ALU_##OPCODE##_BPF_K: \
+ ALU_##OPCODE##_K: \
A = (u32) A OP (u32) K; \
CONT;
- ALU(BPF_ADD, +)
- ALU(BPF_SUB, -)
- ALU(BPF_AND, &)
- ALU(BPF_OR, |)
- ALU(BPF_LSH, <<)
- ALU(BPF_RSH, >>)
- ALU(BPF_XOR, ^)
- ALU(BPF_MUL, *)
+ ALU(ADD, +)
+ ALU(SUB, -)
+ ALU(AND, &)
+ ALU(OR, |)
+ ALU(LSH, <<)
+ ALU(RSH, >>)
+ ALU(XOR, ^)
+ ALU(MUL, *)
#undef ALU
- BPF_ALU_BPF_NEG_0:
+ ALU_NEG_0:
A = (u32) -A;
CONT;
- BPF_ALU64_BPF_NEG_0:
+ ALU64_NEG_0:
A = -A;
CONT;
- BPF_ALU_BPF_MOV_BPF_X:
+ ALU_MOV_X:
A = (u32) X;
CONT;
- BPF_ALU_BPF_MOV_BPF_K:
+ ALU_MOV_K:
A = (u32) K;
CONT;
- BPF_ALU64_BPF_MOV_BPF_X:
+ ALU64_MOV_X:
A = X;
CONT;
- BPF_ALU64_BPF_MOV_BPF_K:
+ ALU64_MOV_K:
A = K;
CONT;
- BPF_ALU64_BPF_ARSH_BPF_X:
+ ALU64_ARSH_X:
(*(s64 *) &A) >>= X;
CONT;
- BPF_ALU64_BPF_ARSH_BPF_K:
+ ALU64_ARSH_K:
(*(s64 *) &A) >>= K;
CONT;
- BPF_ALU64_BPF_MOD_BPF_X:
+ ALU64_MOD_X:
if (unlikely(X == 0))
return 0;
tmp = A;
A = do_div(tmp, X);
CONT;
- BPF_ALU_BPF_MOD_BPF_X:
+ ALU_MOD_X:
if (unlikely(X == 0))
return 0;
tmp = (u32) A;
A = do_div(tmp, (u32) X);
CONT;
- BPF_ALU64_BPF_MOD_BPF_K:
+ ALU64_MOD_K:
tmp = A;
A = do_div(tmp, K);
CONT;
- BPF_ALU_BPF_MOD_BPF_K:
+ ALU_MOD_K:
tmp = (u32) A;
A = do_div(tmp, (u32) K);
CONT;
- BPF_ALU64_BPF_DIV_BPF_X:
+ ALU64_DIV_X:
if (unlikely(X == 0))
return 0;
do_div(A, X);
CONT;
- BPF_ALU_BPF_DIV_BPF_X:
+ ALU_DIV_X:
if (unlikely(X == 0))
return 0;
tmp = (u32) A;
do_div(tmp, (u32) X);
A = (u32) tmp;
CONT;
- BPF_ALU64_BPF_DIV_BPF_K:
+ ALU64_DIV_K:
do_div(A, K);
CONT;
- BPF_ALU_BPF_DIV_BPF_K:
+ ALU_DIV_K:
tmp = (u32) A;
do_div(tmp, (u32) K);
A = (u32) tmp;
CONT;
- BPF_ALU_BPF_END_BPF_TO_BE:
+ ALU_END_TO_BE:
switch (K) {
case 16:
A = (__force u16) cpu_to_be16(A);
@@ -356,7 +356,7 @@ select_insn:
break;
}
CONT;
- BPF_ALU_BPF_END_BPF_TO_LE:
+ ALU_END_TO_LE:
switch (K) {
case 16:
A = (__force u16) cpu_to_le16(A);
@@ -371,7 +371,7 @@ select_insn:
CONT;
/* CALL */
- BPF_JMP_BPF_CALL_0:
+ JMP_CALL_0:
/* Function call scratches R1-R5 registers, preserves R6-R9,
* and stores return value into R0.
*/
@@ -380,122 +380,122 @@ select_insn:
CONT;
/* JMP */
- BPF_JMP_BPF_JA_0:
+ JMP_JA_0:
insn += insn->off;
CONT;
- BPF_JMP_BPF_JEQ_BPF_X:
+ JMP_JEQ_X:
if (A == X) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JEQ_BPF_K:
+ JMP_JEQ_K:
if (A == K) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JNE_BPF_X:
+ JMP_JNE_X:
if (A != X) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JNE_BPF_K:
+ JMP_JNE_K:
if (A != K) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JGT_BPF_X:
+ JMP_JGT_X:
if (A > X) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JGT_BPF_K:
+ JMP_JGT_K:
if (A > K) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JGE_BPF_X:
+ JMP_JGE_X:
if (A >= X) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JGE_BPF_K:
+ JMP_JGE_K:
if (A >= K) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JSGT_BPF_X:
- if (((s64)A) > ((s64)X)) {
+ JMP_JSGT_X:
+ if (((s64) A) > ((s64) X)) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JSGT_BPF_K:
- if (((s64)A) > ((s64)K)) {
+ JMP_JSGT_K:
+ if (((s64) A) > ((s64) K)) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JSGE_BPF_X:
- if (((s64)A) >= ((s64)X)) {
+ JMP_JSGE_X:
+ if (((s64) A) >= ((s64) X)) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JSGE_BPF_K:
- if (((s64)A) >= ((s64)K)) {
+ JMP_JSGE_K:
+ if (((s64) A) >= ((s64) K)) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JSET_BPF_X:
+ JMP_JSET_X:
if (A & X) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_JSET_BPF_K:
+ JMP_JSET_K:
if (A & K) {
insn += insn->off;
CONT_JMP;
}
CONT;
- BPF_JMP_BPF_EXIT_0:
+ JMP_EXIT_0:
return R0;
/* STX and ST and LDX*/
#define LDST(SIZEOP, SIZE) \
- BPF_STX_BPF_MEM_##SIZEOP: \
+ STX_MEM_##SIZEOP: \
*(SIZE *)(unsigned long) (A + insn->off) = X; \
CONT; \
- BPF_ST_BPF_MEM_##SIZEOP: \
+ ST_MEM_##SIZEOP: \
*(SIZE *)(unsigned long) (A + insn->off) = K; \
CONT; \
- BPF_LDX_BPF_MEM_##SIZEOP: \
+ LDX_MEM_##SIZEOP: \
A = *(SIZE *)(unsigned long) (X + insn->off); \
CONT;
- LDST(BPF_B, u8)
- LDST(BPF_H, u16)
- LDST(BPF_W, u32)
- LDST(BPF_DW, u64)
+ LDST(B, u8)
+ LDST(H, u16)
+ LDST(W, u32)
+ LDST(DW, u64)
#undef LDST
- BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
+ STX_XADD_W: /* lock xadd *(u32 *)(A + insn->off) += X */
atomic_add((u32) X, (atomic_t *)(unsigned long)
(A + insn->off));
CONT;
- BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
+ STX_XADD_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
atomic64_add((u64) X, (atomic64_t *)(unsigned long)
(A + insn->off));
CONT;
- BPF_LD_BPF_ABS_BPF_W: /* R0 = ntohl(*(u32 *) (skb->data + K)) */
+ LD_ABS_W: /* R0 = ntohl(*(u32 *) (skb->data + K)) */
off = K;
load_word:
/* BPF_LD + BPD_ABS and BPF_LD + BPF_IND insns are only
@@ -524,7 +524,7 @@ load_word:
CONT;
}
return 0;
- BPF_LD_BPF_ABS_BPF_H: /* R0 = ntohs(*(u16 *) (skb->data + K)) */
+ LD_ABS_H: /* R0 = ntohs(*(u16 *) (skb->data + K)) */
off = K;
load_half:
ptr = load_pointer((struct sk_buff *) ctx, off, 2, &tmp);
@@ -533,7 +533,7 @@ load_half:
CONT;
}
return 0;
- BPF_LD_BPF_ABS_BPF_B: /* R0 = *(u8 *) (ctx + K) */
+ LD_ABS_B: /* R0 = *(u8 *) (ctx + K) */
off = K;
load_byte:
ptr = load_pointer((struct sk_buff *) ctx, off, 1, &tmp);
@@ -542,13 +542,13 @@ load_byte:
CONT;
}
return 0;
- BPF_LD_BPF_IND_BPF_W: /* R0 = ntohl(*(u32 *) (skb->data + X + K)) */
+ LD_IND_W: /* R0 = ntohl(*(u32 *) (skb->data + X + K)) */
off = K + X;
goto load_word;
- BPF_LD_BPF_IND_BPF_H: /* R0 = ntohs(*(u16 *) (skb->data + X + K)) */
+ LD_IND_H: /* R0 = ntohs(*(u16 *) (skb->data + X + K)) */
off = K + X;
goto load_half;
- BPF_LD_BPF_IND_BPF_B: /* R0 = *(u8 *) (skb->data + X + K) */
+ LD_IND_B: /* R0 = *(u8 *) (skb->data + X + K) */
off = K + X;
goto load_byte;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next v3 2/3] net: filter: make register namings more comprehensible
2014-05-01 16:16 [PATCH net-next v3 0/3] BPF cleanups Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table Daniel Borkmann
@ 2014-05-01 16:16 ` Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 3/3] net: filter: misc/various cleanups Daniel Borkmann
2 siblings, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2014-05-01 16:16 UTC (permalink / raw)
To: davem; +Cc: ast, netdev
The current code is a bit hard to parse on which registers can be used,
how they are mapped and all play together. It makes much more sense to
define this a bit more clearly so that the code is a bit more intuitive.
This patch cleans this up, and makes naming a bit more consistent among
the code. This also allows for moving some of the defines into the header
file. Clearing of A and X registers in __sk_run_filter() do not get a
particular register name assigned as they have not an 'official' function,
but rather just result from the concrete initial mapping of old BPF
programs. Since for BPF helper functions for BPF_CALL we already use
small letters, so be consistent here as well. No functional changes.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
include/linux/filter.h | 44 ++++++++--
net/core/filter.c | 215 +++++++++++++++++++++++++------------------------
2 files changed, 145 insertions(+), 114 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index b042d1d..ed1efab 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -40,16 +40,47 @@
/* Placeholder/dummy for 0 */
#define BPF_0 0
+/* Register numbers */
+enum {
+ BPF_REG_0 = 0,
+ BPF_REG_1,
+ BPF_REG_2,
+ BPF_REG_3,
+ BPF_REG_4,
+ BPF_REG_5,
+ BPF_REG_6,
+ BPF_REG_7,
+ BPF_REG_8,
+ BPF_REG_9,
+ BPF_REG_10,
+ __MAX_BPF_REG,
+};
+
/* BPF has 10 general purpose 64-bit registers and stack frame. */
-#define MAX_BPF_REG 11
+#define MAX_BPF_REG __MAX_BPF_REG
+
+/* ArgX, context and stack frame pointer register positions. Note,
+ * Arg1, Arg2, Arg3, etc are used as argument mappings of function
+ * calls in BPF_CALL instruction.
+ */
+#define BPF_REG_ARG1 BPF_REG_1
+#define BPF_REG_ARG2 BPF_REG_2
+#define BPF_REG_ARG3 BPF_REG_3
+#define BPF_REG_ARG4 BPF_REG_4
+#define BPF_REG_ARG5 BPF_REG_5
+#define BPF_REG_CTX BPF_REG_6
+#define BPF_REG_FP BPF_REG_10
+
+/* Additional register mappings for converted user programs. */
+#define BPF_REG_A BPF_REG_0
+#define BPF_REG_X BPF_REG_7
+#define BPF_REG_TMP BPF_REG_8
/* BPF program can access up to 512 bytes of stack space. */
#define MAX_BPF_STACK 512
-/* Arg1, context and stack frame pointer register positions. */
-#define ARG1_REG 1
-#define CTX_REG 6
-#define FP_REG 10
+/* Macro to invoke filter function. */
+#define SK_RUN_FILTER(filter, ctx) (*filter->bpf_func)(ctx, filter->insnsi)
struct sock_filter_int {
__u8 code; /* opcode */
@@ -100,9 +131,6 @@ static inline unsigned int sk_filter_size(unsigned int proglen)
#define sk_filter_proglen(fprog) \
(fprog->len * sizeof(fprog->filter[0]))
-#define SK_RUN_FILTER(filter, ctx) \
- (*filter->bpf_func)(ctx, filter->insnsi)
-
int sk_filter(struct sock *sk, struct sk_buff *skb);
u32 sk_run_filter_int_seccomp(const struct seccomp_data *ctx,
diff --git a/net/core/filter.c b/net/core/filter.c
index a1784e9..c93ca8d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -45,6 +45,27 @@
#include <linux/seccomp.h>
#include <linux/if_vlan.h>
+/* Registers */
+#define R0 regs[BPF_REG_0]
+#define R1 regs[BPF_REG_1]
+#define R2 regs[BPF_REG_2]
+#define R3 regs[BPF_REG_3]
+#define R4 regs[BPF_REG_4]
+#define R5 regs[BPF_REG_5]
+#define R6 regs[BPF_REG_6]
+#define R7 regs[BPF_REG_7]
+#define R8 regs[BPF_REG_8]
+#define R9 regs[BPF_REG_9]
+#define R10 regs[BPF_REG_10]
+
+/* Named registers */
+#define A regs[insn->a_reg]
+#define X regs[insn->x_reg]
+#define FP regs[BPF_REG_FP]
+#define ARG1 regs[BPF_REG_ARG1]
+#define CTX regs[BPF_REG_CTX]
+#define K insn->imm
+
/* No hurry in this branch
*
* Exported for the bpf jit load helper.
@@ -122,13 +143,6 @@ noinline u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
return 0;
}
-/* Register mappings for user programs. */
-#define A_REG 0
-#define X_REG 7
-#define TMP_REG 8
-#define ARG2_REG 2
-#define ARG3_REG 3
-
/**
* __sk_run_filter - run a filter on a given context
* @ctx: buffer to run the filter on
@@ -142,17 +156,6 @@ unsigned int __sk_run_filter(void *ctx, const struct sock_filter_int *insn)
{
u64 stack[MAX_BPF_STACK / sizeof(u64)];
u64 regs[MAX_BPF_REG], tmp;
- void *ptr;
- int off;
-
-#define K insn->imm
-#define A regs[insn->a_reg]
-#define X regs[insn->x_reg]
-#define R0 regs[0]
-
-#define CONT ({insn++; goto select_insn; })
-#define CONT_JMP ({insn++; goto select_insn; })
-
static const void *jumptable[256] = {
[0 ... 255] = &&default_label,
/* Now overwrite non-defaults ... */
@@ -246,11 +249,18 @@ unsigned int __sk_run_filter(void *ctx, const struct sock_filter_int *insn)
DL(LD, IND, B),
#undef DL
};
+ void *ptr;
+ int off;
- regs[FP_REG] = (u64) (unsigned long) &stack[ARRAY_SIZE(stack)];
- regs[ARG1_REG] = (u64) (unsigned long) ctx;
- regs[A_REG] = 0;
- regs[X_REG] = 0;
+#define CONT ({ insn++; goto select_insn; })
+#define CONT_JMP ({ insn++; goto select_insn; })
+
+ FP = (u64) (unsigned long) &stack[ARRAY_SIZE(stack)];
+ ARG1 = (u64) (unsigned long) ctx;
+
+ /* Register for user BPF programs need to be reset first. */
+ regs[BPF_REG_A] = 0;
+ regs[BPF_REG_X] = 0;
select_insn:
goto *jumptable[insn->code];
@@ -375,8 +385,7 @@ select_insn:
/* Function call scratches R1-R5 registers, preserves R6-R9,
* and stores return value into R0.
*/
- R0 = (__bpf_call_base + insn->imm)(regs[1], regs[2], regs[3],
- regs[4], regs[5]);
+ R0 = (__bpf_call_base + insn->imm)(R1, R2, R3, R4, R5);
CONT;
/* JMP */
@@ -500,7 +509,7 @@ select_insn:
load_word:
/* BPF_LD + BPD_ABS and BPF_LD + BPF_IND insns are only
* appearing in the programs where ctx == skb. All programs
- * keep 'ctx' in regs[CTX_REG] == R6, sk_convert_filter()
+ * keep 'ctx' in regs[BPF_REG_CTX] == R6, sk_convert_filter()
* saves it in R6, internal BPF verifier will check that
* R6 == ctx.
*
@@ -556,13 +565,6 @@ load_byte:
/* If we ever reach this, we have a bug somewhere. */
WARN_RATELIMIT(1, "unknown opcode %02x\n", insn->code);
return 0;
-#undef CONT_JMP
-#undef CONT
-
-#undef R0
-#undef X
-#undef A
-#undef K
}
u32 sk_run_filter_int_seccomp(const struct seccomp_data *ctx,
@@ -594,14 +596,14 @@ static unsigned int pkt_type_offset(void)
return -1;
}
-static u64 __skb_get_pay_offset(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
+static u64 __skb_get_pay_offset(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
struct sk_buff *skb = (struct sk_buff *)(long) ctx;
return __skb_get_poff(skb);
}
-static u64 __skb_get_nlattr(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
+static u64 __skb_get_nlattr(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
struct sk_buff *skb = (struct sk_buff *)(long) ctx;
struct nlattr *nla;
@@ -612,17 +614,17 @@ static u64 __skb_get_nlattr(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
if (skb->len < sizeof(struct nlattr))
return 0;
- if (A > skb->len - sizeof(struct nlattr))
+ if (a > skb->len - sizeof(struct nlattr))
return 0;
- nla = nla_find((struct nlattr *) &skb->data[A], skb->len - A, X);
+ nla = nla_find((struct nlattr *) &skb->data[a], skb->len - a, x);
if (nla)
return (void *) nla - (void *) skb->data;
return 0;
}
-static u64 __skb_get_nlattr_nest(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
+static u64 __skb_get_nlattr_nest(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
struct sk_buff *skb = (struct sk_buff *)(long) ctx;
struct nlattr *nla;
@@ -633,27 +635,27 @@ static u64 __skb_get_nlattr_nest(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
if (skb->len < sizeof(struct nlattr))
return 0;
- if (A > skb->len - sizeof(struct nlattr))
+ if (a > skb->len - sizeof(struct nlattr))
return 0;
- nla = (struct nlattr *) &skb->data[A];
- if (nla->nla_len > skb->len - A)
+ nla = (struct nlattr *) &skb->data[a];
+ if (nla->nla_len > skb->len - a)
return 0;
- nla = nla_find_nested(nla, X);
+ nla = nla_find_nested(nla, x);
if (nla)
return (void *) nla - (void *) skb->data;
return 0;
}
-static u64 __get_raw_cpu_id(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
+static u64 __get_raw_cpu_id(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
return raw_smp_processor_id();
}
/* note that this only generates 32-bit random numbers */
-static u64 __get_random_u32(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
+static u64 __get_random_u32(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
return (u64)prandom_u32();
}
@@ -668,28 +670,28 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, protocol) != 2);
insn->code = BPF_LDX | BPF_MEM | BPF_H;
- insn->a_reg = A_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_CTX;
insn->off = offsetof(struct sk_buff, protocol);
insn++;
/* A = ntohs(A) [emitting a nop or swap16] */
insn->code = BPF_ALU | BPF_END | BPF_FROM_BE;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = 16;
break;
case SKF_AD_OFF + SKF_AD_PKTTYPE:
insn->code = BPF_LDX | BPF_MEM | BPF_B;
- insn->a_reg = A_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_CTX;
insn->off = pkt_type_offset();
if (insn->off < 0)
return false;
insn++;
insn->code = BPF_ALU | BPF_AND | BPF_K;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = PKT_TYPE_MAX;
break;
@@ -699,13 +701,13 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
insn->code = BPF_LDX | BPF_MEM | BPF_DW;
else
insn->code = BPF_LDX | BPF_MEM | BPF_W;
- insn->a_reg = TMP_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_TMP;
+ insn->x_reg = BPF_REG_CTX;
insn->off = offsetof(struct sk_buff, dev);
insn++;
insn->code = BPF_JMP | BPF_JNE | BPF_K;
- insn->a_reg = TMP_REG;
+ insn->a_reg = BPF_REG_TMP;
insn->imm = 0;
insn->off = 1;
insn++;
@@ -716,8 +718,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
BUILD_BUG_ON(FIELD_SIZEOF(struct net_device, ifindex) != 4);
BUILD_BUG_ON(FIELD_SIZEOF(struct net_device, type) != 2);
- insn->a_reg = A_REG;
- insn->x_reg = TMP_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_TMP;
if (fp->k == SKF_AD_OFF + SKF_AD_IFINDEX) {
insn->code = BPF_LDX | BPF_MEM | BPF_W;
@@ -732,8 +734,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
insn->code = BPF_LDX | BPF_MEM | BPF_W;
- insn->a_reg = A_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_CTX;
insn->off = offsetof(struct sk_buff, mark);
break;
@@ -741,8 +743,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, hash) != 4);
insn->code = BPF_LDX | BPF_MEM | BPF_W;
- insn->a_reg = A_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_CTX;
insn->off = offsetof(struct sk_buff, hash);
break;
@@ -750,8 +752,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, queue_mapping) != 2);
insn->code = BPF_LDX | BPF_MEM | BPF_H;
- insn->a_reg = A_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_CTX;
insn->off = offsetof(struct sk_buff, queue_mapping);
break;
@@ -760,8 +762,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, vlan_tci) != 2);
insn->code = BPF_LDX | BPF_MEM | BPF_H;
- insn->a_reg = A_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_CTX;
insn->off = offsetof(struct sk_buff, vlan_tci);
insn++;
@@ -769,16 +771,16 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
if (fp->k == SKF_AD_OFF + SKF_AD_VLAN_TAG) {
insn->code = BPF_ALU | BPF_AND | BPF_K;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = ~VLAN_TAG_PRESENT;
} else {
insn->code = BPF_ALU | BPF_RSH | BPF_K;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = 12;
insn++;
insn->code = BPF_ALU | BPF_AND | BPF_K;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = 1;
}
break;
@@ -790,20 +792,20 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
case SKF_AD_OFF + SKF_AD_RANDOM:
/* arg1 = ctx */
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = ARG1_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_ARG1;
+ insn->x_reg = BPF_REG_CTX;
insn++;
/* arg2 = A */
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = ARG2_REG;
- insn->x_reg = A_REG;
+ insn->a_reg = BPF_REG_ARG2;
+ insn->x_reg = BPF_REG_A;
insn++;
/* arg3 = X */
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = ARG3_REG;
- insn->x_reg = X_REG;
+ insn->a_reg = BPF_REG_ARG3;
+ insn->x_reg = BPF_REG_X;
insn++;
/* Emit call(ctx, arg2=A, arg3=X) */
@@ -829,8 +831,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
case SKF_AD_OFF + SKF_AD_ALU_XOR_X:
insn->code = BPF_ALU | BPF_XOR | BPF_X;
- insn->a_reg = A_REG;
- insn->x_reg = X_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_X;
break;
default:
@@ -880,7 +882,7 @@ int sk_convert_filter(struct sock_filter *prog, int len,
u8 bpf_src;
BUILD_BUG_ON(BPF_MEMWORDS * sizeof(u32) > MAX_BPF_STACK);
- BUILD_BUG_ON(FP_REG + 1 != MAX_BPF_REG);
+ BUILD_BUG_ON(BPF_REG_FP + 1 != MAX_BPF_REG);
if (len <= 0 || len >= BPF_MAXINSNS)
return -EINVAL;
@@ -897,8 +899,8 @@ do_pass:
if (new_insn) {
new_insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- new_insn->a_reg = CTX_REG;
- new_insn->x_reg = ARG1_REG;
+ new_insn->a_reg = BPF_REG_CTX;
+ new_insn->x_reg = BPF_REG_ARG1;
}
new_insn++;
@@ -948,8 +950,8 @@ do_pass:
break;
insn->code = fp->code;
- insn->a_reg = A_REG;
- insn->x_reg = X_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_X;
insn->imm = fp->k;
break;
@@ -983,16 +985,16 @@ do_pass:
* in compare insn.
*/
insn->code = BPF_ALU | BPF_MOV | BPF_K;
- insn->a_reg = TMP_REG;
+ insn->a_reg = BPF_REG_TMP;
insn->imm = fp->k;
insn++;
- insn->a_reg = A_REG;
- insn->x_reg = TMP_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_TMP;
bpf_src = BPF_X;
} else {
- insn->a_reg = A_REG;
- insn->x_reg = X_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_X;
insn->imm = fp->k;
bpf_src = BPF_SRC(fp->code);
}
@@ -1027,33 +1029,33 @@ do_pass:
/* ldxb 4 * ([14] & 0xf) is remaped into 6 insns. */
case BPF_LDX | BPF_MSH | BPF_B:
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = TMP_REG;
- insn->x_reg = A_REG;
+ insn->a_reg = BPF_REG_TMP;
+ insn->x_reg = BPF_REG_A;
insn++;
insn->code = BPF_LD | BPF_ABS | BPF_B;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = fp->k;
insn++;
insn->code = BPF_ALU | BPF_AND | BPF_K;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = 0xf;
insn++;
insn->code = BPF_ALU | BPF_LSH | BPF_K;
- insn->a_reg = A_REG;
+ insn->a_reg = BPF_REG_A;
insn->imm = 2;
insn++;
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = X_REG;
- insn->x_reg = A_REG;
+ insn->a_reg = BPF_REG_X;
+ insn->x_reg = BPF_REG_A;
insn++;
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = A_REG;
- insn->x_reg = TMP_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_TMP;
break;
/* RET_K, RET_A are remaped into 2 insns. */
@@ -1063,7 +1065,7 @@ do_pass:
(BPF_RVAL(fp->code) == BPF_K ?
BPF_K : BPF_X);
insn->a_reg = 0;
- insn->x_reg = A_REG;
+ insn->x_reg = BPF_REG_A;
insn->imm = fp->k;
insn++;
@@ -1074,8 +1076,9 @@ do_pass:
case BPF_ST:
case BPF_STX:
insn->code = BPF_STX | BPF_MEM | BPF_W;
- insn->a_reg = FP_REG;
- insn->x_reg = fp->code == BPF_ST ? A_REG : X_REG;
+ insn->a_reg = BPF_REG_FP;
+ insn->x_reg = fp->code == BPF_ST ?
+ BPF_REG_A : BPF_REG_X;
insn->off = -(BPF_MEMWORDS - fp->k) * 4;
break;
@@ -1084,8 +1087,8 @@ do_pass:
case BPF_LDX | BPF_MEM:
insn->code = BPF_LDX | BPF_MEM | BPF_W;
insn->a_reg = BPF_CLASS(fp->code) == BPF_LD ?
- A_REG : X_REG;
- insn->x_reg = FP_REG;
+ BPF_REG_A : BPF_REG_X;
+ insn->x_reg = BPF_REG_FP;
insn->off = -(BPF_MEMWORDS - fp->k) * 4;
break;
@@ -1094,22 +1097,22 @@ do_pass:
case BPF_LDX | BPF_IMM:
insn->code = BPF_ALU | BPF_MOV | BPF_K;
insn->a_reg = BPF_CLASS(fp->code) == BPF_LD ?
- A_REG : X_REG;
+ BPF_REG_A : BPF_REG_X;
insn->imm = fp->k;
break;
/* X = A */
case BPF_MISC | BPF_TAX:
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = X_REG;
- insn->x_reg = A_REG;
+ insn->a_reg = BPF_REG_X;
+ insn->x_reg = BPF_REG_A;
break;
/* A = X */
case BPF_MISC | BPF_TXA:
insn->code = BPF_ALU64 | BPF_MOV | BPF_X;
- insn->a_reg = A_REG;
- insn->x_reg = X_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_X;
break;
/* A = skb->len or X = skb->len */
@@ -1117,16 +1120,16 @@ do_pass:
case BPF_LDX | BPF_W | BPF_LEN:
insn->code = BPF_LDX | BPF_MEM | BPF_W;
insn->a_reg = BPF_CLASS(fp->code) == BPF_LD ?
- A_REG : X_REG;
- insn->x_reg = CTX_REG;
+ BPF_REG_A : BPF_REG_X;
+ insn->x_reg = BPF_REG_CTX;
insn->off = offsetof(struct sk_buff, len);
break;
/* access seccomp_data fields */
case BPF_LDX | BPF_ABS | BPF_W:
insn->code = BPF_LDX | BPF_MEM | BPF_W;
- insn->a_reg = A_REG;
- insn->x_reg = CTX_REG;
+ insn->a_reg = BPF_REG_A;
+ insn->x_reg = BPF_REG_CTX;
insn->off = fp->k;
break;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next v3 3/3] net: filter: misc/various cleanups
2014-05-01 16:16 [PATCH net-next v3 0/3] BPF cleanups Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 2/3] net: filter: make register namings more comprehensible Daniel Borkmann
@ 2014-05-01 16:16 ` Daniel Borkmann
2 siblings, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2014-05-01 16:16 UTC (permalink / raw)
To: davem; +Cc: ast, netdev
This contains only some minor misc cleanpus. We can spare us the
extra variable declaration in __skb_get_pay_offset(), the cast in
__get_random_u32() is rather unnecessary and in __sk_migrate_realloc()
we can remove the memcpy() and do a direct assignment of the structs.
Latter was suggested by Fengguang Wu found with coccinelle. Also,
remaining pointer casts of long should be unsigned long instead.
Suggested-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
net/core/filter.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index c93ca8d..36cfceb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -78,9 +78,9 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns
ptr = skb_network_header(skb) + k - SKF_NET_OFF;
else if (k >= SKF_LL_OFF)
ptr = skb_mac_header(skb) + k - SKF_LL_OFF;
-
if (ptr >= skb->head && ptr + size <= skb_tail_pointer(skb))
return ptr;
+
return NULL;
}
@@ -89,6 +89,7 @@ static inline void *load_pointer(const struct sk_buff *skb, int k,
{
if (k >= 0)
return skb_header_pointer(skb, k, size, buffer);
+
return bpf_internal_load_pointer_neg_helper(skb, k, size);
}
@@ -598,14 +599,12 @@ static unsigned int pkt_type_offset(void)
static u64 __skb_get_pay_offset(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
- struct sk_buff *skb = (struct sk_buff *)(long) ctx;
-
- return __skb_get_poff(skb);
+ return __skb_get_poff((struct sk_buff *)(long unsigned) ctx);
}
static u64 __skb_get_nlattr(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
- struct sk_buff *skb = (struct sk_buff *)(long) ctx;
+ struct sk_buff *skb = (struct sk_buff *)(unsigned long) ctx;
struct nlattr *nla;
if (skb_is_nonlinear(skb))
@@ -626,7 +625,7 @@ static u64 __skb_get_nlattr(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
static u64 __skb_get_nlattr_nest(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
- struct sk_buff *skb = (struct sk_buff *)(long) ctx;
+ struct sk_buff *skb = (struct sk_buff *)(unsigned long) ctx;
struct nlattr *nla;
if (skb_is_nonlinear(skb))
@@ -657,7 +656,7 @@ static u64 __get_raw_cpu_id(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
/* note that this only generates 32-bit random numbers */
static u64 __get_random_u32(u64 ctx, u64 a, u64 x, u64 r4, u64 r5)
{
- return (u64)prandom_u32();
+ return prandom_u32();
}
static bool convert_bpf_extensions(struct sock_filter *fp,
@@ -1475,7 +1474,7 @@ static struct sk_filter *__sk_migrate_realloc(struct sk_filter *fp,
fp_new = sock_kmalloc(sk, len, GFP_KERNEL);
if (fp_new) {
- memcpy(fp_new, fp, sizeof(struct sk_filter));
+ *fp_new = *fp;
/* As we're kepping orig_prog in fp_new along,
* we need to make sure we're not evicting it
* from the old fp.
--
1.7.11.7
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table
2014-05-01 16:16 ` [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table Daniel Borkmann
@ 2014-05-01 17:20 ` Eric Dumazet
2014-05-01 17:39 ` Daniel Borkmann
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2014-05-01 17:20 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, ast, netdev
On Thu, 2014-05-01 at 18:16 +0200, Daniel Borkmann wrote:
> This patch simplifies label naming for the BPF jump-table.
> When we define labels via DL(), we just concatenate/textify
> the combination of instruction opcode which consists of the
> class, subclass, word size, target register and so on. Each
> time we leave BPF_ prefix intact, so that e.g. the preprocessor
> generates a label BPF_ALU_BPF_ADD_BPF_X for DL(BPF_ALU, BPF_ADD,
> BPF_X) whereas a label name of ALU_ADD_X is much more easy
> to grasp. Pure cleanup only.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
This exactly makes harder to use grep and future code reviews.
# git grep -n BPF_XADD
include/linux/filter.h:21:#define BPF_XADD 0xc0 /* exclusive add */
net/core/filter.c:224: DL(BPF_STX, BPF_XADD, BPF_W),
net/core/filter.c:225: DL(BPF_STX, BPF_XADD, BPF_DW),
net/core/filter.c:481: BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
net/core/filter.c:485: BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
Compare now with :
# git grep -n XADD
So I am quite against this patch.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table
2014-05-01 17:20 ` Eric Dumazet
@ 2014-05-01 17:39 ` Daniel Borkmann
2014-05-01 18:50 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Borkmann @ 2014-05-01 17:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, ast, netdev
On 05/01/2014 07:20 PM, Eric Dumazet wrote:
> On Thu, 2014-05-01 at 18:16 +0200, Daniel Borkmann wrote:
>> This patch simplifies label naming for the BPF jump-table.
>> When we define labels via DL(), we just concatenate/textify
>> the combination of instruction opcode which consists of the
>> class, subclass, word size, target register and so on. Each
>> time we leave BPF_ prefix intact, so that e.g. the preprocessor
>> generates a label BPF_ALU_BPF_ADD_BPF_X for DL(BPF_ALU, BPF_ADD,
>> BPF_X) whereas a label name of ALU_ADD_X is much more easy
>> to grasp. Pure cleanup only.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>> ---
>
> This exactly makes harder to use grep and future code reviews.
>
> # git grep -n BPF_XADD
> include/linux/filter.h:21:#define BPF_XADD 0xc0 /* exclusive add */
> net/core/filter.c:224: DL(BPF_STX, BPF_XADD, BPF_W),
> net/core/filter.c:225: DL(BPF_STX, BPF_XADD, BPF_DW),
> net/core/filter.c:481: BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
> net/core/filter.c:485: BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
>
> Compare now with :
>
> # git grep -n XADD
>
> So I am quite against this patch.
Well, if you review the code itself in filter.c it makes it hard
to read and review, with this patch, you'll immediately get the
label name and what it actually does, so I think it's quite convenient
and way more readable by itself. For grepping, you can always add
subdirectories you're searching for, besides that I don't think
that everything in the kernel needs to have unique names only so
that one can grep for it among the whole tree.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table
2014-05-01 17:39 ` Daniel Borkmann
@ 2014-05-01 18:50 ` Eric Dumazet
2014-05-01 19:47 ` Alexei Starovoitov
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2014-05-01 18:50 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, ast, netdev
On Thu, 2014-05-01 at 19:39 +0200, Daniel Borkmann wrote:
> Well, if you review the code itself in filter.c it makes it hard
> to read and review, with this patch, you'll immediately get the
> label name and what it actually does, so I think it's quite convenient
> and way more readable by itself. For grepping, you can always add
> subdirectories you're searching for, besides that I don't think
> that everything in the kernel needs to have unique names only so
> that one can grep for it among the whole tree.
Considering that Alexei claimed :
+Internal BPF is a general purpose RISC instruction set. Not every register and
+every instruction are used during translation from original BPF to new format.
+For example, socket filters are not using 'exclusive add' instruction, but
+tracing filters may do to maintain counters of events, for example. Register R9
+is not used by socket filters either, but more complex filters may be running
+out of registers and would have to resort to spill/fill to stack.
I tried to verify this claim and I could not.
After your patch, it will be really this :
"We, Daniel and Alexei, hereby certify that this code is 100% safe.
If you Eric, or anyone else, want to check, you'll have to grep in
appropriate places, and use your brain as we did."
I find very worrying all this stuff. This looks like you guys make
everything possible to make it absolutely unmaintainable.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table
2014-05-01 18:50 ` Eric Dumazet
@ 2014-05-01 19:47 ` Alexei Starovoitov
0 siblings, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2014-05-01 19:47 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Daniel Borkmann, David S. Miller, Network Development
On Thu, May 1, 2014 at 11:50 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-05-01 at 19:39 +0200, Daniel Borkmann wrote:
>
>> Well, if you review the code itself in filter.c it makes it hard
>> to read and review, with this patch, you'll immediately get the
>> label name and what it actually does, so I think it's quite convenient
>> and way more readable by itself. For grepping, you can always add
>> subdirectories you're searching for, besides that I don't think
>> that everything in the kernel needs to have unique names only so
>> that one can grep for it among the whole tree.
>
>
>
> Considering that Alexei claimed :
>
> +Internal BPF is a general purpose RISC instruction set. Not every register and
> +every instruction are used during translation from original BPF to new format.
> +For example, socket filters are not using 'exclusive add' instruction, but
> +tracing filters may do to maintain counters of events, for example. Register R9
> +is not used by socket filters either, but more complex filters may be running
> +out of registers and would have to resort to spill/fill to stack.
>
> I tried to verify this claim and I could not.
I'm not sure what part are you referring to.
That xadd is not used right now? It's not. I think we agreed on it
last week.
What else do you mean?
> After your patch, it will be really this :
>
> "We, Daniel and Alexei, hereby certify that this code is 100% safe.
I don't see where we say this.
Few paragraphs below the doc clearly says that it's a RISC instruction set
and safety on instruction set comes from verifier which is TBD.
Instruction set by itself is just an instruction set. Just like original BPF.
> If you Eric, or anyone else, want to check, you'll have to grep in
> appropriate places, and use your brain as we did."
>
> I find very worrying all this stuff. This looks like you guys make
> everything possible to make it absolutely unmaintainable.
imo that's a harsh and unfair statement.
Calling it 'unmaintainable' just because you don't see how xadd
can be used?
I'm working on tracing filters that will demonstrate that in a bit.
This particular Daniel's patch I think is a nice cleanup.
It indeed makes interpreter code shorter, less verbose and easier to read.
I suspect you think of this internal BPF format as it's solving only
socket filters use case and all filters will be coming from user space.
That's very narrow view. In case of tracing filters there is no BPF
to send to kernel. User to kernel interface is a character string
like "a==1 || b == 2". That is the filter. I'm parsing the string
and converting it inside the kernel to internal bpf format,
so I would like to use full power of underlying cpu.
Think of internal BPF as 'uasm' that mips guys are using to construct tlb
handler on the fly.
But this 'uasm' is generic for all architectures.
Eventually we may want to expose internal bpf to user space, only then
verifier will be must have and we'll be discussing '100% safety'.
Right now everything is internal and I'm pretty sure you understood
very well how original bpf was translated to internal bpf, so I don't follow
your concern about 'unmaintainable'.
At the same time it's impossible for me to follow what you guys are
doing with tcp stack, but I'm not calling it unmaintainable just because
I don't understand it.
If you feel more documentation is needed, sure. Please let us know
what pieces are not explained well.
When I look at the doc by myself, it's hard to see what else is missing
or what pieces are difficult to grasp.
Regards,
Alexei
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-05-01 19:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-01 16:16 [PATCH net-next v3 0/3] BPF cleanups Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table Daniel Borkmann
2014-05-01 17:20 ` Eric Dumazet
2014-05-01 17:39 ` Daniel Borkmann
2014-05-01 18:50 ` Eric Dumazet
2014-05-01 19:47 ` Alexei Starovoitov
2014-05-01 16:16 ` [PATCH net-next v3 2/3] net: filter: make register namings more comprehensible Daniel Borkmann
2014-05-01 16:16 ` [PATCH net-next v3 3/3] net: filter: misc/various cleanups Daniel Borkmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).