Re: [PATCH bpf-next v2 2/3] bpf: Remove 'may_goto 0' instruction in opt_remove_nops()

[Daniel Borkmann <daniel@xxxxxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>   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