Re: [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs

From: KP Singh <kpsingh@google.com>
To: Alexei Starovoitov <ast@kernel.org>
Cc: davem@davemloft.net, Daniel Borkmann <daniel@iogearbox.net>,
	Jann Horn <jannh@google.com>,
	netdev@vger.kernel.org, bpf@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs
Date: Wed, 11 Mar 2020 02:15:43 +0100	[thread overview]
Message-ID: <CAFLU3Ktok8WnjjPTuSOgecKFcz+ZsrUOzCKpjc2btb-UwapFwQ@mail.gmail.com> (raw)
In-Reply-To: <20200311003906.3643037-1-ast@kernel.org>

Good catch and thanks for fixing this!

On Wed, Mar 11, 2020 at 1:39 AM Alexei Starovoitov <ast@kernel.org> wrote:
>
> fmod_ret progs are emitted as:
>
> start = __bpf_prog_enter();
> call fmod_ret
> *(u64 *)(rbp - 8) = rax
> __bpf_prog_exit(, start);
> test eax, eax
> jne do_fexit
>
> That 'test eax, eax' is working by accident. The compiler is free to use rax
> inside __bpf_prog_exit() or inside functions that __bpf_prog_exit() is calling.
> Which caused "test_progs -t modify_return" to sporadically fail depending on
> compiler version and kconfig. Fix it by using 'cmp [rbp - 8], 0' instead of
> 'test eax, eax'.
>
> Fixes: ae24082331d9 ("bpf: Introduce BPF_MODIFY_RETURN")
> Reported-by: Andrii Nakryiko <andriin@fb.com>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 31 +++++--------------------------
>  1 file changed, 5 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index b1fd000feb89..5ea7c2cf7ab4 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1449,23 +1449,6 @@ static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
>         return 0;
>  }
>
> -static int emit_mod_ret_check_imm8(u8 **pprog, int value)
> -{
> -       u8 *prog = *pprog;
> -       int cnt = 0;
> -
> -       if (!is_imm8(value))
> -               return -EINVAL;
> -
> -       if (value == 0)
> -               EMIT2(0x85, add_2reg(0xC0, BPF_REG_0, BPF_REG_0));
> -       else
> -               EMIT3(0x83, add_1reg(0xF8, BPF_REG_0), value);
> -
> -       *pprog = prog;
> -       return 0;
> -}
> -
>  static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
>                       struct bpf_tramp_progs *tp, int stack_size)
>  {
> @@ -1485,7 +1468,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>                               u8 **branches)
>  {
>         u8 *prog = *pprog;
> -       int i;
> +       int i, cnt = 0;
>
>         /* The first fmod_ret program will receive a garbage return value.
>          * Set this to 0 to avoid confusing the program.
> @@ -1496,16 +1479,12 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>                 if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size, true))
>                         return -EINVAL;
>
> -               /* Generate a branch:
> -                *
> -                * if (ret !=  0)
> +               /* mod_ret prog stored return value into [rbp - 8]. Emit:
> +                * if (*(u64 *)(rbp - 8) !=  0)
>                  *      goto do_fexit;
> -                *
> -                * If needed this can be extended to any integer value which can
> -                * be passed by user-space when the program is loaded.
>                  */
> -               if (emit_mod_ret_check_imm8(&prog, 0))
> -                       return -EINVAL;
> +               /* cmp QWORD PTR [rbp - 0x8], 0x0 */
> +               EMIT4(0x48, 0x83, 0x7d, 0xf8); EMIT1(0x00);
>
>                 /* Save the location of the branch and Generate 6 nops
>                  * (4 bytes for an offset and 2 bytes for the jump) These nops
> --
> 2.23.0
>

Acked-by: KP Singh <kpsingh@google.com>