2022-05-20 18:59:33

by Shung-Hsi Yu

Subject: [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64

It may not be immediately clear why that ld_imm64 test cases are
rejected, especially for test1 and test2 where JMP to the 2nd
instruction of BPF_LD_IMM64 is performed.

Add brief explaination of why each test case in verifier/ld_imm64.c
should be rejected.

Signed-off-by: Shung-Hsi Yu <[email protected]>
---
.../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c
index f9297900cea6..021312641aaf 100644
--- a/tools/testing/selftests/bpf/verifier/ld_imm64.c
+++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c
@@ -1,5 +1,6 @@
+/* Note: BPF_LD_IMM64 is composed of two instructions of class BPF_LD */
{
- "test1 ld_imm64",
+ "test1 ld_imm64: reject JMP to 2nd instruction of BPF_LD_IMM64",
.insns = {
BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1),
BPF_LD_IMM64(BPF_REG_0, 0),
@@ -14,7 +15,7 @@
.result = REJECT,
},
{
- "test2 ld_imm64",
+ "test2 ld_imm64: reject JMP to 2nd instruction of BPF_LD_IMM64",
.insns = {
BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1),
BPF_LD_IMM64(BPF_REG_0, 0),
@@ -28,7 +29,7 @@
.result = REJECT,
},
{
- "test3 ld_imm64",
+ "test3 ld_imm64: reject incomplete BPF_LD_IMM64 instruction",
.insns = {
BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1),
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0),
@@ -42,7 +43,7 @@
.result = REJECT,
},
{
- "test4 ld_imm64",
+ "test4 ld_imm64: reject incomplete BPF_LD_IMM64 instruction",
.insns = {
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0),
BPF_EXIT_INSN(),
@@ -70,7 +71,7 @@
.retval = 1,
},
{
- "test8 ld_imm64",
+ "test8 ld_imm64: reject 1st off!=0",
.insns = {
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 1, 1),
BPF_RAW_INSN(0, 0, 0, 0, 1),
@@ -80,7 +81,7 @@
.result = REJECT,
},
{
- "test9 ld_imm64",
+ "test9 ld_imm64: reject 2nd off!=0",
.insns = {
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
BPF_RAW_INSN(0, 0, 0, 1, 1),
@@ -90,7 +91,7 @@
.result = REJECT,
},
{
- "test10 ld_imm64",
+ "test10 ld_imm64: reject 2nd dst_reg!=0",
.insns = {
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
BPF_RAW_INSN(0, BPF_REG_1, 0, 0, 1),
@@ -100,7 +101,7 @@
.result = REJECT,
},
{
- "test11 ld_imm64",
+ "test11 ld_imm64: reject 2nd src_reg!=0",
.insns = {
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
BPF_RAW_INSN(0, 0, BPF_REG_1, 0, 1),
@@ -113,6 +114,7 @@
"test12 ld_imm64",
.insns = {
BPF_MOV64_IMM(BPF_REG_1, 0),
+ /* BPF_REG_1 is interpreted as BPF_PSEUDO_MAP_FD */
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
BPF_RAW_INSN(0, 0, 0, 0, 0),
BPF_EXIT_INSN(),
@@ -121,7 +123,7 @@
.result = REJECT,
},
{
- "test13 ld_imm64",
+ "test13 ld_imm64: 2nd src_reg!=0",
.insns = {
BPF_MOV64_IMM(BPF_REG_1, 0),
BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
--
2.36.1

2022-05-21 11:14:21

by Yonghong Song

Subject: Re: [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64

On 5/20/22 4:37 AM, Shung-Hsi Yu wrote:
> It may not be immediately clear why that ld_imm64 test cases are
> rejected, especially for test1 and test2 where JMP to the 2nd
> instruction of BPF_LD_IMM64 is performed.
>
> Add brief explaination of why each test case in verifier/ld_imm64.c
> should be rejected.
>
> Signed-off-by: Shung-Hsi Yu <[email protected]>
> ---
> .../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c
> index f9297900cea6..021312641aaf 100644
> --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c
> +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c
> @@ -1,5 +1,6 @@
> +/* Note: BPF_LD_IMM64 is composed of two instructions of class BPF_LD */

> [...]LD | BPF_IMM | BPF_DW, 0, 0, 0, 0),
> @@ -42,7 +43,7 @@
> .result = REJECT,
> },
> {
> - "test4 ld_imm64",
> + "test4 ld_imm64: reject incomplete BPF_LD_IMM64 instruction",
> .insns = {
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0),
> BPF_EXIT_INSN(),
> @@ -70,7 +71,7 @@
> .retval = 1,
> },
> {
> - "test8 ld_imm64",
> + "test8 ld_imm64: reject 1st off!=0",

Let add some space like 'off != 0'. The same for
some of later test names.

> .insns = {
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 1, 1),
> BPF_RAW_INSN(0, 0, 0, 0, 1),
> @@ -80,7 +81,7 @@
> .result = REJECT,
> },
> {
> - "test9 ld_imm64",
> + "test9 ld_imm64: reject 2nd off!=0",
> .insns = {
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
> BPF_RAW_INSN(0, 0, 0, 1, 1),
> @@ -90,7 +91,7 @@
> .result = REJECT,
> },
> {
> - "test10 ld_imm64",
> + "test10 ld_imm64: reject 2nd dst_reg!=0",
> .insns = {
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
> BPF_RAW_INSN(0, BPF_REG_1, 0, 0, 1),
> @@ -100,7 +101,7 @@
> .result = REJECT,
> },
> {
> - "test11 ld_imm64",
> + "test11 ld_imm64: reject 2nd src_reg!=0",
> .insns = {
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
> BPF_RAW_INSN(0, 0, BPF_REG_1, 0, 1),
> @@ -113,6 +114,7 @@
> "test12 ld_imm64",
> .insns = {
> BPF_MOV64_IMM(BPF_REG_1, 0),
> + /* BPF_REG_1 is interpreted as BPF_PSEUDO_MAP_FD */
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
> BPF_RAW_INSN(0, 0, 0, 0, 0),
> BPF_EXIT_INSN(),
> @@ -121,7 +123,7 @@
> .result = REJECT,
> },
> {
> - "test13 ld_imm64",
> + "test13 ld_imm64: 2nd src_reg!=0",
> .insns = {
> BPF_MOV64_IMM(BPF_REG_1, 0),
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),

2022-05-24 07:45:50

by Shung-Hsi Yu

Subject: Re: [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64

On Fri, May 20, 2022 at 05:27:12PM -0700, Yonghong Song wrote:
>
>
> On 5/20/22 4:37 AM, Shung-Hsi Yu wrote:
> > It may not be immediately clear why that ld_imm64 test cases are
> > rejected, especially for test1 and test2 where JMP to the 2nd
> > instruction of BPF_LD_IMM64 is performed.
> >
> > Add brief explaination of why each test case in verifier/ld_imm64.c
> > should be rejected.
> >
> > Signed-off-by: Shung-Hsi Yu <[email protected]>
> > ---
> > .../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++++++---------
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c
> > index f9297900cea6..021312641aaf 100644
> > --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c
> > +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c
> > @@ -1,5 +1,6 @@
> > +/* Note: BPF_LD_IMM64 is composed of two instructions of class BPF_LD */
>
> > [...]LD | BPF_IMM | BPF_DW, 0, 0, 0, 0),
> > @@ -42,7 +43,7 @@
> > .result = REJECT,
> > },
> > {
> > - "test4 ld_imm64",
> > + "test4 ld_imm64: reject incomplete BPF_LD_IMM64 instruction",
> > .insns = {
> > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0),
> > BPF_EXIT_INSN(),
> > @@ -70,7 +71,7 @@
> > .retval = 1,
> > },
> > {
> > - "test8 ld_imm64",
> > + "test8 ld_imm64: reject 1st off!=0",
>
> Let add some space like 'off != 0'. The same for
> some of later test names.

Okay, will do that in the next version. Thanks!

> > .insns = {
> > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 1, 1),
> > BPF_RAW_INSN(0, 0, 0, 0, 1),
> > @@ -80,7 +81,7 @@
> > .result = REJECT,
> > },
> > {
> > - "test9 ld_imm64",
> > + "test9 ld_imm64: reject 2nd off!=0",
> > .insns = {
> > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
> > BPF_RAW_INSN(0, 0, 0, 1, 1),
> > @@ -90,7 +91,7 @@
> > .result = REJECT,
> > },
> > {
> > - "test10 ld_imm64",
> > + "test10 ld_imm64: reject 2nd dst_reg!=0",
> > .insns = {
> > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
> > BPF_RAW_INSN(0, BPF_REG_1, 0, 0, 1),
> > @@ -100,7 +101,7 @@
> > .result = REJECT,
> > },
> > {
> > - "test11 ld_imm64",
> > + "test11 ld_imm64: reject 2nd src_reg!=0",
> > .insns = {
> > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1),
> > BPF_RAW_INSN(0, 0, BPF_REG_1, 0, 1),
> > @@ -113,6 +114,7 @@
> > "test12 ld_imm64",
> > .insns = {
> > BPF_MOV64_IMM(BPF_REG_1, 0),
> > + /* BPF_REG_1 is interpreted as BPF_PSEUDO_MAP_FD */
> > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
> > BPF_RAW_INSN(0, 0, 0, 0, 0),
> > BPF_EXIT_INSN(),
> > @@ -121,7 +123,7 @@
> > .result = REJECT,
> > },
> > {
> > - "test13 ld_imm64",
> > + "test13 ld_imm64: 2nd src_reg!=0",
> > .insns = {
> > BPF_MOV64_IMM(BPF_REG_1, 0),
> > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
>