bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

PATCH Applied Stanislav Fomichev 4 participants 8 messages Mar 20, 2021 – Mar 22, 2021

Subsystems (5)

Networking Network drivers x86 BPF +1 more subsystems

lore

Share & tools

lore

Stanislav Fomichev

author

2021-03-20 00:01 UTC #0 lore raw

PATCH 1/1 bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:

P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)

Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.

Fixes e21aa341785c ("bpf: Fix fexit trampoline.")

Signed-off-by Stanislav Fomichev <[email protected]>

---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/net/bpf_jit_comp.c +2 -1

index 72b5a57e9e31..b35fc8023884 100644
@@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		/* remember return value in a stack for bpf prog to access */
 		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
 		im->ip_after_call = prog;
-		emit_nops(&prog, 5);
+		memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
+		prog += X86_PATCH_SIZE;
 	}
 
 	if (fmod_ret->nr_progs) {

Signature · 1 line

2.31.0.rc2.261.g7f71774620-goog

Alexei Starovoitov

2021-03-20 00:14 UTC #1 lore raw

Quoted 21 lines · Stanislav Fomichev

__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:
P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.
Fixes e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by Stanislav Fomichev <[email protected]>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/net/bpf_jit_comp.c +0 -0
index 72b5a57e9e31..b35fc8023884 100644

arch/x86/net/bpf_jit_comp.c int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i · line 2016

Quoted 6 lines

arch/x86/net/bpf_jit_comp.c +2 -1

@@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
                /* remember return value in a stack for bpf prog to access */
                emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
                im->ip_after_call = prog;
-               emit_nops(&prog, 5);
+               memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
+               prog += X86_PATCH_SIZE;

I'm well aware, but ideal_nops are pretty much gone already. The changes are already in the -tip tree.
So I decided to reduce the conflicts for the merge window.

Do you actually see the breakage or it's purely theoretical?

Stanislav Fomichev

author

2021-03-20 00:26 UTC #2 lore raw

Quoted 34 lines · Alexei Starovoitov

Quoted 21 lines · Stanislav Fomichev
__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:
P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.
Fixes e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by Stanislav Fomichev <[email protected]>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/net/bpf_jit_comp.c +0 -0
index 72b5a57e9e31..b35fc8023884 100644
arch/x86/net/bpf_jit_comp.c int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i · line 2016
Quoted 6 lines
arch/x86/net/bpf_jit_comp.c +2 -1
@@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
                /* remember return value in a stack for bpf prog to access */
                emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
                im->ip_after_call = prog;
-               emit_nops(&prog, 5);
+               memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
+               prog += X86_PATCH_SIZE;
I'm well aware, but ideal_nops are pretty much gone already. The changes are already in the -tip tree.
So I decided to reduce the conflicts for the merge window.

Quoted 1 line

Do you actually see the breakage or it's purely theoretical?

We do see it, but it's on our tree that pulls from bpf.
And it obviously doesn't have that "x86: Remove dynamic NOP selection" yet. Thanks for the pointer, I guess I can just wait for the real merge then.

Alexei Starovoitov

2021-03-20 00:34 UTC #3 lore raw

Quoted 37 lines · Stanislav Fomichev

Quoted 34 lines · Alexei Starovoitov
Quoted 21 lines · Stanislav Fomichev
__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:
P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.
Fixes e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by Stanislav Fomichev <[email protected]>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/net/bpf_jit_comp.c +0 -0
index 72b5a57e9e31..b35fc8023884 100644
arch/x86/net/bpf_jit_comp.c int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i · line 2016
Quoted 6 lines
arch/x86/net/bpf_jit_comp.c +2 -1
@@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
                /* remember return value in a stack for bpf prog to access */
                emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
                im->ip_after_call = prog;
-               emit_nops(&prog, 5);
+               memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
+               prog += X86_PATCH_SIZE;
I'm well aware, but ideal_nops are pretty much gone already. The changes are already in the -tip tree.
So I decided to reduce the conflicts for the merge window.

Quoted 4 lines

Quoted 1 line
Do you actually see the breakage or it's purely theoretical?
We do see it, but it's on our tree that pulls from bpf.
And it obviously doesn't have that "x86: Remove dynamic NOP selection" yet. Thanks for the pointer, I guess I can just wait for the real merge then.

If it breaks the real users we have to land the fix, but let me ask how come that you run with k8 cpu? k8 does other nasty things. Do you run with all of amd errata?

Stanislav Fomichev

author

2021-03-20 01:41 UTC #4 lore raw

Quoted 39 lines · Alexei Starovoitov

Quoted 37 lines · Stanislav Fomichev
Quoted 34 lines · Alexei Starovoitov
Quoted 21 lines · Stanislav Fomichev
__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:
P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.
Fixes e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by Stanislav Fomichev <[email protected]>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/net/bpf_jit_comp.c +0 -0
index 72b5a57e9e31..b35fc8023884 100644
arch/x86/net/bpf_jit_comp.c int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i · line 2016
Quoted 6 lines
arch/x86/net/bpf_jit_comp.c +2 -1
@@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
                /* remember return value in a stack for bpf prog to access */
                emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
                im->ip_after_call = prog;
-               emit_nops(&prog, 5);
+               memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
+               prog += X86_PATCH_SIZE;
I'm well aware, but ideal_nops are pretty much gone already. The changes are already in the -tip tree.
So I decided to reduce the conflicts for the merge window.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Do you actually see the breakage or it's purely theoretical?
We do see it, but it's on our tree that pulls from bpf.
And it obviously doesn't have that "x86: Remove dynamic NOP selection" yet. Thanks for the pointer, I guess I can just wait for the real merge then.
If it breaks the real users we have to land the fix, but let me ask how come that you run with k8 cpu? k8 does other nasty things. Do you run with all of amd errata?

It's not amd, it's intel:

cpu family : 6
model : 45
model name : Intel(R) Xeon(R) CPU E5-2689 0 @ 2.60GHz

I think I'm hitting the following from the arch/x86/kernel/alternative.c:

/*
* Due to a decoder implementation quirk, some
* specific Intel CPUs actually perform better with
* the "k8_nops" than with the SDM-recommended NOPs. */
if (boot_cpu_data.x86 == 6 &&
boot_cpu_data.x86_model >= 0x0f && boot_cpu_data.x86_model != 0x1c && boot_cpu_data.x86_model != 0x26 && boot_cpu_data.x86_model != 0x27 && boot_cpu_data.x86_model < 0x30) {
ideal_nops = k8_nops;

Alexei Starovoitov

2021-03-20 02:32 UTC #5 lore raw

Quoted 58 lines · Stanislav Fomichev

Quoted 39 lines · Alexei Starovoitov
Quoted 37 lines · Stanislav Fomichev
Quoted 34 lines · Alexei Starovoitov
Quoted 21 lines · Stanislav Fomichev
__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:
P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.
Fixes e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by Stanislav Fomichev <[email protected]>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/net/bpf_jit_comp.c +0 -0
index 72b5a57e9e31..b35fc8023884 100644
arch/x86/net/bpf_jit_comp.c int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i · line 2016
Quoted 6 lines
arch/x86/net/bpf_jit_comp.c +2 -1
@@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
                /* remember return value in a stack for bpf prog to access */
                emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
                im->ip_after_call = prog;
-               emit_nops(&prog, 5);
+               memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
+               prog += X86_PATCH_SIZE;
I'm well aware, but ideal_nops are pretty much gone already. The changes are already in the -tip tree.
So I decided to reduce the conflicts for the merge window.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Do you actually see the breakage or it's purely theoretical?
We do see it, but it's on our tree that pulls from bpf.
And it obviously doesn't have that "x86: Remove dynamic NOP selection" yet. Thanks for the pointer, I guess I can just wait for the real merge then.
If it breaks the real users we have to land the fix, but let me ask how come that you run with k8 cpu? k8 does other nasty things. Do you run with all of amd errata?
It's not amd, it's intel:
cpu family : 6
model : 45
model name : Intel(R) Xeon(R) CPU E5-2689 0 @ 2.60GHz
I think I'm hitting the following from the arch/x86/kernel/alternative.c:

Quoted 12 lines

/*
* Due to a decoder implementation quirk, some
* specific Intel CPUs actually perform better with
* the "k8_nops" than with the SDM-recommended NOPs. */
if (boot_cpu_data.x86 == 6 &&
boot_cpu_data.x86_model >= 0x0f && boot_cpu_data.x86_model != 0x1c && boot_cpu_data.x86_model != 0x26 && boot_cpu_data.x86_model != 0x27 && boot_cpu_data.x86_model < 0x30) {
ideal_nops = k8_nops;

Ohh. Thanks for explaining. Applied.

[email protected]

CI Applied

2021-03-20 02:40 UTC #6 lore raw

In reply to Stanislav Fomichev

This patch was applied to bpf/bpf.git (refs/heads/master):

Hello:

This patch was applied to bpf/bpf.git (refs/heads/master):

Quoted 10 lines · On Fri, 19 Mar 2021 17:00:01 -0700 you wrote:

__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:
P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.
[...]

Here is the summary with links:
- [bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

    https://git.kernel.org/bpf/bpf/c/b90829704780

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Yauheni Kaliuta

2021-03-22 06:40 UTC #7 lore raw

In reply to Stanislav Fomichev

Quoted 4 lines · Stanislav Fomichev

__bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5) emits non-atomic one which breaks fentry/fexit with k8 atomics:

Quoted 8 lines

P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000) K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops() and running fexit_bpf2bpf selftest.
Fixes e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by Stanislav Fomichev <[email protected]>

Could you mention x86 in the subject?

Quoted 21 lines

---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/net/bpf_jit_comp.c +2 -2

index 72b5a57e9e31..b35fc8023884 100644
@@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
                /* remember return value in a stack for bpf prog to access */
                emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
                im->ip_after_call = prog;
-               emit_nops(&prog, 5);
+               memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
+               prog += X86_PATCH_SIZE;
        }
 
        if (fmod_ret->nr_progs) {
--

2.31.0.rc2.261.g7f71774620-goog

Signature · 1 line

WBR, Yauheni