[PATCH v2 net-next 0/2] bpf/verifier: disassembly improvements

[PATCH v2 net-next 0/2] bpf/verifier: disassembly improvements

[Edward Cree]

Fix the output of print_bpf_insn() for ALU ops that don't look like
 compound assignment (i.e. BPF_END and BPF_NEG).

Sample output for a short test program:
0: (b4) (u32) r0 = (u32) 0
1: (dc) r0 = be32 r0
2: (84) r0 = (u32) -r0
3: (95) exit
processed 4 insns, stack depth 0

Edward Cree (2):
  bpf/verifier: improve disassembly of BPF_END instructions
  bpf/verifier: improve disassembly of BPF_NEG instructions

 kernel/bpf/verifier.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

[PATCH v2 net-next 1/2] bpf/verifier: improve disassembly of BPF_END instructions

[Edward Cree]

print_bpf_insn() was treating all BPF_ALU[64] the same, but BPF_END has a
 different structure: it has a size in insn->imm (even if it's BPF_X) and
 uses the BPF_SRC (X or K) to indicate which endianness to use.  So it
 needs different code to print it.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 kernel/bpf/verifier.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 799b245..3aaa3262 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -325,26 +325,40 @@ static const char *const bpf_jmp_string[16] = {
 	[BPF_EXIT >> 4] = "exit",
 };
 
+static void print_bpf_end_insn(const struct bpf_verifier_env *env,
+			       const struct bpf_insn *insn)
+{
+	verbose("(%02x) r%d = %s%d r%d\n", insn->code, insn->dst_reg,
+		BPF_SRC(insn->code) == BPF_TO_BE ? "be" : "le",
+		insn->imm, insn->dst_reg);
+}
+
 static void print_bpf_insn(const struct bpf_verifier_env *env,
 			   const struct bpf_insn *insn)
 {
 	u8 class = BPF_CLASS(insn->code);
 
 	if (class == BPF_ALU || class == BPF_ALU64) {
-		if (BPF_SRC(insn->code) == BPF_X)
+		if (BPF_OP(insn->code) == BPF_END) {
+			if (class == BPF_ALU64)
+				verbose("BUG_alu64_%02x\n", insn->code);
+			else
+				print_bpf_end_insn(env, insn);
+		} else if (BPF_SRC(insn->code) == BPF_X) {
 			verbose("(%02x) %sr%d %s %sr%d\n",
 				insn->code, class == BPF_ALU ? "(u32) " : "",
 				insn->dst_reg,
 				bpf_alu_string[BPF_OP(insn->code) >> 4],
 				class == BPF_ALU ? "(u32) " : "",
 				insn->src_reg);
-		else
+		} else {
 			verbose("(%02x) %sr%d %s %s%d\n",
 				insn->code, class == BPF_ALU ? "(u32) " : "",
 				insn->dst_reg,
 				bpf_alu_string[BPF_OP(insn->code) >> 4],
 				class == BPF_ALU ? "(u32) " : "",
 				insn->imm);
+		}
 	} else if (class == BPF_STX) {
 		if (BPF_MODE(insn->code) == BPF_MEM)
 			verbose("(%02x) *(%s *)(r%d %+d) = r%d\n",

[PATCH v2 net-next 2/2] bpf/verifier: improve disassembly of BPF_NEG instructions

[Edward Cree]

BPF_NEG takes only one operand, unlike the bulk of BPF_ALU[64] which are
 compound-assignments.  So give it its own format in print_bpf_insn().

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3aaa3262..04e0508 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -344,6 +344,11 @@ static void print_bpf_insn(const struct bpf_verifier_env *env,
 				verbose("BUG_alu64_%02x\n", insn->code);
 			else
 				print_bpf_end_insn(env, insn);
+		} else if (BPF_OP(insn->code) == BPF_NEG) {
+			verbose("(%02x) r%d = %s-r%d\n",
+				insn->code, insn->dst_reg,
+				class == BPF_ALU ? "(u32) " : "",
+				insn->dst_reg);
 		} else if (BPF_SRC(insn->code) == BPF_X) {
 			verbose("(%02x) %sr%d %s %sr%d\n",
 				insn->code, class == BPF_ALU ? "(u32) " : "",

Re: [PATCH v2 net-next 1/2] bpf/verifier: improve disassembly of BPF_END instructions

[Alexei Starovoitov]

On Tue, Sep 26, 2017 at 04:35:13PM +0100, Edward Cree wrote:
> print_bpf_insn() was treating all BPF_ALU[64] the same, but BPF_END has a
>  different structure: it has a size in insn->imm (even if it's BPF_X) and
>  uses the BPF_SRC (X or K) to indicate which endianness to use.  So it
>  needs different code to print it.
> 
> Signed-off-by: Edward Cree <ecree@solarflare.com>

well, it's an improvement over what we have today, so
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH v2 net-next 2/2] bpf/verifier: improve disassembly of BPF_NEG instructions

[Alexei Starovoitov]

On Tue, Sep 26, 2017 at 04:35:29PM +0100, Edward Cree wrote:
> BPF_NEG takes only one operand, unlike the bulk of BPF_ALU[64] which are
>  compound-assignments.  So give it its own format in print_bpf_insn().
> 
> Signed-off-by: Edward Cree <ecree@solarflare.com>

Acked-by: Alexei Starovoitov <ast@kernel.org>

thank you for the cleanup.

Re: [PATCH v2 net-next 1/2] bpf/verifier: improve disassembly of BPF_END instructions

[Daniel Borkmann]

On 09/26/2017 05:35 PM, Edward Cree wrote:
> print_bpf_insn() was treating all BPF_ALU[64] the same, but BPF_END has a
>   different structure: it has a size in insn->imm (even if it's BPF_X) and
>   uses the BPF_SRC (X or K) to indicate which endianness to use.  So it
>   needs different code to print it.
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH v2 net-next 2/2] bpf/verifier: improve disassembly of BPF_NEG instructions

[Daniel Borkmann]

On 09/26/2017 05:35 PM, Edward Cree wrote:
> BPF_NEG takes only one operand, unlike the bulk of BPF_ALU[64] which are
>   compound-assignments.  So give it its own format in print_bpf_insn().
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH v2 net-next 0/2] bpf/verifier: disassembly improvements

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Tue, 26 Sep 2017 16:32:15 +0100

> Fix the output of print_bpf_insn() for ALU ops that don't look like
>  compound assignment (i.e. BPF_END and BPF_NEG).
> 
> Sample output for a short test program:
> 0: (b4) (u32) r0 = (u32) 0
> 1: (dc) r0 = be32 r0
> 2: (84) r0 = (u32) -r0
> 3: (95) exit
> processed 4 insns, stack depth 0

Series applied.