[PATCH 00/17] Misc MIPS/BPF fixes for 3.16

[PATCH 00/17] Misc MIPS/BPF fixes for 3.16

[Markos Chandras]

Hi,

Here are some fixes for MIPS/BPF for 3.16. These fixes make
the bpf testsuite *almost* happy with only 2 tests (LD_IND_LL,
LD_IND_NET) failing at the moment. Since fixing the remaining tests
is not so trivial, it would be nice to have these fixes in 3.16 for now.

The patches are based on the upstream-sfr/mips-for-linux-next tree
because they depend on https://patchwork.linux-mips.org/patch/7099/

Markos Chandras (17):
  MIPS: uasm: Add s3s1s2 instruction builder
  MIPS: uasm: Add slt uasm instruction
  MIPS: mm: uasm: Fix lh micro-assembler instruction
  MIPS: bpf: Use the LO register to get division's quotient
  MIPS: bpf: Return error code if the offset is a negative number
  MIPS: bpf: Use 'andi' instead of 'and' for the VLAN cases
  MIPS: bpf: Add SEEN_SKB to flags when looking for the PKT_TYPE
  MIPS: bpf: Fix branch conditional for BPF_J{GT/GE} cases
  MIPS: bpf: Use correct mask for VLAN_TAG case
  MIPS: bpf: Fix return values for VLAN_TAG_PRESENT case
  MIPS: bpf: Use pr_debug instead of pr_warn for unhandled opcodes
  MIPS: bpf: Fix is_range() semantics
  MIPS: bpf: Drop update_on_xread and always initialize the X register
  MIPS: bpf: Prevent kernel fall over for >=32bit shifts
  MIPS: bpf: Fix PKT_TYPE case for big-endian cores
  MIPS: bpf: Use 32 or 64-bit load instruction to load an address to
    register
  MIPS: bpf: Fix stack space allocation for BPF memwords on MIPS64

 arch/mips/include/asm/uasm.h      |   4 ++
 arch/mips/include/uapi/asm/inst.h |   1 +
 arch/mips/mm/uasm-micromips.c     |   1 +
 arch/mips/mm/uasm-mips.c          |   3 +-
 arch/mips/mm/uasm.c               |  10 +++-
 arch/mips/net/bpf_jit.c           | 115 ++++++++++++++++++++++++--------------
 6 files changed, 90 insertions(+), 44 deletions(-)

-- 
2.0.0

[PATCH 04/17] MIPS: bpf: Use the LO register to get division's quotient

[Markos Chandras]

Reading from the HI register to get the division result is wrong.
The quotient is placed in the LO register.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index f7c206404989..5cc92c4590cb 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -408,7 +408,7 @@ static inline void emit_div(unsigned int dst, unsigned int src,
 		u32 *p = &ctx->target[ctx->idx];
 		uasm_i_divu(&p, dst, src);
 		p = &ctx->target[ctx->idx + 1];
-		uasm_i_mfhi(&p, dst);
+		uasm_i_mflo(&p, dst);
 	}
 	ctx->idx += 2; /* 2 insts */
 }
-- 
2.0.0

[PATCH 05/17] MIPS: bpf: Return error code if the offset is a negative number

[Markos Chandras]

Previously, the negative offset was not checked leading to failures
due to trying to load data beyond the skb struct boundaries. Until we
have proper asm helpers in place, it's best if we return ENOSUPP if K
is negative when trying to JIT the filter or 0 during runtime if we
do an indirect load where the value of X is unknown during build time.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 5cc92c4590cb..95728ea6cb74 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -331,6 +331,12 @@ static inline void emit_srl(unsigned int dst, unsigned int src,
 	emit_instr(ctx, srl, dst, src, sa);
 }
 
+static inline void emit_slt(unsigned int dst, unsigned int src1,
+			    unsigned int src2, struct jit_ctx *ctx)
+{
+	emit_instr(ctx, slt, dst, src1, src2);
+}
+
 static inline void emit_sltu(unsigned int dst, unsigned int src1,
 			     unsigned int src2, struct jit_ctx *ctx)
 {
@@ -816,8 +822,21 @@ static int build_body(struct jit_ctx *ctx)
 			/* A <- P[k:1] */
 			load_order = 0;
 load:
+			/* the interpreter will deal with the negative K */
+			if ((int)k < 0)
+				return -ENOTSUPP;
+
 			emit_load_imm(r_off, k, ctx);
 load_common:
+			/*
+			 * We may got here from the indirect loads so
+			 * return if offset is negative.
+			 */
+			emit_slt(r_s0, r_off, r_zero, ctx);
+			emit_bcond(MIPS_COND_NE, r_s0, r_zero,
+				   b_imm(prog->len, ctx), ctx);
+			emit_reg_move(r_ret, r_zero, ctx);
+
 			ctx->flags |= SEEN_CALL | SEEN_OFF | SEEN_S0 |
 				SEEN_SKB | SEEN_A;
 
@@ -880,6 +899,10 @@ load_ind:
 			emit_load(r_X, r_skb, off, ctx);
 			break;
 		case BPF_LDX | BPF_B | BPF_MSH:
+			/* the interpreter will deal with the negative K */
+			if ((int)k < 0)
+				return -ENOTSUPP;
+
 			/* X <- 4 * (P[k:1] & 0xf) */
 			ctx->flags |= SEEN_X | SEEN_CALL | SEEN_S0 | SEEN_SKB;
 			/* Load offset to a1 */
-- 
2.0.0

[PATCH 06/17] MIPS: bpf: Use 'andi' instead of 'and' for the VLAN cases

[Markos Chandras]

The VLAN_VID_MASK and VLAN_TAG_PRESENT are immediates, so using
'and' which expects 3 registers will produce wrong results. Fix
this by using the 'andi' instruction.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 95728ea6cb74..fe5041bdc6fb 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -1317,9 +1317,9 @@ jmp_cmp:
 			off = offsetof(struct sk_buff, vlan_tci);
 			emit_half_load(r_s0, r_skb, off, ctx);
 			if (code == (BPF_ANC | SKF_AD_VLAN_TAG))
-				emit_and(r_A, r_s0, VLAN_VID_MASK, ctx);
+				emit_andi(r_A, r_s0, VLAN_VID_MASK, ctx);
 			else
-				emit_and(r_A, r_s0, VLAN_TAG_PRESENT, ctx);
+				emit_andi(r_A, r_s0, VLAN_TAG_PRESENT, ctx);
 			break;
 		case BPF_ANC | SKF_AD_PKTTYPE:
 			off = pkt_type_offset();
-- 
2.0.0

[PATCH 07/17] MIPS: bpf: Add SEEN_SKB to flags when looking for the PKT_TYPE

[Markos Chandras]

The SKF_AD_PKTTYPE uses the skb pointer so make sure it's in the
flags so it will be initialized in time.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index fe5041bdc6fb..8cae27af03da 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -1322,6 +1322,8 @@ jmp_cmp:
 				emit_andi(r_A, r_s0, VLAN_TAG_PRESENT, ctx);
 			break;
 		case BPF_ANC | SKF_AD_PKTTYPE:
+			ctx->flags |= SEEN_SKB;
+
 			off = pkt_type_offset();
 
 			if (off < 0)
-- 
2.0.0

[PATCH 08/17] MIPS: bpf: Fix branch conditional for BPF_J{GT/GE} cases

[Markos Chandras]

The sltiu and sltu instructions will set the scratch register
to 1 if A <= X|K so fix the emitted branch conditional to check
for scratch != zero rather than scratch >= zero which would complicate
the resuling branch logic given that MIPS does not have a BGT or BGET
instructions to compare general purpose registers directly.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 8cae27af03da..500f97fdc0e1 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -1127,7 +1127,7 @@ jmp_cmp:
 				}
 				/* A < (K|X) ? r_scrach = 1 */
 				b_off = b_imm(i + inst->jf + 1, ctx);
-				emit_bcond(MIPS_COND_GT, r_s0, r_zero, b_off,
+				emit_bcond(MIPS_COND_NE, r_s0, r_zero, b_off,
 					   ctx);
 				emit_nop(ctx);
 				/* A > (K|X) ? scratch = 0 */
-- 
2.0.0

[PATCH 09/17] MIPS: bpf: Use correct mask for VLAN_TAG case

[Markos Chandras]

Using VLAN_VID_MASK is not correct to get the vlan tag. Use
~VLAN_PRESENT_MASK instead and make sure it's u16 so the top 16-bits
will be removed. This will ensure that the emit_andi() code will not
treat this as a big 32-bit unsigned value.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 500f97fdc0e1..a4d1b76e7373 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -1317,7 +1317,7 @@ jmp_cmp:
 			off = offsetof(struct sk_buff, vlan_tci);
 			emit_half_load(r_s0, r_skb, off, ctx);
 			if (code == (BPF_ANC | SKF_AD_VLAN_TAG))
-				emit_andi(r_A, r_s0, VLAN_VID_MASK, ctx);
+				emit_andi(r_A, r_s0, (u16)~VLAN_TAG_PRESENT, ctx);
 			else
 				emit_andi(r_A, r_s0, VLAN_TAG_PRESENT, ctx);
 			break;
-- 
2.0.0

[PATCH 10/17] MIPS: bpf: Fix return values for VLAN_TAG_PRESENT case

[Markos Chandras]

If VLAN_TAG_PRESENT is not zero, then return 1 as expected by
classic BPF. Otherwise return 0.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index a4d1b76e7373..d852bb6d3fe3 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -1316,10 +1316,13 @@ jmp_cmp:
 						  vlan_tci) != 2);
 			off = offsetof(struct sk_buff, vlan_tci);
 			emit_half_load(r_s0, r_skb, off, ctx);
-			if (code == (BPF_ANC | SKF_AD_VLAN_TAG))
+			if (code == (BPF_ANC | SKF_AD_VLAN_TAG)) {
 				emit_andi(r_A, r_s0, (u16)~VLAN_TAG_PRESENT, ctx);
-			else
+			} else {
 				emit_andi(r_A, r_s0, VLAN_TAG_PRESENT, ctx);
+				/* return 1 if present */
+				emit_sltu(r_A, r_zero, r_A, ctx);
+			}
 			break;
 		case BPF_ANC | SKF_AD_PKTTYPE:
 			ctx->flags |= SEEN_SKB;
-- 
2.0.0

[PATCH 11/17] MIPS: bpf: Use pr_debug instead of pr_warn for unhandled opcodes

[Markos Chandras]

We should prevent spamming the logs during normal execution of bpf-jit.

Suggested-by: Alexei Starovoitov <ast@plumgrid.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index d852bb6d3fe3..1d228d27d759 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -1345,8 +1345,8 @@ jmp_cmp:
 			emit_half_load(r_A, r_skb, off, ctx);
 			break;
 		default:
-			pr_warn("%s: Unhandled opcode: 0x%02x\n", __FILE__,
-				inst->code);
+			pr_debug("%s: Unhandled opcode: 0x%02x\n", __FILE__,
+				 inst->code);
 			return -1;
 		}
 	}
-- 
2.0.0

[PATCH 12/17] MIPS: bpf: Fix is_range() semantics

[Markos Chandras]

is_range() was meant to check whether the number is within
the s16 range or not. However the return values and consumers expected
the exact opposite. We fix that by inverting the logic in the function
to return 'true' for < s16 and 'false' for > s16.

Reported-by: Alexei Starovoitov <ast@plumgrid.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 1d228d27d759..00c4c83972bb 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -166,9 +166,7 @@ do {							\
 /* Determine if immediate is within the 16-bit signed range */
 static inline bool is_range16(s32 imm)
 {
-	if (imm >= SBIT(15) || imm < -SBIT(15))
-		return true;
-	return false;
+	return !(imm >= SBIT(15) || imm < -SBIT(15));
 }
 
 static inline void emit_addu(unsigned int dst, unsigned int src1,
@@ -187,7 +185,7 @@ static inline void emit_load_imm(unsigned int dst, u32 imm, struct jit_ctx *ctx)
 {
 	if (ctx->target != NULL) {
 		/* addiu can only handle s16 */
-		if (is_range16(imm)) {
+		if (!is_range16(imm)) {
 			u32 *p = &ctx->target[ctx->idx];
 			uasm_i_lui(&p, r_tmp_imm, (s32)imm >> 16);
 			p = &ctx->target[ctx->idx + 1];
@@ -199,7 +197,7 @@ static inline void emit_load_imm(unsigned int dst, u32 imm, struct jit_ctx *ctx)
 	}
 	ctx->idx++;
 
-	if (is_range16(imm))
+	if (!is_range16(imm))
 		ctx->idx++;
 }
 
@@ -240,7 +238,7 @@ static inline void emit_daddiu(unsigned int dst, unsigned int src,
 static inline void emit_addiu(unsigned int dst, unsigned int src,
 			      u32 imm, struct jit_ctx *ctx)
 {
-	if (is_range16(imm)) {
+	if (!is_range16(imm)) {
 		emit_load_imm(r_tmp, imm, ctx);
 		emit_addu(dst, r_tmp, src, ctx);
 	} else {
@@ -347,7 +345,7 @@ static inline void emit_sltiu(unsigned dst, unsigned int src,
 			      unsigned int imm, struct jit_ctx *ctx)
 {
 	/* 16 bit immediate */
-	if (is_range16((s32)imm)) {
+	if (!is_range16((s32)imm)) {
 		emit_load_imm(r_tmp, imm, ctx);
 		emit_sltu(dst, src, r_tmp, ctx);
 	} else {
-- 
2.0.0

[PATCH 13/17] MIPS: bpf: Drop update_on_xread and always initialize the X register

[Markos Chandras]

Previously, update_on_xread() only set the reset flag if SEEN_X hasn't
been set already. However, SEEN_X is used to indicate that X is used
as destination or source register so there are some cases where X
is only used as source register and we really need to make sure that it
has been initialized in time. As a result of which, drop this function and
always set X to zero if it's used in any of the opcodes.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 00c4c83972bb..1bcd599d9971 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -119,8 +119,6 @@
 /* Arguments used by JIT */
 #define ARGS_USED_BY_JIT	2 /* only applicable to 64-bit */
 
-#define FLAG_NEED_X_RESET	(1 << 0)
-
 #define SBIT(x)			(1 << (x)) /* Signed version of BIT() */
 
 /**
@@ -549,14 +547,6 @@ static inline u16 align_sp(unsigned int num)
 	return num;
 }
 
-static inline void update_on_xread(struct jit_ctx *ctx)
-{
-	if (!(ctx->flags & SEEN_X))
-		ctx->flags |= FLAG_NEED_X_RESET;
-
-	ctx->flags |= SEEN_X;
-}
-
 static bool is_load_to_a(u16 inst)
 {
 	switch (inst) {
@@ -701,7 +691,7 @@ static void build_prologue(struct jit_ctx *ctx)
 	if (ctx->flags & SEEN_SKB)
 		emit_reg_move(r_skb, MIPS_R_A0, ctx);
 
-	if (ctx->flags & FLAG_NEED_X_RESET)
+	if (ctx->flags & SEEN_X)
 		emit_jit_reg_move(r_X, r_zero, ctx);
 
 	/* Do not leak kernel data to userspace */
@@ -876,7 +866,6 @@ load_common:
 			/* A <- P[X + k:1] */
 			load_order = 0;
 load_ind:
-			update_on_xread(ctx);
 			ctx->flags |= SEEN_OFF | SEEN_X;
 			emit_addiu(r_off, r_X, k, ctx);
 			goto load_common;
@@ -972,7 +961,6 @@ load_ind:
 			break;
 		case BPF_ALU | BPF_MUL | BPF_X:
 			/* A *= X */
-			update_on_xread(ctx);
 			ctx->flags |= SEEN_A | SEEN_X;
 			emit_mul(r_A, r_A, r_X, ctx);
 			break;
@@ -1002,7 +990,6 @@ load_ind:
 			break;
 		case BPF_ALU | BPF_DIV | BPF_X:
 			/* A /= X */
-			update_on_xread(ctx);
 			ctx->flags |= SEEN_X | SEEN_A;
 			/* Check if r_X is zero */
 			emit_bcond(MIPS_COND_EQ, r_X, r_zero,
@@ -1012,7 +999,6 @@ load_ind:
 			break;
 		case BPF_ALU | BPF_MOD | BPF_X:
 			/* A %= X */
-			update_on_xread(ctx);
 			ctx->flags |= SEEN_X | SEEN_A;
 			/* Check if r_X is zero */
 			emit_bcond(MIPS_COND_EQ, r_X, r_zero,
@@ -1027,7 +1013,6 @@ load_ind:
 			break;
 		case BPF_ALU | BPF_OR | BPF_X:
 			/* A |= X */
-			update_on_xread(ctx);
 			ctx->flags |= SEEN_A;
 			emit_ori(r_A, r_A, r_X, ctx);
 			break;
@@ -1039,7 +1024,6 @@ load_ind:
 		case BPF_ANC | SKF_AD_ALU_XOR_X:
 		case BPF_ALU | BPF_XOR | BPF_X:
 			/* A ^= X */
-			update_on_xread(ctx);
 			ctx->flags |= SEEN_A;
 			emit_xor(r_A, r_A, r_X, ctx);
 			break;
@@ -1050,7 +1034,6 @@ load_ind:
 			break;
 		case BPF_ALU | BPF_AND | BPF_X:
 			/* A &= X */
-			update_on_xread(ctx);
 			ctx->flags |= SEEN_A | SEEN_X;
 			emit_and(r_A, r_A, r_X, ctx);
 			break;
@@ -1062,7 +1045,6 @@ load_ind:
 		case BPF_ALU | BPF_LSH | BPF_X:
 			/* A <<= X */
 			ctx->flags |= SEEN_A | SEEN_X;
-			update_on_xread(ctx);
 			emit_sllv(r_A, r_A, r_X, ctx);
 			break;
 		case BPF_ALU | BPF_RSH | BPF_K:
@@ -1072,7 +1054,6 @@ load_ind:
 			break;
 		case BPF_ALU | BPF_RSH | BPF_X:
 			ctx->flags |= SEEN_A | SEEN_X;
-			update_on_xread(ctx);
 			emit_srlv(r_A, r_A, r_X, ctx);
 			break;
 		case BPF_ALU | BPF_NEG:
@@ -1243,7 +1224,6 @@ jmp_cmp:
 		case BPF_MISC | BPF_TXA:
 			/* A = X */
 			ctx->flags |= SEEN_A | SEEN_X;
-			update_on_xread(ctx);
 			emit_jit_reg_move(r_A, r_X, ctx);
 			break;
 		/* AUX */
-- 
2.0.0

[PATCH 14/17] MIPS: bpf: Prevent kernel fall over for >=32bit shifts

[Markos Chandras]

Remove BUG_ON() if the shift immediate is >=32 to avoid
kernel crashes due to malicious user input. Since the micro-assembler
will not allow an immediate greater or equal to 32, we will use the
maximum value which is 31. This will do the correct thing on either 32-
or 64-bit cores since no 64-bit instructions are being used in JIT.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 1bcd599d9971..09ebc886c7aa 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -309,7 +309,8 @@ static inline void emit_sll(unsigned int dst, unsigned int src,
 			    unsigned int sa, struct jit_ctx *ctx)
 {
 	/* sa is 5-bits long */
-	BUG_ON(sa >= BIT(5));
+	if (sa >= BIT(5))
+		sa = BIT(5) - 1;
 	emit_instr(ctx, sll, dst, src, sa);
 }
 
@@ -323,7 +324,8 @@ static inline void emit_srl(unsigned int dst, unsigned int src,
 			    unsigned int sa, struct jit_ctx *ctx)
 {
 	/* sa is 5-bits long */
-	BUG_ON(sa >= BIT(5));
+	if (sa >= BIT(5))
+		sa =  BIT(5) - 1;
 	emit_instr(ctx, srl, dst, src, sa);
 }
 
-- 
2.0.0

[PATCH 15/17] MIPS: bpf: Fix PKT_TYPE case for big-endian cores

[Markos Chandras]

The skb->pkt_type field is defined as follows:

u8 pkt_type:3,
   fclone:2,
   ipvs_property:1,
   peeked:1,
   nf_trace:1

resulting to the following layout in big-endian systems

[pkt_type][fclone][ipvs_propery][peeked][nf_trace]
^                                                ^
|                                                |
LSB                                             MSB

As a result, the existing code did not work because it was trying to
match pkt_type == 7 whereas in reality it is 7<<5 on big-endian
systems.

This has been fixed in the interpreter in
0dcceabb0c1bf2d4c12a748df9933fad303072a7
"net: filter: fix SKF_AD_PKTTYPE extension on big-endian"

The fix is to look for 7<<5 on big-endian systems for the pkt_type
field, and shift by 5 so the packet type will be at the lower 3 bits
of the A register.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 09ebc886c7aa..4920e0fd05ee 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -745,13 +745,17 @@ static u64 jit_get_skb_w(struct sk_buff *skb, unsigned offset)
 	return (u64)err << 32 | ntohl(ret);
 }
 
-#define PKT_TYPE_MAX 7
+#ifdef __BIG_ENDIAN_BITFIELD
+#define PKT_TYPE_MAX	(7 << 5)
+#else
+#define PKT_TYPE_MAX	7
+#endif
 static int pkt_type_offset(void)
 {
 	struct sk_buff skb_probe = {
 		.pkt_type = ~0,
 	};
-	char *ct = (char *)&skb_probe;
+	u8 *ct = (u8 *)&skb_probe;
 	unsigned int off;
 
 	for (off = 0; off < sizeof(struct sk_buff); off++) {
@@ -1314,6 +1318,10 @@ jmp_cmp:
 			emit_load_byte(r_tmp, r_skb, off, ctx);
 			/* Keep only the last 3 bits */
 			emit_andi(r_A, r_tmp, PKT_TYPE_MAX, ctx);
+#ifdef __BIG_ENDIAN_BITFIELD
+			/* Get the actual packet type to the lower 3 bits */
+			emit_srl(r_A, r_A, 5, ctx);
+#endif
 			break;
 		case BPF_ANC | SKF_AD_QUEUE:
 			ctx->flags |= SEEN_SKB | SEEN_A;
-- 
2.0.0

[PATCH 16/17] MIPS: bpf: Use 32 or 64-bit load instruction to load an address to register

[Markos Chandras]

When loading a pointer to a register we need to use the appropriate
32 or 64bit instruction to preserve the pointer's top 32bits.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 4920e0fd05ee..d8dba7b523a5 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -447,6 +447,17 @@ static inline void emit_wsbh(unsigned int dst, unsigned int src,
 	emit_instr(ctx, wsbh, dst, src);
 }
 
+/* load address to register */
+static inline void emit_load_addr(unsigned int dst, unsigned int src,
+				     int imm, struct jit_ctx *ctx)
+{
+	/* src contains the base addr of the 32/64-pointer */
+	if (config_enabled(CONFIG_64BIT))
+		emit_instr(ctx, ld, dst, imm, src);
+	else
+		emit_instr(ctx, lw, dst, imm, src);
+}
+
 /* load a function pointer to register */
 static inline void emit_load_func(unsigned int reg, ptr imm,
 				  struct jit_ctx *ctx)
@@ -1271,7 +1282,8 @@ jmp_cmp:
 			/* A = skb->dev->ifindex */
 			ctx->flags |= SEEN_SKB | SEEN_A | SEEN_S0;
 			off = offsetof(struct sk_buff, dev);
-			emit_load(r_s0, r_skb, off, ctx);
+			/* Load address of *dev member */
+			emit_load_addr(r_s0, r_skb, off, ctx);
 			/* error (0) in the delay slot */
 			emit_bcond(MIPS_COND_EQ, r_s0, r_zero,
 				   b_imm(prog->len, ctx), ctx);
-- 
2.0.0

[PATCH 17/17] MIPS: bpf: Fix stack space allocation for BPF memwords on MIPS64

[Markos Chandras]

When allocating stack space for BPF memwords we need to use the
appropriate 32 or 64-bit instruction to avoid losing the top 32 bits
of the stack pointer.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/net/bpf_jit.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index d8dba7b523a5..545c8487542c 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -617,7 +617,10 @@ static void save_bpf_jit_regs(struct jit_ctx *ctx, unsigned offset)
 	if (ctx->flags & SEEN_MEM) {
 		if (real_off % (RSIZE * 2))
 			real_off += RSIZE;
-		emit_addiu(r_M, r_sp, real_off, ctx);
+		if (config_enabled(CONFIG_64BIT))
+			emit_daddiu(r_M, r_sp, real_off, ctx);
+		else
+			emit_addiu(r_M, r_sp, real_off, ctx);
 	}
 }
 
-- 
2.0.0

RE: [PATCH 14/17] MIPS: bpf: Prevent kernel fall over for >=32bit shifts

[David Laight]

From: Markos Chandras
> Remove BUG_ON() if the shift immediate is >=32 to avoid
> kernel crashes due to malicious user input. Since the micro-assembler
> will not allow an immediate greater or equal to 32, we will use the
> maximum value which is 31. This will do the correct thing on either 32-
> or 64-bit cores since no 64-bit instructions are being used in JIT.

I'm not sure that bounding the shift to 31 bits 'is the correct thing'.
I'd have thought that emulating the large shift or masking the shift
to 5 bits are equally 'correct'.

...
>  {
>  	/* sa is 5-bits long */
> -	BUG_ON(sa >= BIT(5));
> +	if (sa >= BIT(5))
> +		sa = BIT(5) - 1;
>  	emit_instr(ctx, sll, dst, src, sa);
...

	David

Re: [PATCH 14/17] MIPS: bpf: Prevent kernel fall over for >=32bit shifts

[Markos Chandras]

On 06/23/2014 10:44 AM, David Laight wrote:
> From: Markos Chandras
>> Remove BUG_ON() if the shift immediate is >=32 to avoid
>> kernel crashes due to malicious user input. Since the micro-assembler
>> will not allow an immediate greater or equal to 32, we will use the
>> maximum value which is 31. This will do the correct thing on either 32-
>> or 64-bit cores since no 64-bit instructions are being used in JIT.
> 
> I'm not sure that bounding the shift to 31 bits 'is the correct thing'.
> I'd have thought that emulating the large shift or masking the shift
> to 5 bits are equally 'correct'.
> 
> ...
Hi David,

Since we use 32-bit registers (or rather, we ignore the top 32bits on
MIPS64), shifting >= 32 will always result to 0.
Alexei suggested [1] to allow large shifts and emulate them, so this
patch aims to do that by treating >=32 shift values as 31. Please tell
me if I got this wrong.

[1] http://www.linux-mips.org/archives/linux-mips/2014-06/msg00212.html


-- 
markos

RE: [PATCH 14/17] MIPS: bpf: Prevent kernel fall over for >=32bit shifts

[David Laight]

From: Markos Chandras
> On 06/23/2014 10:44 AM, David Laight wrote:
> > From: Markos Chandras
> >> Remove BUG_ON() if the shift immediate is >=32 to avoid
> >> kernel crashes due to malicious user input. Since the micro-assembler
> >> will not allow an immediate greater or equal to 32, we will use the
> >> maximum value which is 31. This will do the correct thing on either 32-
> >> or 64-bit cores since no 64-bit instructions are being used in JIT.
> >
> > I'm not sure that bounding the shift to 31 bits 'is the correct thing'.
> > I'd have thought that emulating the large shift or masking the shift
> > to 5 bits are equally 'correct'.
> >
> > ...
> Hi David,
> 
> Since we use 32-bit registers (or rather, we ignore the top 32bits on
> MIPS64), shifting >= 32 will always result to 0.
> Alexei suggested [1] to allow large shifts and emulate them, so this
> patch aims to do that by treating >=32 shift values as 31. Please tell
> me if I got this wrong.

Shifting by 31 converts 0xffffffff to 1, not 0.

	David

Re: [PATCH 14/17] MIPS: bpf: Prevent kernel fall over for >=32bit shifts

[Markos Chandras]

On 06/23/2014 12:08 PM, David Laight wrote:
> From: Markos Chandras
>> On 06/23/2014 10:44 AM, David Laight wrote:
>>> From: Markos Chandras
>>>> Remove BUG_ON() if the shift immediate is >=32 to avoid
>>>> kernel crashes due to malicious user input. Since the micro-assembler
>>>> will not allow an immediate greater or equal to 32, we will use the
>>>> maximum value which is 31. This will do the correct thing on either 32-
>>>> or 64-bit cores since no 64-bit instructions are being used in JIT.
>>>
>>> I'm not sure that bounding the shift to 31 bits 'is the correct thing'.
>>> I'd have thought that emulating the large shift or masking the shift
>>> to 5 bits are equally 'correct'.
>>>
>>> ...
>> Hi David,
>>
>> Since we use 32-bit registers (or rather, we ignore the top 32bits on
>> MIPS64), shifting >= 32 will always result to 0.
>> Alexei suggested [1] to allow large shifts and emulate them, so this
>> patch aims to do that by treating >=32 shift values as 31. Please tell
>> me if I got this wrong.
> 
> Shifting by 31 converts 0xffffffff to 1, not 0.
> 
> 	David
> 
> 
> 
oops indeed. Maybe it can be fixed by something like this?


diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 545c8487542c..32233ec747e0 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -151,6 +151,8 @@ static inline int optimize_div(u32 *k)
         return 0;
 }

+static inline void emit_jit_reg_move(ptr dst, ptr src, struct jit_ctx
*ctx);
+
 /* Simply emit the instruction if the JIT memory space has been
allocated */
 #define emit_instr(ctx, func, ...)                      \
 do {                                                    \
@@ -310,8 +312,10 @@ static inline void emit_sll(unsigned int dst,
unsigned int src,
 {
         /* sa is 5-bits long */
         if (sa >= BIT(5))
-                sa = BIT(5) - 1;
-        emit_instr(ctx, sll, dst, src, sa);
+                /* Shifting >= 32 results in zero */
+                emit_jit_reg_move(dst, r_zero, ctx);
+        else
+                emit_instr(ctx, sll, dst, src, sa);
 }

 static inline void emit_srlv(unsigned int dst, unsigned int src,
@@ -325,8 +329,10 @@ static inline void emit_srl(unsigned int dst,
unsigned int src,
 {
         /* sa is 5-bits long */
         if (sa >= BIT(5))
-                sa =  BIT(5) - 1;
-        emit_instr(ctx, srl, dst, src, sa);
+                /* Shifting >= 32 results in zero */
+                emit_jit_reg_move(dst, r_zero, ctx);
+        else
+                emit_instr(ctx, srl, dst, src, sa);
 }


-- 
markos

Re: [PATCH 00/17] Misc MIPS/BPF fixes for 3.16

[David Miller]

From: Markos Chandras <markos.chandras@imgtec.com>
Date: Mon, 23 Jun 2014 10:38:43 +0100

> Here are some fixes for MIPS/BPF for 3.16. These fixes make
> the bpf testsuite *almost* happy with only 2 tests (LD_IND_LL,
> LD_IND_NET) failing at the moment. Since fixing the remaining tests
> is not so trivial, it would be nice to have these fixes in 3.16 for now.
> 
> The patches are based on the upstream-sfr/mips-for-linux-next tree
> because they depend on https://patchwork.linux-mips.org/patch/7099/

You did not CC: netdev on patches 1, 2, and 3.  Please do not do this,
people on this list will want to review the series as a whole.

Re: [PATCH 16/17] MIPS: bpf: Use 32 or 64-bit load instruction to load an address to register

[Paul Burton]

On Mon, Jun 23, 2014 at 10:38:59AM +0100, Markos Chandras wrote:
> When loading a pointer to a register we need to use the appropriate
> 32 or 64bit instruction to preserve the pointer's top 32bits.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Daniel Borkmann <dborkman@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
>  arch/mips/net/bpf_jit.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
> index 4920e0fd05ee..d8dba7b523a5 100644
> --- a/arch/mips/net/bpf_jit.c
> +++ b/arch/mips/net/bpf_jit.c
> @@ -447,6 +447,17 @@ static inline void emit_wsbh(unsigned int dst, unsigned int src,
>  	emit_instr(ctx, wsbh, dst, src);
>  }
>  
> +/* load address to register */
> +static inline void emit_load_addr(unsigned int dst, unsigned int src,
> +				     int imm, struct jit_ctx *ctx)

(I originally sent this in reply to your internal posting, but assume you
missed it or it got eaten somewhere along the way.)

The name emit_load_addr & comment "load address to register" makes this
sound like an equivalent of the "la" pseudo instruction, but it appears
to really emit a pointer sized load? How about emit_load_ptr or something
instead, and similarly s/address/pointer/ in the comment?

> +{
> +	/* src contains the base addr of the 32/64-pointer */
> +	if (config_enabled(CONFIG_64BIT))
> +		emit_instr(ctx, ld, dst, imm, src);
> +	else
> +		emit_instr(ctx, lw, dst, imm, src);

Is there some way you could make use of the UASM_i_LW macro (note the
capitalisation) instead of the if statement here?

Thanks,
    Paul

> +}
> +
>  /* load a function pointer to register */
>  static inline void emit_load_func(unsigned int reg, ptr imm,
>  				  struct jit_ctx *ctx)
> @@ -1271,7 +1282,8 @@ jmp_cmp:
>  			/* A = skb->dev->ifindex */
>  			ctx->flags |= SEEN_SKB | SEEN_A | SEEN_S0;
>  			off = offsetof(struct sk_buff, dev);
> -			emit_load(r_s0, r_skb, off, ctx);
> +			/* Load address of *dev member */
> +			emit_load_addr(r_s0, r_skb, off, ctx);
>  			/* error (0) in the delay slot */
>  			emit_bcond(MIPS_COND_EQ, r_s0, r_zero,
>  				   b_imm(prog->len, ctx), ctx);
> -- 
> 2.0.0
> 
> 

Re: [PATCH 05/17] MIPS: bpf: Return error code if the offset is a negative number

[Alexei Starovoitov]

On Mon, Jun 23, 2014 at 2:38 AM, Markos Chandras
<markos.chandras@imgtec.com> wrote:
> Previously, the negative offset was not checked leading to failures
> due to trying to load data beyond the skb struct boundaries. Until we
> have proper asm helpers in place, it's best if we return ENOSUPP if K
> is negative when trying to JIT the filter or 0 during runtime if we
> do an indirect load where the value of X is unknown during build time.
>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Daniel Borkmann <dborkman@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>

Hi Markos,

thank you for addressing all of my earlier comments.
Looks like test_bpf was quite useful in finding all of these bugs :)
For the patches that reached netdev:

Acked-by: Alexei Starovoitov <ast@plumgrid.com>

One minor nit below:

> ---
>  arch/mips/net/bpf_jit.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
> index 5cc92c4590cb..95728ea6cb74 100644
> --- a/arch/mips/net/bpf_jit.c
> +++ b/arch/mips/net/bpf_jit.c
> @@ -331,6 +331,12 @@ static inline void emit_srl(unsigned int dst, unsigned int src,
>         emit_instr(ctx, srl, dst, src, sa);
>  }
>
> +static inline void emit_slt(unsigned int dst, unsigned int src1,
> +                           unsigned int src2, struct jit_ctx *ctx)
> +{
> +       emit_instr(ctx, slt, dst, src1, src2);
> +}
> +
>  static inline void emit_sltu(unsigned int dst, unsigned int src1,
>                              unsigned int src2, struct jit_ctx *ctx)
>  {
> @@ -816,8 +822,21 @@ static int build_body(struct jit_ctx *ctx)
>                         /* A <- P[k:1] */
>                         load_order = 0;
>  load:
> +                       /* the interpreter will deal with the negative K */
> +                       if ((int)k < 0)

should be a space after cast.

Re: [PATCH 00/17] Misc MIPS/BPF fixes for 3.16

[Markos Chandras]

On 06/23/2014 08:49 PM, David Miller wrote:
> From: Markos Chandras <markos.chandras@imgtec.com>
> Date: Mon, 23 Jun 2014 10:38:43 +0100
> 
>> Here are some fixes for MIPS/BPF for 3.16. These fixes make
>> the bpf testsuite *almost* happy with only 2 tests (LD_IND_LL,
>> LD_IND_NET) failing at the moment. Since fixing the remaining tests
>> is not so trivial, it would be nice to have these fixes in 3.16 for now.
>>
>> The patches are based on the upstream-sfr/mips-for-linux-next tree
>> because they depend on https://patchwork.linux-mips.org/patch/7099/
> 
> You did not CC: netdev on patches 1, 2, and 3.  Please do not do this,
> people on this list will want to review the series as a whole.
> 
Apologies David. Patches 1,2,3 have nothing to do with bpf-jit and they
only fix/add MIPS micro-assembler instructions so I didn't though
netdev@ actually cared about that. I will be more careful in the future.

-- 
markos

Re: [PATCH 05/17] MIPS: bpf: Return error code if the offset is a negative number

[Markos Chandras]

On 06/23/2014 11:09 PM, Alexei Starovoitov wrote:
> On Mon, Jun 23, 2014 at 2:38 AM, Markos Chandras
> <markos.chandras@imgtec.com> wrote:
>> Previously, the negative offset was not checked leading to failures
>> due to trying to load data beyond the skb struct boundaries. Until we
>> have proper asm helpers in place, it's best if we return ENOSUPP if K
>> is negative when trying to JIT the filter or 0 during runtime if we
>> do an indirect load where the value of X is unknown during build time.
>>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Daniel Borkmann <dborkman@redhat.com>
>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> 
> Hi Markos,
> 
> thank you for addressing all of my earlier comments.
> Looks like test_bpf was quite useful in finding all of these bugs :)
> For the patches that reached netdev:
> 
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
> 

Thank you for the review and your constructive comments in your previous
emails.

-- 
markos

Re: [PATCH 16/17] MIPS: bpf: Use 32 or 64-bit load instruction to load an address to register

[Markos Chandras]

On 06/23/2014 09:24 PM, Paul Burton wrote:
> On Mon, Jun 23, 2014 at 10:38:59AM +0100, Markos Chandras wrote:
>> diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
>> index 4920e0fd05ee..d8dba7b523a5 100644
>> --- a/arch/mips/net/bpf_jit.c
>> +++ b/arch/mips/net/bpf_jit.c
>> @@ -447,6 +447,17 @@ static inline void emit_wsbh(unsigned int dst, unsigned int src,
>>  	emit_instr(ctx, wsbh, dst, src);
>>  }
>>  
>> +/* load address to register */
>> +static inline void emit_load_addr(unsigned int dst, unsigned int src,
>> +				     int imm, struct jit_ctx *ctx)
> 
> (I originally sent this in reply to your internal posting, but assume you
> missed it or it got eaten somewhere along the way.)
> 
> The name emit_load_addr & comment "load address to register" makes this
> sound like an equivalent of the "la" pseudo instruction, but it appears
> to really emit a pointer sized load? How about emit_load_ptr or something
> instead, and similarly s/address/pointer/ in the comment?
> 
Hi Paul,

I suppose I could do that. I will send a v2

>> +{
>> +	/* src contains the base addr of the 32/64-pointer */
>> +	if (config_enabled(CONFIG_64BIT))
>> +		emit_instr(ctx, ld, dst, imm, src);
>> +	else
>> +		emit_instr(ctx, lw, dst, imm, src);
> 
> Is there some way you could make use of the UASM_i_LW macro (note the
> capitalisation) instead of the if statement here?
> 

Not right now. I use config_enabled(CONFIG_64BIT) everywhere in that
file to emit 32-bit or 64-bit instructions. So I will look into
switching to the USAM_i_* macros when i submit the remaining fixes
probably for 3.17.

-- 
markos

[PATCH v2 14/17] MIPS: bpf: Prevent kernel fall over for >=32bit shifts

[Markos Chandras]

Remove BUG_ON() if the shift immediate is >=32 to avoid kernel crashes
due to malicious user input. If the shift immediate is >= 32,
we simply load the destination register with 0 since only
32-bit instructions are used by JIT so this will do the
correct thing even on MIPS64.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
Changes since v1:
- For sa >=32, load destination register with zero instead of treating
the immediate as 31
---
 arch/mips/net/bpf_jit.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 1bcd599d9971..9476e7f061a1 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -151,6 +151,8 @@ static inline int optimize_div(u32 *k)
 	return 0;
 }
 
+static inline void emit_jit_reg_move(ptr dst, ptr src, struct jit_ctx *ctx);
+
 /* Simply emit the instruction if the JIT memory space has been allocated */
 #define emit_instr(ctx, func, ...)			\
 do {							\
@@ -309,8 +311,11 @@ static inline void emit_sll(unsigned int dst, unsigned int src,
 			    unsigned int sa, struct jit_ctx *ctx)
 {
 	/* sa is 5-bits long */
-	BUG_ON(sa >= BIT(5));
-	emit_instr(ctx, sll, dst, src, sa);
+	if (sa >= BIT(5))
+		/* Shifting >= 32 results in zero */
+		emit_jit_reg_move(dst, r_zero, ctx);
+	else
+		emit_instr(ctx, sll, dst, src, sa);
 }
 
 static inline void emit_srlv(unsigned int dst, unsigned int src,
@@ -323,8 +328,11 @@ static inline void emit_srl(unsigned int dst, unsigned int src,
 			    unsigned int sa, struct jit_ctx *ctx)
 {
 	/* sa is 5-bits long */
-	BUG_ON(sa >= BIT(5));
-	emit_instr(ctx, srl, dst, src, sa);
+	if (sa >= BIT(5))
+		/* Shifting >= 32 results in zero */
+		emit_jit_reg_move(dst, r_zero, ctx);
+	else
+		emit_instr(ctx, srl, dst, src, sa);
 }
 
 static inline void emit_slt(unsigned int dst, unsigned int src1,
-- 
2.0.0

[PATCH v2 16/17] MIPS: bpf: Use 32 or 64-bit load instruction to load an address to register

[Markos Chandras]

When loading a pointer to register we need to use the appropriate
32 or 64bit instruction to preserve the pointers' top 32bits.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
Changes since v1:
- Change function name to make it clear that we are loading a
pointer to a register, not an address
---
 arch/mips/net/bpf_jit.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 4505e2e6ab53..6e3963425b64 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -453,6 +453,17 @@ static inline void emit_wsbh(unsigned int dst, unsigned int src,
 	emit_instr(ctx, wsbh, dst, src);
 }
 
+/* load pointer to register */
+static inline void emit_load_ptr(unsigned int dst, unsigned int src,
+				     int imm, struct jit_ctx *ctx)
+{
+	/* src contains the base addr of the 32/64-pointer */
+	if (config_enabled(CONFIG_64BIT))
+		emit_instr(ctx, ld, dst, imm, src);
+	else
+		emit_instr(ctx, lw, dst, imm, src);
+}
+
 /* load a function pointer to register */
 static inline void emit_load_func(unsigned int reg, ptr imm,
 				  struct jit_ctx *ctx)
@@ -1277,7 +1288,8 @@ jmp_cmp:
 			/* A = skb->dev->ifindex */
 			ctx->flags |= SEEN_SKB | SEEN_A | SEEN_S0;
 			off = offsetof(struct sk_buff, dev);
-			emit_load(r_s0, r_skb, off, ctx);
+			/* Load *dev pointer */
+			emit_load_ptr(r_s0, r_skb, off, ctx);
 			/* error (0) in the delay slot */
 			emit_bcond(MIPS_COND_EQ, r_s0, r_zero,
 				   b_imm(prog->len, ctx), ctx);
-- 
2.0.0

Re: [PATCH v2 16/17] MIPS: bpf: Use 32 or 64-bit load instruction to load an address to register

[Alexei Starovoitov]

On Wed, Jun 25, 2014 at 1:39 AM, Markos Chandras
<markos.chandras@imgtec.com> wrote:
> When loading a pointer to register we need to use the appropriate
> 32 or 64bit instruction to preserve the pointers' top 32bits.
>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Daniel Borkmann <dborkman@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
> Changes since v1:
> - Change function name to make it clear that we are loading a
> pointer to a register, not an address

Markos,

when you post v2, please refresh the whole series, add v2 to subject and repost
all patches from scratch.

Thanks