* [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations
@ 2019-05-30 19:07 Luke Nelson
2019-05-30 19:08 ` [PATCH 2/2] bpf: test_bpf: add tests for upper bits of 32-bit operations Luke Nelson
2019-05-30 20:53 ` [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations Song Liu
0 siblings, 2 replies; 7+ messages in thread
From: Luke Nelson @ 2019-05-30 19:07 UTC (permalink / raw)
Cc: Song Liu, Albert Ou, bpf, Daniel Borkmann, Luke Nelson,
Björn Töpel, Palmer Dabbelt, Alexei Starovoitov,
linux-kernel, netdev, Yonghong Song, linux-riscv,
Martin KaFai Lau, Xi Wang
In BPF, 32-bit ALU operations should zero-extend their results into
the 64-bit registers. The current BPF JIT on RISC-V emits incorrect
instructions that perform either sign extension only (e.g., addw/subw)
or no extension on 32-bit add, sub, and, or, xor, lsh, rsh, arsh,
and neg. This behavior diverges from the interpreter and JITs for
other architectures.
This patch fixes the bugs by performing zero extension on the destination
register of 32-bit ALU operations.
Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G")
Cc: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
---
arch/riscv/net/bpf_jit_comp.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/arch/riscv/net/bpf_jit_comp.c b/arch/riscv/net/bpf_jit_comp.c
index 80b12aa5e10d..426d5c33ea90 100644
--- a/arch/riscv/net/bpf_jit_comp.c
+++ b/arch/riscv/net/bpf_jit_comp.c
@@ -751,22 +751,32 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
case BPF_ALU | BPF_ADD | BPF_X:
case BPF_ALU64 | BPF_ADD | BPF_X:
emit(is64 ? rv_add(rd, rd, rs) : rv_addw(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_SUB | BPF_X:
case BPF_ALU64 | BPF_SUB | BPF_X:
emit(is64 ? rv_sub(rd, rd, rs) : rv_subw(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_AND | BPF_X:
case BPF_ALU64 | BPF_AND | BPF_X:
emit(rv_and(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_OR | BPF_X:
case BPF_ALU64 | BPF_OR | BPF_X:
emit(rv_or(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_XOR | BPF_X:
case BPF_ALU64 | BPF_XOR | BPF_X:
emit(rv_xor(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_MUL | BPF_X:
case BPF_ALU64 | BPF_MUL | BPF_X:
@@ -789,14 +799,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
case BPF_ALU | BPF_LSH | BPF_X:
case BPF_ALU64 | BPF_LSH | BPF_X:
emit(is64 ? rv_sll(rd, rd, rs) : rv_sllw(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_RSH | BPF_X:
case BPF_ALU64 | BPF_RSH | BPF_X:
emit(is64 ? rv_srl(rd, rd, rs) : rv_srlw(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_ARSH | BPF_X:
case BPF_ALU64 | BPF_ARSH | BPF_X:
emit(is64 ? rv_sra(rd, rd, rs) : rv_sraw(rd, rd, rs), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
/* dst = -dst */
@@ -804,6 +820,8 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
case BPF_ALU64 | BPF_NEG:
emit(is64 ? rv_sub(rd, RV_REG_ZERO, rd) :
rv_subw(rd, RV_REG_ZERO, rd), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
/* dst = BSWAP##imm(dst) */
@@ -958,14 +976,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
case BPF_ALU | BPF_LSH | BPF_K:
case BPF_ALU64 | BPF_LSH | BPF_K:
emit(is64 ? rv_slli(rd, rd, imm) : rv_slliw(rd, rd, imm), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_RSH | BPF_K:
case BPF_ALU64 | BPF_RSH | BPF_K:
emit(is64 ? rv_srli(rd, rd, imm) : rv_srliw(rd, rd, imm), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
case BPF_ALU | BPF_ARSH | BPF_K:
case BPF_ALU64 | BPF_ARSH | BPF_K:
emit(is64 ? rv_srai(rd, rd, imm) : rv_sraiw(rd, rd, imm), ctx);
+ if (!is64)
+ emit_zext_32(rd, ctx);
break;
/* JUMP off */
--
2.19.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] bpf: test_bpf: add tests for upper bits of 32-bit operations
2019-05-30 19:07 [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations Luke Nelson
@ 2019-05-30 19:08 ` Luke Nelson
2019-05-30 20:30 ` Jiong Wang
2019-05-30 20:53 ` [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations Song Liu
1 sibling, 1 reply; 7+ messages in thread
From: Luke Nelson @ 2019-05-30 19:08 UTC (permalink / raw)
Cc: Song Liu, Albert Ou, bpf, Daniel Borkmann, Luke Nelson,
Björn Töpel, Palmer Dabbelt, Alexei Starovoitov,
linux-kernel, netdev, Yonghong Song, linux-riscv,
Martin KaFai Lau, Xi Wang
This commit introduces tests that validate the upper 32 bits
of the result of 32-bit BPF ALU operations.
The existing tests for 32-bit operations do not check the upper 32
bits of results because the exit instruction truncates the result.
These tests perform a 32-bit ALU operation followed by a right shift.
These tests can catch subtle bugs in the extension behavior of JITed
instructions, including several bugs in the RISC-V BPF JIT, fixed in
another patch.
The added tests pass the JIT and interpreter on x86, as well as the
JIT and interpreter of RISC-V once the zero extension bugs were fixed.
Cc: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
---
lib/test_bpf.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 164 insertions(+)
diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index 0845f635f404..4580dc0220f1 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -2461,6 +2461,20 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 1 } },
},
+ {
+ "ALU_ADD_X: (1 + 4294967294) >> 32 + 4294967294 = 4294967294",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 1U),
+ BPF_ALU32_IMM(BPF_MOV, R1, 4294967294U),
+ BPF_ALU32_REG(BPF_ADD, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_REG(BPF_ADD, R0, R1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 4294967294U } },
+ },
{
"ALU64_ADD_X: 1 + 2 = 3",
.u.insns_int = {
@@ -2812,6 +2826,20 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 1 } },
},
+ {
+ "ALU_SUB_X: (4294967295 - 1) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 4294967295U),
+ BPF_ALU32_IMM(BPF_MOV, R1, 1U),
+ BPF_ALU32_REG(BPF_SUB, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_REG(BPF_ADD, R0, R1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU64_SUB_X: 3 - 1 = 2",
.u.insns_int = {
@@ -3391,6 +3419,20 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 0xffffffff } },
},
+ {
+ "ALU_AND_X: (-1 & -1) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, -1UL),
+ BPF_LD_IMM64(R1, -1UL),
+ BPF_ALU32_REG(BPF_AND, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1U),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU64_AND_X: 3 & 2 = 2",
.u.insns_int = {
@@ -3533,6 +3575,20 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 0xffffffff } },
},
+ {
+ "ALU_OR_X: (0 & -1) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 0),
+ BPF_LD_IMM64(R1, -1UL),
+ BPF_ALU32_REG(BPF_OR, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1U),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU64_OR_X: 1 | 2 = 3",
.u.insns_int = {
@@ -3675,6 +3731,20 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 0xfffffffe } },
},
+ {
+ "ALU_XOR_X: (0 ^ -1) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 0),
+ BPF_LD_IMM64(R1, -1UL),
+ BPF_ALU32_REG(BPF_XOR, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1U),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU64_XOR_X: 5 ^ 6 = 3",
.u.insns_int = {
@@ -3817,6 +3887,20 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 0x80000000 } },
},
+ {
+ "ALU_LSH_X: (1 << 31) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 1),
+ BPF_ALU32_IMM(BPF_MOV, R1, 31),
+ BPF_ALU32_REG(BPF_LSH, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU64_LSH_X: 1 << 1 = 2",
.u.insns_int = {
@@ -3842,6 +3926,19 @@ static struct bpf_test tests[] = {
{ { 0, 0x80000000 } },
},
/* BPF_ALU | BPF_LSH | BPF_K */
+ {
+ "ALU_LSH_K: (1 << 31) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 1),
+ BPF_ALU32_IMM(BPF_LSH, R0, 31),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU_LSH_K: 1 << 1 = 2",
.u.insns_int = {
@@ -3911,6 +4008,20 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 1 } },
},
+ {
+ "ALU_RSH_X: (0x80000000 >> 0) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 0x80000000),
+ BPF_ALU32_IMM(BPF_MOV, R1, 0),
+ BPF_ALU32_REG(BPF_RSH, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU64_RSH_X: 2 >> 1 = 1",
.u.insns_int = {
@@ -3936,6 +4047,19 @@ static struct bpf_test tests[] = {
{ { 0, 1 } },
},
/* BPF_ALU | BPF_RSH | BPF_K */
+ {
+ "ALU_RSH_K: (0x80000000 >> 0) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 0x80000000),
+ BPF_ALU32_IMM(BPF_RSH, R0, 0),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU_RSH_K: 2 >> 1 = 1",
.u.insns_int = {
@@ -3993,7 +4117,34 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 0xffff00ff } },
},
+ {
+ "ALU_ARSH_X: (0x80000000 >> 0) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 0x80000000),
+ BPF_ALU32_IMM(BPF_MOV, R1, 0),
+ BPF_ALU32_REG(BPF_ARSH, R0, R1),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
/* BPF_ALU | BPF_ARSH | BPF_K */
+ {
+ "ALU_ARSH_K: (0x80000000 >> 0) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_LD_IMM64(R0, 0x80000000),
+ BPF_ALU32_IMM(BPF_ARSH, R0, 0),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU32_IMM(BPF_ADD, R0, 1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU_ARSH_K: 0xff00ff0000000000 >> 40 = 0xffffffffffff00ff",
.u.insns_int = {
@@ -4028,6 +4179,19 @@ static struct bpf_test tests[] = {
{ },
{ { 0, 3 } },
},
+ {
+ "ALU_NEG: -(1) >> 32 + 1 = 1",
+ .u.insns_int = {
+ BPF_ALU32_IMM(BPF_MOV, R0, 1),
+ BPF_ALU32_IMM(BPF_NEG, R0, 0),
+ BPF_ALU64_IMM(BPF_RSH, R0, 32),
+ BPF_ALU64_IMM(BPF_ADD, R0, 1),
+ BPF_EXIT_INSN(),
+ },
+ INTERNAL,
+ { },
+ { { 0, 1 } },
+ },
{
"ALU64_NEG: -(3) = -3",
.u.insns_int = {
--
2.19.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] bpf: test_bpf: add tests for upper bits of 32-bit operations
2019-05-30 19:08 ` [PATCH 2/2] bpf: test_bpf: add tests for upper bits of 32-bit operations Luke Nelson
@ 2019-05-30 20:30 ` Jiong Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jiong Wang @ 2019-05-30 20:30 UTC (permalink / raw)
To: Luke Nelson
Cc: Song Liu, Albert Ou, bpf, Daniel Borkmann, Björn Töpel,
Palmer Dabbelt, Alexei Starovoitov, linux-kernel, netdev,
Yonghong Song, linux-riscv, Martin KaFai Lau, Xi Wang
Luke Nelson writes:
> This commit introduces tests that validate the upper 32 bits
> of the result of 32-bit BPF ALU operations.
>
> The existing tests for 32-bit operations do not check the upper 32
> bits of results because the exit instruction truncates the result.
> These tests perform a 32-bit ALU operation followed by a right shift.
> These tests can catch subtle bugs in the extension behavior of JITed
> instructions, including several bugs in the RISC-V BPF JIT, fixed in
> another patch.
Hi Luke,
Have you seen the following?
https://www.spinics.net/lists/netdev/msg573355.html
it has been merged to bpf tree and should have full test coverage of all
bpf insns that could write to sub-register and are exposed to JIT
back-end.
And AFAIK, we add new unit tests to test_verifier which is a userspace
test infrastructure which offers more test functionality plus tests will
go through verifier.
Regards,
Jiong
> The added tests pass the JIT and interpreter on x86, as well as the
> JIT and interpreter of RISC-V once the zero extension bugs were fixed.
>
> Cc: Xi Wang <xi.wang@gmail.com>
> Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
> ---
> lib/test_bpf.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 164 insertions(+)
>
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index 0845f635f404..4580dc0220f1 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -2461,6 +2461,20 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 1 } },
> },
> + {
> + "ALU_ADD_X: (1 + 4294967294) >> 32 + 4294967294 = 4294967294",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 1U),
> + BPF_ALU32_IMM(BPF_MOV, R1, 4294967294U),
> + BPF_ALU32_REG(BPF_ADD, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_REG(BPF_ADD, R0, R1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 4294967294U } },
> + },
> {
> "ALU64_ADD_X: 1 + 2 = 3",
> .u.insns_int = {
> @@ -2812,6 +2826,20 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 1 } },
> },
> + {
> + "ALU_SUB_X: (4294967295 - 1) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 4294967295U),
> + BPF_ALU32_IMM(BPF_MOV, R1, 1U),
> + BPF_ALU32_REG(BPF_SUB, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_REG(BPF_ADD, R0, R1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU64_SUB_X: 3 - 1 = 2",
> .u.insns_int = {
> @@ -3391,6 +3419,20 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 0xffffffff } },
> },
> + {
> + "ALU_AND_X: (-1 & -1) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, -1UL),
> + BPF_LD_IMM64(R1, -1UL),
> + BPF_ALU32_REG(BPF_AND, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1U),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU64_AND_X: 3 & 2 = 2",
> .u.insns_int = {
> @@ -3533,6 +3575,20 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 0xffffffff } },
> },
> + {
> + "ALU_OR_X: (0 & -1) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 0),
> + BPF_LD_IMM64(R1, -1UL),
> + BPF_ALU32_REG(BPF_OR, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1U),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU64_OR_X: 1 | 2 = 3",
> .u.insns_int = {
> @@ -3675,6 +3731,20 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 0xfffffffe } },
> },
> + {
> + "ALU_XOR_X: (0 ^ -1) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 0),
> + BPF_LD_IMM64(R1, -1UL),
> + BPF_ALU32_REG(BPF_XOR, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1U),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU64_XOR_X: 5 ^ 6 = 3",
> .u.insns_int = {
> @@ -3817,6 +3887,20 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 0x80000000 } },
> },
> + {
> + "ALU_LSH_X: (1 << 31) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 1),
> + BPF_ALU32_IMM(BPF_MOV, R1, 31),
> + BPF_ALU32_REG(BPF_LSH, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU64_LSH_X: 1 << 1 = 2",
> .u.insns_int = {
> @@ -3842,6 +3926,19 @@ static struct bpf_test tests[] = {
> { { 0, 0x80000000 } },
> },
> /* BPF_ALU | BPF_LSH | BPF_K */
> + {
> + "ALU_LSH_K: (1 << 31) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 1),
> + BPF_ALU32_IMM(BPF_LSH, R0, 31),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU_LSH_K: 1 << 1 = 2",
> .u.insns_int = {
> @@ -3911,6 +4008,20 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 1 } },
> },
> + {
> + "ALU_RSH_X: (0x80000000 >> 0) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 0x80000000),
> + BPF_ALU32_IMM(BPF_MOV, R1, 0),
> + BPF_ALU32_REG(BPF_RSH, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU64_RSH_X: 2 >> 1 = 1",
> .u.insns_int = {
> @@ -3936,6 +4047,19 @@ static struct bpf_test tests[] = {
> { { 0, 1 } },
> },
> /* BPF_ALU | BPF_RSH | BPF_K */
> + {
> + "ALU_RSH_K: (0x80000000 >> 0) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 0x80000000),
> + BPF_ALU32_IMM(BPF_RSH, R0, 0),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU_RSH_K: 2 >> 1 = 1",
> .u.insns_int = {
> @@ -3993,7 +4117,34 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 0xffff00ff } },
> },
> + {
> + "ALU_ARSH_X: (0x80000000 >> 0) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 0x80000000),
> + BPF_ALU32_IMM(BPF_MOV, R1, 0),
> + BPF_ALU32_REG(BPF_ARSH, R0, R1),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> /* BPF_ALU | BPF_ARSH | BPF_K */
> + {
> + "ALU_ARSH_K: (0x80000000 >> 0) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_LD_IMM64(R0, 0x80000000),
> + BPF_ALU32_IMM(BPF_ARSH, R0, 0),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU32_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU_ARSH_K: 0xff00ff0000000000 >> 40 = 0xffffffffffff00ff",
> .u.insns_int = {
> @@ -4028,6 +4179,19 @@ static struct bpf_test tests[] = {
> { },
> { { 0, 3 } },
> },
> + {
> + "ALU_NEG: -(1) >> 32 + 1 = 1",
> + .u.insns_int = {
> + BPF_ALU32_IMM(BPF_MOV, R0, 1),
> + BPF_ALU32_IMM(BPF_NEG, R0, 0),
> + BPF_ALU64_IMM(BPF_RSH, R0, 32),
> + BPF_ALU64_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + INTERNAL,
> + { },
> + { { 0, 1 } },
> + },
> {
> "ALU64_NEG: -(3) = -3",
> .u.insns_int = {
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations
2019-05-30 19:07 [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations Luke Nelson
2019-05-30 19:08 ` [PATCH 2/2] bpf: test_bpf: add tests for upper bits of 32-bit operations Luke Nelson
@ 2019-05-30 20:53 ` Song Liu
2019-05-30 22:34 ` Luke Nelson
1 sibling, 1 reply; 7+ messages in thread
From: Song Liu @ 2019-05-30 20:53 UTC (permalink / raw)
To: Luke Nelson
Cc: Song Liu, Albert Ou, bpf, Daniel Borkmann, Björn Töpel,
Palmer Dabbelt, Alexei Starovoitov, open list, Networking,
Yonghong Song, linux-riscv, Martin KaFai Lau, Xi Wang
On Thu, May 30, 2019 at 12:09 PM Luke Nelson <luke.r.nels@gmail.com> wrote:
>
> In BPF, 32-bit ALU operations should zero-extend their results into
> the 64-bit registers. The current BPF JIT on RISC-V emits incorrect
> instructions that perform either sign extension only (e.g., addw/subw)
> or no extension on 32-bit add, sub, and, or, xor, lsh, rsh, arsh,
> and neg. This behavior diverges from the interpreter and JITs for
> other architectures.
>
> This patch fixes the bugs by performing zero extension on the destination
> register of 32-bit ALU operations.
>
> Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G")
> Cc: Xi Wang <xi.wang@gmail.com>
> Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
This is a little messy. How about we introduce some helper function
like:
/* please find a better name... */
emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct
rv_jit_context *ctx)
{
if (is64)
emit(insn_64, ctx);
else {
emit(insn_32, ctx);
rd = xxxx;
emit_zext_32(rd, ctx);
}
}
Thanks,
Song
> ---
> arch/riscv/net/bpf_jit_comp.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/arch/riscv/net/bpf_jit_comp.c b/arch/riscv/net/bpf_jit_comp.c
> index 80b12aa5e10d..426d5c33ea90 100644
> --- a/arch/riscv/net/bpf_jit_comp.c
> +++ b/arch/riscv/net/bpf_jit_comp.c
> @@ -751,22 +751,32 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
> case BPF_ALU | BPF_ADD | BPF_X:
> case BPF_ALU64 | BPF_ADD | BPF_X:
> emit(is64 ? rv_add(rd, rd, rs) : rv_addw(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_SUB | BPF_X:
> case BPF_ALU64 | BPF_SUB | BPF_X:
> emit(is64 ? rv_sub(rd, rd, rs) : rv_subw(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_AND | BPF_X:
> case BPF_ALU64 | BPF_AND | BPF_X:
> emit(rv_and(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_OR | BPF_X:
> case BPF_ALU64 | BPF_OR | BPF_X:
> emit(rv_or(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_XOR | BPF_X:
> case BPF_ALU64 | BPF_XOR | BPF_X:
> emit(rv_xor(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_MUL | BPF_X:
> case BPF_ALU64 | BPF_MUL | BPF_X:
> @@ -789,14 +799,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
> case BPF_ALU | BPF_LSH | BPF_X:
> case BPF_ALU64 | BPF_LSH | BPF_X:
> emit(is64 ? rv_sll(rd, rd, rs) : rv_sllw(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_RSH | BPF_X:
> case BPF_ALU64 | BPF_RSH | BPF_X:
> emit(is64 ? rv_srl(rd, rd, rs) : rv_srlw(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_ARSH | BPF_X:
> case BPF_ALU64 | BPF_ARSH | BPF_X:
> emit(is64 ? rv_sra(rd, rd, rs) : rv_sraw(rd, rd, rs), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
>
> /* dst = -dst */
> @@ -804,6 +820,8 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
> case BPF_ALU64 | BPF_NEG:
> emit(is64 ? rv_sub(rd, RV_REG_ZERO, rd) :
> rv_subw(rd, RV_REG_ZERO, rd), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
>
> /* dst = BSWAP##imm(dst) */
> @@ -958,14 +976,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
> case BPF_ALU | BPF_LSH | BPF_K:
> case BPF_ALU64 | BPF_LSH | BPF_K:
> emit(is64 ? rv_slli(rd, rd, imm) : rv_slliw(rd, rd, imm), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_RSH | BPF_K:
> case BPF_ALU64 | BPF_RSH | BPF_K:
> emit(is64 ? rv_srli(rd, rd, imm) : rv_srliw(rd, rd, imm), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
> case BPF_ALU | BPF_ARSH | BPF_K:
> case BPF_ALU64 | BPF_ARSH | BPF_K:
> emit(is64 ? rv_srai(rd, rd, imm) : rv_sraiw(rd, rd, imm), ctx);
> + if (!is64)
> + emit_zext_32(rd, ctx);
> break;
>
> /* JUMP off */
> --
> 2.19.1
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations
2019-05-30 20:53 ` [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations Song Liu
@ 2019-05-30 22:34 ` Luke Nelson
2019-05-30 23:08 ` Song Liu
0 siblings, 1 reply; 7+ messages in thread
From: Luke Nelson @ 2019-05-30 22:34 UTC (permalink / raw)
To: Song Liu
Cc: Song Liu, Albert Ou, bpf, Daniel Borkmann, Björn Töpel,
Palmer Dabbelt, Alexei Starovoitov, open list, Networking,
Yonghong Song, linux-riscv, Martin KaFai Lau, Xi Wang
On Thu, May 30, 2019 at 1:53 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> This is a little messy. How about we introduce some helper function
> like:
>
> /* please find a better name... */
> emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct
> rv_jit_context *ctx)
> {
> if (is64)
> emit(insn_64, ctx);
> else {
> emit(insn_32, ctx);
> rd = xxxx;
> emit_zext_32(rd, ctx);
> }
> }
This same check is used throughout the file, maybe clean it up in a
separate patch?
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations
2019-05-30 22:34 ` Luke Nelson
@ 2019-05-30 23:08 ` Song Liu
2019-05-31 7:22 ` Jiong Wang
0 siblings, 1 reply; 7+ messages in thread
From: Song Liu @ 2019-05-30 23:08 UTC (permalink / raw)
To: Luke Nelson
Cc: Song Liu, Albert Ou, bpf, Daniel Borkmann, Björn Töpel,
Palmer Dabbelt, Alexei Starovoitov, open list, Networking,
Yonghong Song, linux-riscv, Martin KaFai Lau, Xi Wang
On Thu, May 30, 2019 at 3:34 PM Luke Nelson <luke.r.nels@gmail.com> wrote:
>
> On Thu, May 30, 2019 at 1:53 PM Song Liu <liu.song.a23@gmail.com> wrote:
> >
> > This is a little messy. How about we introduce some helper function
> > like:
> >
> > /* please find a better name... */
> > emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct
> > rv_jit_context *ctx)
> > {
> > if (is64)
> > emit(insn_64, ctx);
> > else {
> > emit(insn_32, ctx);
> > rd = xxxx;
> > emit_zext_32(rd, ctx);
> > }
> > }
>
> This same check is used throughout the file, maybe clean it up in a
> separate patch?
Yes, let's do follow up patch.
Thanks,
Song
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations
2019-05-30 23:08 ` Song Liu
@ 2019-05-31 7:22 ` Jiong Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jiong Wang @ 2019-05-31 7:22 UTC (permalink / raw)
To: Luke Nelson, Song Liu
Cc: Song Liu, Albert Ou, bpf, Daniel Borkmann, Björn Töpel,
Palmer Dabbelt, Alexei Starovoitov, open list, Networking,
Yonghong Song, linux-riscv, Martin KaFai Lau, Xi Wang
Song Liu writes:
> On Thu, May 30, 2019 at 3:34 PM Luke Nelson <luke.r.nels@gmail.com> wrote:
>>
>> On Thu, May 30, 2019 at 1:53 PM Song Liu <liu.song.a23@gmail.com> wrote:
>> >
>> > This is a little messy. How about we introduce some helper function
>> > like:
>> >
>> > /* please find a better name... */
>> > emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct
>> > rv_jit_context *ctx)
>> > {
>> > if (is64)
>> > emit(insn_64, ctx);
>> > else {
>> > emit(insn_32, ctx);
>> > rd = xxxx;
>> > emit_zext_32(rd, ctx);
>> > }
>> > }
>>
>> This same check is used throughout the file, maybe clean it up in a
>> separate patch?
We also need to enable the recent 32-bit opt (on bpf-next) on these missing
insns, like what has been done at:
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=66d0d5a854a6625974e7de4b874e7934988b0ef8
Perhaps the best way is to wait this patch merged back to bpf-next, then we
do two patches, the first one to enable the opt, the second one then do the
re-factor. I guess this could avoid some code conflict.
Regards,
Jiong
>
> Yes, let's do follow up patch.
>
> Thanks,
> Song
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-31 7:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-30 19:07 [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations Luke Nelson
2019-05-30 19:08 ` [PATCH 2/2] bpf: test_bpf: add tests for upper bits of 32-bit operations Luke Nelson
2019-05-30 20:30 ` Jiong Wang
2019-05-30 20:53 ` [PATCH 1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations Song Liu
2019-05-30 22:34 ` Luke Nelson
2019-05-30 23:08 ` Song Liu
2019-05-31 7:22 ` Jiong Wang
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).