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

Subsystems: bpf jit for x86 64-bit, bpf [general] (safe dynamic programs and tools), the rest, x86 architecture (32-bit and 64-bit)

STALE1965d LANDED

Landed in mainline as b90829704780 on 2021-03-20.

8 messages, 4 authors, 2021-03-22 · open the first message on its own page

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

From: Stanislav Fomichev <hidden>
Date: 2021-03-20 00:01:07 

__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 <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 72b5a57e9e31..b35fc8023884 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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

Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

From: Alexei Starovoitov <hidden>
Date: 2021-03-20 00:14:59 

On Fri, Mar 19, 2021 at 5:00 PM Stanislav Fomichev [off-list ref] wrote:
quoted hunk
__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 <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 72b5a57e9e31..b35fc8023884 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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?

Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

From: Stanislav Fomichev <hidden>
Date: 2021-03-20 00:26:03 

On Fri, Mar 19, 2021 at 5:14 PM Alexei Starovoitov
[off-list ref] wrote:
On Fri, Mar 19, 2021 at 5:00 PM Stanislav Fomichev [off-list ref] wrote:
quoted
__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 <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 72b5a57e9e31..b35fc8023884 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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?
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.

Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

From: Alexei Starovoitov <hidden>
Date: 2021-03-20 00:34:21 

On Fri, Mar 19, 2021 at 5:25 PM Stanislav Fomichev [off-list ref] wrote:
On Fri, Mar 19, 2021 at 5:14 PM Alexei Starovoitov
[off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 5:00 PM Stanislav Fomichev [off-list ref] wrote:
quoted
__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 <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 72b5a57e9e31..b35fc8023884 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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?
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?

Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

From: Stanislav Fomichev <hidden>
Date: 2021-03-20 01:41:46 

On Fri, Mar 19, 2021 at 5:33 PM Alexei Starovoitov
[off-list ref] wrote:
On Fri, Mar 19, 2021 at 5:25 PM Stanislav Fomichev [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 5:14 PM Alexei Starovoitov
[off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 5:00 PM Stanislav Fomichev [off-list ref] wrote:
quoted
__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 <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 72b5a57e9e31..b35fc8023884 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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?
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;

Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

From: Alexei Starovoitov <hidden>
Date: 2021-03-20 02:32:53 

On Fri, Mar 19, 2021 at 6:40 PM Stanislav Fomichev [off-list ref] wrote:
On Fri, Mar 19, 2021 at 5:33 PM Alexei Starovoitov
[off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 5:25 PM Stanislav Fomichev [off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 5:14 PM Alexei Starovoitov
[off-list ref] wrote:
quoted
On Fri, Mar 19, 2021 at 5:00 PM Stanislav Fomichev [off-list ref] wrote:
quoted
__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 <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 72b5a57e9e31..b35fc8023884 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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?
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;
Ohh. Thanks for explaining. Applied.

Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

From: patchwork-bot+netdevbpf@kernel.org
Date: 2021-03-20 02:40:48 

Hello:

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

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

Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL_ORIG

From: Yauheni Kaliuta <hidden>
Date: 2021-03-22 06:40:37 

On Sat, Mar 20, 2021 at 2:01 AM Stanislav Fomichev [off-list ref] 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.

Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by: Stanislav Fomichev <redacted>
Could you mention x86 in the subject?
quoted hunk
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 72b5a57e9e31..b35fc8023884 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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

-- 
WBR, Yauheni
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help