BPF List
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/3] bpf: Allow 'may_goto 0' instruction
@ 2025-01-18 19:20 Yonghong Song
  2025-01-18 19:20 ` [PATCH bpf-next v2 1/3] bpf: Allow 'may_goto 0' instruction in verifier Yonghong Song
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Yonghong Song @ 2025-01-18 19:20 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
	Martin KaFai Lau

Emil Tsalapatis from Meta reported such a case where 'may_goto 0' insn is
generated by clang-19 compiler and this caused verification failure
since 'may_goto 0' is rejected by verifier.

In fact, 'may_goto 0' insn is actually a no-op and it won't hurt
verification. The only side effect is that the verifier will convert
the insn to a sequence of codes like
   /* r10 - 8 stores the implicit loop count */
   r11 = *(u64 *)(r10 -8)
   if r11 == 0x0 goto pc+2
   r11 -= 1
   *(u64 *)(r10 -8) = r11

With this patch set 'may_goto 0' insns are allowed in verification which
also removes those insns.

Changelogs:
  v1 -> v2:
    - Instead of a separate function, removing 'may_goto 0' in existing
      func opt_remove_nops().

Yonghong Song (3):
  bpf: Allow 'may_goto 0' instruction in verifier
  bpf: Remove 'may_goto 0' instruction in opt_remove_nops()
  selftests/bpf: Add some tests related to 'may_goto 0' insns

 kernel/bpf/verifier.c                         | 14 ++-
 .../selftests/bpf/prog_tests/verifier.c       |  4 +
 .../selftests/bpf/progs/verifier_may_goto_1.c | 97 +++++++++++++++++++
 .../selftests/bpf/progs/verifier_may_goto_2.c | 28 ++++++
 4 files changed, 138 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_may_goto_1.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_may_goto_2.c

-- 
2.43.5


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH bpf-next v2 1/3] bpf: Allow 'may_goto 0' instruction in verifier
  2025-01-18 19:20 [PATCH bpf-next v2 0/3] bpf: Allow 'may_goto 0' instruction Yonghong Song
@ 2025-01-18 19:20 ` Yonghong Song
  2025-01-20 15:20   ` Daniel Borkmann
  2025-01-18 19:20 ` [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops() Yonghong Song
  2025-01-18 19:20 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add some tests related to 'may_goto 0' insns Yonghong Song
  2 siblings, 1 reply; 8+ messages in thread
From: Yonghong Song @ 2025-01-18 19:20 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
	Martin KaFai Lau, Emil Tsalapatis, Eduard Zingerman

Commit 011832b97b31 ("bpf: Introduce may_goto instruction") added support
for may_goto insn. The 'may_goto 0' insn is disallowed since the insn is
equivalent to a nop as both branch will go to the next insn.

But it is possible that compiler transformation may generate 'may_goto 0'
insn. Emil Tsalapatis from Meta reported such a case which caused
verification failure. For example, for the following code,
   int i, tmp[3];
   for (i = 0; i < 3 && can_loop; i++)
     tmp[i] = 0;
   ...

clang 20 may generate code like
   may_goto 2;
   may_goto 1;
   may_goto 0;
   r1 = 0; /* tmp[0] = 0; */
   r2 = 0; /* tmp[1] = 0; */
   r3 = 0; /* tmp[2] = 0; */

Let us permit 'may_goto 0' insn to avoid verification failure for codes
like the above.

Reported-by: Emil Tsalapatis <etsal@meta.com>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/verifier.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 245f1f3f1aec..963dfda81c06 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15972,9 +15972,8 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
 
 		if (insn->code != (BPF_JMP | BPF_JCOND) ||
 		    insn->src_reg != BPF_MAY_GOTO ||
-		    insn->dst_reg || insn->imm || insn->off == 0) {
-			verbose(env, "invalid may_goto off %d imm %d\n",
-				insn->off, insn->imm);
+		    insn->dst_reg || insn->imm) {
+			verbose(env, "invalid may_goto imm %d\n", insn->imm);
 			return -EINVAL;
 		}
 		prev_st = find_prev_entry(env, cur_st->parent, idx);
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops()
  2025-01-18 19:20 [PATCH bpf-next v2 0/3] bpf: Allow 'may_goto 0' instruction Yonghong Song
  2025-01-18 19:20 ` [PATCH bpf-next v2 1/3] bpf: Allow 'may_goto 0' instruction in verifier Yonghong Song
@ 2025-01-18 19:20 ` Yonghong Song
  2025-01-20 15:29   ` Daniel Borkmann
  2025-01-18 19:20 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add some tests related to 'may_goto 0' insns Yonghong Song
  2 siblings, 1 reply; 8+ messages in thread
From: Yonghong Song @ 2025-01-18 19:20 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
	Martin KaFai Lau

Since 'may_goto 0' insns are actually no-op, let us remove them.
Otherwise, verifier will generate code like
   /* r10 - 8 stores the implicit loop count */
   r11 = *(u64 *)(r10 -8)
   if r11 == 0x0 goto pc+2
   r11 -= 1
   *(u64 *)(r10 -8) = r11

which is the pure overhead.

The following code patterns (from the previous commit) are also
handled:
   may_goto 2
   may_goto 1
   may_goto 0

With this commit, the above three 'may_goto' insns are all
eliminated.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/verifier.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 963dfda81c06..784547aa40a8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20187,20 +20187,25 @@ static const struct bpf_insn NOP = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
 
 static int opt_remove_nops(struct bpf_verifier_env *env)
 {
+	const struct bpf_insn may_goto_0 = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0, 0);
 	const struct bpf_insn ja = NOP;
 	struct bpf_insn *insn = env->prog->insnsi;
 	int insn_cnt = env->prog->len;
+	bool is_may_goto_0, is_ja;
 	int i, err;
 
 	for (i = 0; i < insn_cnt; i++) {
-		if (memcmp(&insn[i], &ja, sizeof(ja)))
+		is_may_goto_0 = !memcmp(&insn[i], &may_goto_0, sizeof(may_goto_0));
+		is_ja = !memcmp(&insn[i], &ja, sizeof(ja));
+
+		if (!is_may_goto_0 && !is_ja)
 			continue;
 
 		err = verifier_remove_insns(env, i, 1);
 		if (err)
 			return err;
 		insn_cnt--;
-		i--;
+		i -= (is_may_goto_0 && i > 0) ? 2 : 1;
 	}
 
 	return 0;
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH bpf-next v2 3/3] selftests/bpf: Add some tests related to 'may_goto 0' insns
  2025-01-18 19:20 [PATCH bpf-next v2 0/3] bpf: Allow 'may_goto 0' instruction Yonghong Song
  2025-01-18 19:20 ` [PATCH bpf-next v2 1/3] bpf: Allow 'may_goto 0' instruction in verifier Yonghong Song
  2025-01-18 19:20 ` [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops() Yonghong Song
@ 2025-01-18 19:20 ` Yonghong Song
  2 siblings, 0 replies; 8+ messages in thread
From: Yonghong Song @ 2025-01-18 19:20 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
	Martin KaFai Lau, Eduard Zingerman

Add both asm-based and C-based tests which have 'may_goto 0' insns.

For the following code in C-based test,
   int i, tmp[3];
   for (i = 0; i < 3 && can_loop; i++)
       tmp[i] = 0;

The clang compiler (clang 19 and 20) generates
   may_goto 2
   may_goto 1
   may_goto 0
   r1 = 0
   r2 = 0
   r3 = 0

The above asm codes are due to llvm pass SROAPass. This ensures the
successful verification since tmp[0-2] are initialized.  Otherwise,
the code without SROAPass like
   may_goto 5
   r1 = 0
   may_goto 3
   r2 = 0
   may_goto 1
   r3 = 0
will have verification failure.

Although from the source code C-based test should have verification
failure, clang compiler optimization generates code with successful
verification. If gcc generates different asm codes than clang, the
following code can be used for gcc:
   int i, tmp[3];
   for (i = 0; i < 3; i++)
       tmp[i] = 0;

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../selftests/bpf/prog_tests/verifier.c       |  4 +
 .../selftests/bpf/progs/verifier_may_goto_1.c | 97 +++++++++++++++++++
 .../selftests/bpf/progs/verifier_may_goto_2.c | 28 ++++++
 3 files changed, 129 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_may_goto_1.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_may_goto_2.c

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 33cd3e035071..8a0e1ff8a2dc 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -52,6 +52,8 @@
 #include "verifier_map_ptr_mixing.skel.h"
 #include "verifier_map_ret_val.skel.h"
 #include "verifier_masking.skel.h"
+#include "verifier_may_goto_1.skel.h"
+#include "verifier_may_goto_2.skel.h"
 #include "verifier_meta_access.skel.h"
 #include "verifier_movsx.skel.h"
 #include "verifier_mtu.skel.h"
@@ -182,6 +184,8 @@ void test_verifier_map_ptr(void)              { RUN(verifier_map_ptr); }
 void test_verifier_map_ptr_mixing(void)       { RUN(verifier_map_ptr_mixing); }
 void test_verifier_map_ret_val(void)          { RUN(verifier_map_ret_val); }
 void test_verifier_masking(void)              { RUN(verifier_masking); }
+void test_verifier_may_goto_1(void)           { RUN(verifier_may_goto_1); }
+void test_verifier_may_goto_2(void)           { RUN(verifier_may_goto_2); }
 void test_verifier_meta_access(void)          { RUN(verifier_meta_access); }
 void test_verifier_movsx(void)                 { RUN(verifier_movsx); }
 void test_verifier_netfilter_ctx(void)        { RUN(verifier_netfilter_ctx); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_may_goto_1.c b/tools/testing/selftests/bpf/progs/verifier_may_goto_1.c
new file mode 100644
index 000000000000..e81097c96fe2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_may_goto_1.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "../../../include/linux/filter.h"
+#include "bpf_misc.h"
+
+SEC("raw_tp")
+__description("may_goto 0")
+__arch_x86_64
+__xlated("0: r0 = 1")
+__xlated("1: exit")
+__success
+__naked void may_goto_simple(void)
+{
+	asm volatile (
+	".8byte %[may_goto];"
+	"r0 = 1;"
+	".8byte %[may_goto];"
+	"exit;"
+	:
+	: __imm_insn(may_goto, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0 /* offset */, 0))
+	: __clobber_all);
+}
+
+SEC("raw_tp")
+__description("batch 2 of may_goto 0")
+__arch_x86_64
+__xlated("0: r0 = 1")
+__xlated("1: exit")
+__success
+__naked void may_goto_batch_0(void)
+{
+	asm volatile (
+	".8byte %[may_goto1];"
+	".8byte %[may_goto1];"
+	"r0 = 1;"
+	".8byte %[may_goto1];"
+	".8byte %[may_goto1];"
+	"exit;"
+	:
+	: __imm_insn(may_goto1, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0 /* offset */, 0))
+	: __clobber_all);
+}
+
+SEC("raw_tp")
+__description("may_goto batch with offsets 2/1/0")
+__arch_x86_64
+__xlated("0: r0 = 1")
+__xlated("1: exit")
+__success
+__naked void may_goto_batch_1(void)
+{
+	asm volatile (
+	".8byte %[may_goto1];"
+	".8byte %[may_goto2];"
+	".8byte %[may_goto3];"
+	"r0 = 1;"
+	".8byte %[may_goto1];"
+	".8byte %[may_goto2];"
+	".8byte %[may_goto3];"
+	"exit;"
+	:
+	: __imm_insn(may_goto1, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 2 /* offset */, 0)),
+	  __imm_insn(may_goto2, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 1 /* offset */, 0)),
+	  __imm_insn(may_goto3, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0 /* offset */, 0))
+	: __clobber_all);
+}
+
+SEC("raw_tp")
+__description("may_goto batch with offsets 2/0")
+__arch_x86_64
+__xlated("0: *(u64 *)(r10 -8) = 8388608")
+__xlated("1: r11 = *(u64 *)(r10 -8)")
+__xlated("2: if r11 == 0x0 goto pc+3")
+__xlated("3: r11 -= 1")
+__xlated("4: *(u64 *)(r10 -8) = r11")
+__xlated("5: r0 = 1")
+__xlated("6: r0 = 2")
+__xlated("7: exit")
+__success
+__naked void may_goto_batch_2(void)
+{
+	asm volatile (
+	".8byte %[may_goto1];"
+	".8byte %[may_goto3];"
+	"r0 = 1;"
+	"r0 = 2;"
+	"exit;"
+	:
+	: __imm_insn(may_goto1, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 2 /* offset */, 0)),
+	  __imm_insn(may_goto3, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0 /* offset */, 0))
+	: __clobber_all);
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_may_goto_2.c b/tools/testing/selftests/bpf/progs/verifier_may_goto_2.c
new file mode 100644
index 000000000000..b891faf50660
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_may_goto_2.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+int gvar;
+
+SEC("raw_tp")
+__description("C code with may_goto 0")
+__success
+int may_goto_c_code(void)
+{
+	int i, tmp[3];
+
+	for (i = 0; i < 3 && can_loop; i++)
+		tmp[i] = 0;
+
+	for (i = 0; i < 3 && can_loop; i++)
+		tmp[i] = gvar - i;
+
+	for (i = 0; i < 3 && can_loop; i++)
+		gvar += tmp[i];
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Allow 'may_goto 0' instruction in verifier
  2025-01-18 19:20 ` [PATCH bpf-next v2 1/3] bpf: Allow 'may_goto 0' instruction in verifier Yonghong Song
@ 2025-01-20 15:20   ` Daniel Borkmann
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2025-01-20 15:20 UTC (permalink / raw)
  To: Yonghong Song, bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, kernel-team,
	Martin KaFai Lau, Emil Tsalapatis, Eduard Zingerman

On 1/18/25 8:20 PM, Yonghong Song wrote:
> Commit 011832b97b31 ("bpf: Introduce may_goto instruction") added support
> for may_goto insn. The 'may_goto 0' insn is disallowed since the insn is
> equivalent to a nop as both branch will go to the next insn.
> 
> But it is possible that compiler transformation may generate 'may_goto 0'
> insn. Emil Tsalapatis from Meta reported such a case which caused
> verification failure. For example, for the following code,
>     int i, tmp[3];
>     for (i = 0; i < 3 && can_loop; i++)
>       tmp[i] = 0;
>     ...
> 
> clang 20 may generate code like
>     may_goto 2;
>     may_goto 1;
>     may_goto 0;
>     r1 = 0; /* tmp[0] = 0; */
>     r2 = 0; /* tmp[1] = 0; */
>     r3 = 0; /* tmp[2] = 0; */
> 
> Let us permit 'may_goto 0' insn to avoid verification failure for codes
> like the above.
> 
> Reported-by: Emil Tsalapatis <etsal@meta.com>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops()
  2025-01-18 19:20 ` [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops() Yonghong Song
@ 2025-01-20 15:29   ` Daniel Borkmann
  2025-01-20 17:49     ` Alexei Starovoitov
  2025-01-21  3:20     ` Yonghong Song
  0 siblings, 2 replies; 8+ messages in thread
From: Daniel Borkmann @ 2025-01-20 15:29 UTC (permalink / raw)
  To: Yonghong Song, bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, kernel-team,
	Martin KaFai Lau

On 1/18/25 8:20 PM, Yonghong Song wrote:
> Since 'may_goto 0' insns are actually no-op, let us remove them.
> Otherwise, verifier will generate code like
>     /* r10 - 8 stores the implicit loop count */
>     r11 = *(u64 *)(r10 -8)
>     if r11 == 0x0 goto pc+2
>     r11 -= 1
>     *(u64 *)(r10 -8) = r11
> 
> which is the pure overhead.
> 
> The following code patterns (from the previous commit) are also
> handled:
>     may_goto 2
>     may_goto 1
>     may_goto 0
> 
> With this commit, the above three 'may_goto' insns are all
> eliminated.
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>   kernel/bpf/verifier.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 963dfda81c06..784547aa40a8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20187,20 +20187,25 @@ static const struct bpf_insn NOP = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
>   
>   static int opt_remove_nops(struct bpf_verifier_env *env)
>   {
> +	const struct bpf_insn may_goto_0 = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0, 0);
>   	const struct bpf_insn ja = NOP;
>   	struct bpf_insn *insn = env->prog->insnsi;
>   	int insn_cnt = env->prog->len;
> +	bool is_may_goto_0, is_ja;
>   	int i, err;
>   
>   	for (i = 0; i < insn_cnt; i++) {
> -		if (memcmp(&insn[i], &ja, sizeof(ja)))
> +		is_may_goto_0 = !memcmp(&insn[i], &may_goto_0, sizeof(may_goto_0));
> +		is_ja = !memcmp(&insn[i], &ja, sizeof(ja));
> +
> +		if (!is_may_goto_0 && !is_ja)
>   			continue;

Why the extra may_goto_0 stack var?

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 245f1f3f1aec..16ba26295ec7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20185,16 +20185,19 @@ static int opt_remove_dead_code(struct bpf_verifier_env *env)
  }
  
  static const struct bpf_insn NOP = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
+static const struct bpf_insn MAY_GOTO_0 = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0, 0);
  
  static int opt_remove_nops(struct bpf_verifier_env *env)
  {
-       const struct bpf_insn ja = NOP;
         struct bpf_insn *insn = env->prog->insnsi;
         int insn_cnt = env->prog->len;
+       bool is_ja, is_may_goto_0;
         int i, err;
  
         for (i = 0; i < insn_cnt; i++) {
-               if (memcmp(&insn[i], &ja, sizeof(ja)))
+               is_may_goto_0 = !memcmp(&insn[i], &MAY_GOTO_0, sizeof(MAY_GOTO_0));
+               is_ja         = !memcmp(&insn[i], &NOP, sizeof(NOP));
+               if (!is_may_goto_0 && !is_ja)
                         continue;

>   		err = verifier_remove_insns(env, i, 1);
>   		if (err)
>   			return err;
>   		insn_cnt--;
> -		i--;
> +		i -= (is_may_goto_0 && i > 0) ? 2 : 1;

Maybe add a comment for this logic?

Thanks,
Daniel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops()
  2025-01-20 15:29   ` Daniel Borkmann
@ 2025-01-20 17:49     ` Alexei Starovoitov
  2025-01-21  3:20     ` Yonghong Song
  1 sibling, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2025-01-20 17:49 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Yonghong Song, bpf, Alexei Starovoitov, Andrii Nakryiko,
	Kernel Team, Martin KaFai Lau

On Mon, Jan 20, 2025 at 7:29 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/18/25 8:20 PM, Yonghong Song wrote:
> > Since 'may_goto 0' insns are actually no-op, let us remove them.
> > Otherwise, verifier will generate code like
> >     /* r10 - 8 stores the implicit loop count */
> >     r11 = *(u64 *)(r10 -8)
> >     if r11 == 0x0 goto pc+2
> >     r11 -= 1
> >     *(u64 *)(r10 -8) = r11
> >
> > which is the pure overhead.
> >
> > The following code patterns (from the previous commit) are also
> > handled:
> >     may_goto 2
> >     may_goto 1
> >     may_goto 0
> >
> > With this commit, the above three 'may_goto' insns are all
> > eliminated.
> >
> > Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> > ---
> >   kernel/bpf/verifier.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 963dfda81c06..784547aa40a8 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -20187,20 +20187,25 @@ static const struct bpf_insn NOP = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> >
> >   static int opt_remove_nops(struct bpf_verifier_env *env)
> >   {
> > +     const struct bpf_insn may_goto_0 = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0, 0);
> >       const struct bpf_insn ja = NOP;
> >       struct bpf_insn *insn = env->prog->insnsi;
> >       int insn_cnt = env->prog->len;
> > +     bool is_may_goto_0, is_ja;
> >       int i, err;
> >
> >       for (i = 0; i < insn_cnt; i++) {
> > -             if (memcmp(&insn[i], &ja, sizeof(ja)))
> > +             is_may_goto_0 = !memcmp(&insn[i], &may_goto_0, sizeof(may_goto_0));
> > +             is_ja = !memcmp(&insn[i], &ja, sizeof(ja));
> > +
> > +             if (!is_may_goto_0 && !is_ja)
> >                       continue;
>
> Why the extra may_goto_0 stack var?
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 245f1f3f1aec..16ba26295ec7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20185,16 +20185,19 @@ static int opt_remove_dead_code(struct bpf_verifier_env *env)
>   }
>
>   static const struct bpf_insn NOP = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> +static const struct bpf_insn MAY_GOTO_0 = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 0, 0);
>
>   static int opt_remove_nops(struct bpf_verifier_env *env)
>   {
> -       const struct bpf_insn ja = NOP;
>          struct bpf_insn *insn = env->prog->insnsi;
>          int insn_cnt = env->prog->len;
> +       bool is_ja, is_may_goto_0;
>          int i, err;
>
>          for (i = 0; i < insn_cnt; i++) {
> -               if (memcmp(&insn[i], &ja, sizeof(ja)))
> +               is_may_goto_0 = !memcmp(&insn[i], &MAY_GOTO_0, sizeof(MAY_GOTO_0));
> +               is_ja         = !memcmp(&insn[i], &NOP, sizeof(NOP));
> +               if (!is_may_goto_0 && !is_ja)
>                          continue;
>
> >               err = verifier_remove_insns(env, i, 1);
> >               if (err)
> >                       return err;
> >               insn_cnt--;
> > -             i--;
> > +             i -= (is_may_goto_0 && i > 0) ? 2 : 1;
>
> Maybe add a comment for this logic?

Adjusted both while applying.
Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops()
  2025-01-20 15:29   ` Daniel Borkmann
  2025-01-20 17:49     ` Alexei Starovoitov
@ 2025-01-21  3:20     ` Yonghong Song
  1 sibling, 0 replies; 8+ messages in thread
From: Yonghong Song @ 2025-01-21  3:20 UTC (permalink / raw)
  To: Daniel Borkmann, bpf
  Cc: Alexei Starovoitov, Andrii Nakryiko, kernel-team,
	Martin KaFai Lau




On 1/20/25 7:29 AM, Daniel Borkmann wrote:
> On 1/18/25 8:20 PM, Yonghong Song wrote:
>> Since 'may_goto 0' insns are actually no-op, let us remove them.
>> Otherwise, verifier will generate code like
>>     /* r10 - 8 stores the implicit loop count */
>>     r11 = *(u64 *)(r10 -8)
>>     if r11 == 0x0 goto pc+2
>>     r11 -= 1
>>     *(u64 *)(r10 -8) = r11
>>
>> which is the pure overhead.
>>
>> The following code patterns (from the previous commit) are also
>> handled:
>>     may_goto 2
>>     may_goto 1
>>     may_goto 0
>>
>> With this commit, the above three 'may_goto' insns are all
>> eliminated.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   kernel/bpf/verifier.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 963dfda81c06..784547aa40a8 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -20187,20 +20187,25 @@ static const struct bpf_insn NOP = 
>> BPF_JMP_IMM(BPF_JA, 0, 0, 0);
>>     static int opt_remove_nops(struct bpf_verifier_env *env)
>>   {
>> +    const struct bpf_insn may_goto_0 = BPF_RAW_INSN(BPF_JMP | 
>> BPF_JCOND, 0, 0, 0, 0);
>>       const struct bpf_insn ja = NOP;
>>       struct bpf_insn *insn = env->prog->insnsi;
>>       int insn_cnt = env->prog->len;
>> +    bool is_may_goto_0, is_ja;
>>       int i, err;
>>         for (i = 0; i < insn_cnt; i++) {
>> -        if (memcmp(&insn[i], &ja, sizeof(ja)))
>> +        is_may_goto_0 = !memcmp(&insn[i], &may_goto_0, 
>> sizeof(may_goto_0));
>> +        is_ja = !memcmp(&insn[i], &ja, sizeof(ja));
>> +
>> +        if (!is_may_goto_0 && !is_ja)
>>               continue;
>
> Why the extra may_goto_0 stack var?
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 245f1f3f1aec..16ba26295ec7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20185,16 +20185,19 @@ static int opt_remove_dead_code(struct 
> bpf_verifier_env *env)
>  }
>
>  static const struct bpf_insn NOP = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> +static const struct bpf_insn MAY_GOTO_0 = BPF_RAW_INSN(BPF_JMP | 
> BPF_JCOND, 0, 0, 0, 0);

This actually is what I did initially. I changed to use the stack var because
NOP is used in other functions too while MAY_GOTO_0 is only used in
opt_remove_nops(). Certainly, using MAY_GOTO_0 as static variable works too.

>
>  static int opt_remove_nops(struct bpf_verifier_env *env)
>  {
> -       const struct bpf_insn ja = NOP;
>         struct bpf_insn *insn = env->prog->insnsi;
>         int insn_cnt = env->prog->len;
> +       bool is_ja, is_may_goto_0;
>         int i, err;
>
>         for (i = 0; i < insn_cnt; i++) {
> -               if (memcmp(&insn[i], &ja, sizeof(ja)))
> +               is_may_goto_0 = !memcmp(&insn[i], &MAY_GOTO_0, 
> sizeof(MAY_GOTO_0));
> +               is_ja         = !memcmp(&insn[i], &NOP, sizeof(NOP));
> +               if (!is_may_goto_0 && !is_ja)
>                         continue;
>
>>           err = verifier_remove_insns(env, i, 1);
>>           if (err)
>>               return err;
>>           insn_cnt--;
>> -        i--;
>> +        i -= (is_may_goto_0 && i > 0) ? 2 : 1;
>
> Maybe add a comment for this logic?

Thanks Alexei for adding comments before merging!

>
> Thanks,
> Daniel


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-01-21  3:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-18 19:20 [PATCH bpf-next v2 0/3] bpf: Allow 'may_goto 0' instruction Yonghong Song
2025-01-18 19:20 ` [PATCH bpf-next v2 1/3] bpf: Allow 'may_goto 0' instruction in verifier Yonghong Song
2025-01-20 15:20   ` Daniel Borkmann
2025-01-18 19:20 ` [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops() Yonghong Song
2025-01-20 15:29   ` Daniel Borkmann
2025-01-20 17:49     ` Alexei Starovoitov
2025-01-21  3:20     ` Yonghong Song
2025-01-18 19:20 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add some tests related to 'may_goto 0' insns Yonghong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox