* [PATCH bpf-next 0/4] bpf: verifier: remove redundant opcode checks
@ 2022-05-20 11:37 Shung-Hsi Yu
2022-05-20 11:37 ` [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64 Shung-Hsi Yu
0 siblings, 1 reply; 4+ messages in thread
From: Shung-Hsi Yu @ 2022-05-20 11:37 UTC (permalink / raw)
To: netdev, bpf, linux-kernel, linux-kselftest
Cc: Shung-Hsi Yu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Shuah Khan
This patch set aims to remove opcode checks in BPF verifier that have
become redundant since commit 5e581dad4fec ("bpf: make unknown opcode
handling more robust"), either remove them entirely, or turn them into
comments in places where the redundancy may not be clear.
The exceptions here are opcode check for BPF_LD_{ABS,IND} and
BPF_JMP_{JA,CALL,EXIT}; they cover opcode validation not done in
bpf_opcode_in_insntable() so is not removed.
After apply the patch set test_verifier passes and does not need further
modification:
Summary: 1348 PASSED, 635 SKIPPED, 0 FAILED
Also, add comments at places that I find confusing while working on the
removal, namely:
1. resolve_pseudo_ldimm64() also validates opcode
2. BPF_SIZE check in check_ld_imm() guards against JMP to the 2nd
BPF_LD_IMM64 instruction
3. reason behind why ld_imm64 test cases should be rejected by the
verifier
Shung-Hsi Yu (4):
bpf: verifier: update resolve_pseudo_ldimm64() comment
bpf: verifier: explain opcode check in check_ld_imm()
bpf: verifier: remove redundant opcode checks
selftests/bpf: add reason of rejection in ld_imm64
kernel/bpf/verifier.c | 33 ++++++++-----------
.../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++-----
2 files changed, 25 insertions(+), 28 deletions(-)
base-commit: 68084a13642001b73aade05819584f18945f3297
--
2.36.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64
2022-05-20 11:37 [PATCH bpf-next 0/4] bpf: verifier: remove redundant opcode checks Shung-Hsi Yu
@ 2022-05-20 11:37 ` Shung-Hsi Yu
2022-05-21 0:27 ` Yonghong Song
0 siblings, 1 reply; 4+ messages in thread
From: Shung-Hsi Yu @ 2022-05-20 11:37 UTC (permalink / raw)
To: Shung-Hsi Yu, linux-kselftest, netdev, bpf, linux-kernel
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh
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 <shung-hsi.yu@suse.com>
---
.../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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64
2022-05-20 11:37 ` [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64 Shung-Hsi Yu
@ 2022-05-21 0:27 ` Yonghong Song
2022-05-24 4:49 ` Shung-Hsi Yu
0 siblings, 1 reply; 4+ messages in thread
From: Yonghong Song @ 2022-05-21 0:27 UTC (permalink / raw)
To: Shung-Hsi Yu, linux-kselftest, netdev, bpf, linux-kernel
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh
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 <shung-hsi.yu@suse.com>
> ---
> .../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),
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64
2022-05-21 0:27 ` Yonghong Song
@ 2022-05-24 4:49 ` Shung-Hsi Yu
0 siblings, 0 replies; 4+ messages in thread
From: Shung-Hsi Yu @ 2022-05-24 4:49 UTC (permalink / raw)
To: Yonghong Song
Cc: linux-kselftest, netdev, bpf, linux-kernel, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh
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 <shung-hsi.yu@suse.com>
> > ---
> > .../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),
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-24 4:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-20 11:37 [PATCH bpf-next 0/4] bpf: verifier: remove redundant opcode checks Shung-Hsi Yu
2022-05-20 11:37 ` [PATCH bpf-next 4/4] selftests/bpf: add reason of rejection in ld_imm64 Shung-Hsi Yu
2022-05-21 0:27 ` Yonghong Song
2022-05-24 4:49 ` Shung-Hsi Yu
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).