* [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs
@ 2020-03-11 0:39 Alexei Starovoitov
2020-03-11 1:15 ` KP Singh
2020-03-11 13:23 ` Daniel Borkmann
0 siblings, 2 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2020-03-11 0:39 UTC (permalink / raw)
To: davem; +Cc: daniel, kpsingh, jannh, netdev, bpf, kernel-team
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs
2020-03-11 0:39 [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs Alexei Starovoitov
@ 2020-03-11 1:15 ` KP Singh
2020-03-11 13:23 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: KP Singh @ 2020-03-11 1:15 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, Daniel Borkmann, Jann Horn, netdev, bpf, kernel-team
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs
2020-03-11 0:39 [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs Alexei Starovoitov
2020-03-11 1:15 ` KP Singh
@ 2020-03-11 13:23 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Borkmann @ 2020-03-11 13:23 UTC (permalink / raw)
To: Alexei Starovoitov, davem; +Cc: kpsingh, jannh, netdev, bpf, kernel-team
On 3/11/20 1:39 AM, Alexei Starovoitov 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>
Applied, thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-11 13:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 0:39 [PATCH bpf-next] bpf: Fix trampoline generation for fmod_ret programs Alexei Starovoitov
2020-03-11 1:15 ` KP Singh
2020-03-11 13:23 ` Daniel Borkmann
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).