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

[Yonghong Song <yonghong.song@linux.dev>]

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