* [PATCH bpf-next] bpf: x86: Fix BPF_FETCH atomic and/or/xor with r0 as src
@ 2021-02-15 16:00 Brendan Jackman
2021-02-15 21:09 ` KP Singh
0 siblings, 1 reply; 4+ messages in thread
From: Brendan Jackman @ 2021-02-15 16:00 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, KP Singh,
Florent Revest, Ilya Leoshkevich, Brendan Jackman
This code generates a CMPXCHG loop in order to implement atomic_fetch
bitwise operations. Because CMPXCHG is hard-coded to use rax (which
holds the BPF r0 value), it saves the _real_ r0 value into the
internal "ax" temporary register and restores it once the loop is
complete.
In the middle of the loop, the actual bitwise operation is performed
using src_reg. The bug occurs when src_reg is r0: as described above,
r0 has been clobbered and the real r0 value is in the ax register.
Therefore, perform this operation on the ax register instead, when
src_reg is r0.
Fixes: 981f94c3e921 ("bpf: Add bitwise atomic instructions")
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
arch/x86/net/bpf_jit_comp.c | 7 +++---
.../selftests/bpf/verifier/atomic_and.c | 23 +++++++++++++++++++
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79e7a0ec1da5..0c9edfe42340 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1349,6 +1349,7 @@ st: if (is_imm8(insn->off))
insn->imm == (BPF_XOR | BPF_FETCH)) {
u8 *branch_target;
bool is64 = BPF_SIZE(insn->code) == BPF_DW;
+ u32 real_src_reg = src_reg == BPF_REG_0 ? BPF_REG_AX : src_reg;
/*
* Can't be implemented with a single x86 insn.
@@ -1366,9 +1367,9 @@ st: if (is_imm8(insn->off))
* put the result in the AUX_REG.
*/
emit_mov_reg(&prog, is64, AUX_REG, BPF_REG_0);
- maybe_emit_mod(&prog, AUX_REG, src_reg, is64);
+ maybe_emit_mod(&prog, AUX_REG, real_src_reg, is64);
EMIT2(simple_alu_opcodes[BPF_OP(insn->imm)],
- add_2reg(0xC0, AUX_REG, src_reg));
+ add_2reg(0xC0, AUX_REG, real_src_reg));
/* Attempt to swap in new value */
err = emit_atomic(&prog, BPF_CMPXCHG,
dst_reg, AUX_REG, insn->off,
@@ -1381,7 +1382,7 @@ st: if (is_imm8(insn->off))
*/
EMIT2(X86_JNE, -(prog - branch_target) - 2);
/* Return the pre-modification value */
- emit_mov_reg(&prog, is64, src_reg, BPF_REG_0);
+ emit_mov_reg(&prog, is64, real_src_reg, BPF_REG_0);
/* Restore R0 after clobbering RAX */
emit_mov_reg(&prog, true, BPF_REG_0, BPF_REG_AX);
break;
diff --git a/tools/testing/selftests/bpf/verifier/atomic_and.c b/tools/testing/selftests/bpf/verifier/atomic_and.c
index 1bdc8e6684f7..fe4bb70eb9c5 100644
--- a/tools/testing/selftests/bpf/verifier/atomic_and.c
+++ b/tools/testing/selftests/bpf/verifier/atomic_and.c
@@ -75,3 +75,26 @@
},
.result = ACCEPT,
},
+{
+ "BPF_ATOMIC_AND with fetch - r0 as source reg",
+ .insns = {
+ /* val = 0x110; */
+ BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0x110),
+ /* old = atomic_fetch_and(&val, 0x011); */
+ BPF_MOV64_IMM(BPF_REG_0, 0x011),
+ BPF_ATOMIC_OP(BPF_DW, BPF_AND | BPF_FETCH, BPF_REG_10, BPF_REG_0, -8),
+ /* if (old != 0x110) exit(3); */
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0x110, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 3),
+ BPF_EXIT_INSN(),
+ /* if (val != 0x010) exit(2); */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_10, -8),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0x010, 2),
+ BPF_MOV64_IMM(BPF_REG_1, 2),
+ BPF_EXIT_INSN(),
+ /* exit(0); */
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+},
base-commit: 5e1d40b75ed85ecd76347273da17e5da195c3e96
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] bpf: x86: Fix BPF_FETCH atomic and/or/xor with r0 as src
2021-02-15 16:00 [PATCH bpf-next] bpf: x86: Fix BPF_FETCH atomic and/or/xor with r0 as src Brendan Jackman
@ 2021-02-15 21:09 ` KP Singh
2021-02-16 10:33 ` Brendan Jackman
0 siblings, 1 reply; 4+ messages in thread
From: KP Singh @ 2021-02-15 21:09 UTC (permalink / raw)
To: Brendan Jackman
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Ilya Leoshkevich
On Mon, Feb 15, 2021 at 5:00 PM Brendan Jackman <jackmanb@google.com> wrote:
>
> This code generates a CMPXCHG loop in order to implement atomic_fetch
> bitwise operations. Because CMPXCHG is hard-coded to use rax (which
> holds the BPF r0 value), it saves the _real_ r0 value into the
> internal "ax" temporary register and restores it once the loop is
> complete.
>
> In the middle of the loop, the actual bitwise operation is performed
> using src_reg. The bug occurs when src_reg is r0: as described above,
> r0 has been clobbered and the real r0 value is in the ax register.
>
> Therefore, perform this operation on the ax register instead, when
> src_reg is r0.
>
> Fixes: 981f94c3e921 ("bpf: Add bitwise atomic instructions")
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
> arch/x86/net/bpf_jit_comp.c | 7 +++---
> .../selftests/bpf/verifier/atomic_and.c | 23 +++++++++++++++++++
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 79e7a0ec1da5..0c9edfe42340 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1349,6 +1349,7 @@ st: if (is_imm8(insn->off))
> insn->imm == (BPF_XOR | BPF_FETCH)) {
> u8 *branch_target;
> bool is64 = BPF_SIZE(insn->code) == BPF_DW;
> + u32 real_src_reg = src_reg == BPF_REG_0 ? BPF_REG_AX : src_reg;
I think it would be more readable as:
u32 real_src_reg = src_reg;
/* Add a comment here why this is needed */
if (src_reg == BPF_REG_0)
real_src_reg = BPF_REG_AX;
>
> /*
> * Can't be implemented with a single x86 insn.
> @@ -1366,9 +1367,9 @@ st: if (is_imm8(insn->off))
> * put the result in the AUX_REG.
> */
> emit_mov_reg(&prog, is64, AUX_REG, BPF_REG_0);
> - maybe_emit_mod(&prog, AUX_REG, src_reg, is64);
> + maybe_emit_mod(&prog, AUX_REG, real_src_reg, is64);
> EMIT2(simple_alu_opcodes[BPF_OP(insn->imm)],
> - add_2reg(0xC0, AUX_REG, src_reg));
> + add_2reg(0xC0, AUX_REG, real_src_reg));
> /* Attempt to swap in new value */
> err = emit_atomic(&prog, BPF_CMPXCHG,
> dst_reg, AUX_REG, insn->off,
> @@ -1381,7 +1382,7 @@ st: if (is_imm8(insn->off))
> */
> EMIT2(X86_JNE, -(prog - branch_target) - 2);
> /* Return the pre-modification value */
> - emit_mov_reg(&prog, is64, src_reg, BPF_REG_0);
> + emit_mov_reg(&prog, is64, real_src_reg, BPF_REG_0);
> /* Restore R0 after clobbering RAX */
> emit_mov_reg(&prog, true, BPF_REG_0, BPF_REG_AX);
[...]
>
> base-commit: 5e1d40b75ed85ecd76347273da17e5da195c3e96
> --
> 2.30.0.478.g8a0d178c01-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] bpf: x86: Fix BPF_FETCH atomic and/or/xor with r0 as src
2021-02-15 21:09 ` KP Singh
@ 2021-02-16 10:33 ` Brendan Jackman
2021-02-16 11:53 ` KP Singh
0 siblings, 1 reply; 4+ messages in thread
From: Brendan Jackman @ 2021-02-16 10:33 UTC (permalink / raw)
To: KP Singh
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Ilya Leoshkevich
On Mon, 15 Feb 2021 at 22:09, KP Singh <kpsingh@kernel.org> wrote:
>
> On Mon, Feb 15, 2021 at 5:00 PM Brendan Jackman <jackmanb@google.com> wrote:
> >
> > This code generates a CMPXCHG loop in order to implement atomic_fetch
> > bitwise operations. Because CMPXCHG is hard-coded to use rax (which
> > holds the BPF r0 value), it saves the _real_ r0 value into the
> > internal "ax" temporary register and restores it once the loop is
> > complete.
> >
> > In the middle of the loop, the actual bitwise operation is performed
> > using src_reg. The bug occurs when src_reg is r0: as described above,
> > r0 has been clobbered and the real r0 value is in the ax register.
> >
> > Therefore, perform this operation on the ax register instead, when
> > src_reg is r0.
> >
> > Fixes: 981f94c3e921 ("bpf: Add bitwise atomic instructions")
> > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > ---
> > arch/x86/net/bpf_jit_comp.c | 7 +++---
> > .../selftests/bpf/verifier/atomic_and.c | 23 +++++++++++++++++++
> > 2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index 79e7a0ec1da5..0c9edfe42340 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1349,6 +1349,7 @@ st: if (is_imm8(insn->off))
> > insn->imm == (BPF_XOR | BPF_FETCH)) {
> > u8 *branch_target;
> > bool is64 = BPF_SIZE(insn->code) == BPF_DW;
> > + u32 real_src_reg = src_reg == BPF_REG_0 ? BPF_REG_AX : src_reg;
>
> I think it would be more readable as:
>
> u32 real_src_reg = src_reg;
>
> /* Add a comment here why this is needed */
> if (src_reg == BPF_REG_0)
> real_src_reg = BPF_REG_AX;
Yes good idea - actually if I put it next to the relevant mov:
/* Will need RAX as a CMPXCHG operand so save R0 */
emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0)
if (src_reg == BPF_REG_0)
real_src_reg = BPF_REG_AX;
I don't think it even needs a comment - what do you think?
> >
> > /*
> > * Can't be implemented with a single x86 insn.
> > @@ -1366,9 +1367,9 @@ st: if (is_imm8(insn->off))
> > * put the result in the AUX_REG.
> > */
> > emit_mov_reg(&prog, is64, AUX_REG, BPF_REG_0);
> > - maybe_emit_mod(&prog, AUX_REG, src_reg, is64);
> > + maybe_emit_mod(&prog, AUX_REG, real_src_reg, is64);
> > EMIT2(simple_alu_opcodes[BPF_OP(insn->imm)],
> > - add_2reg(0xC0, AUX_REG, src_reg));
> > + add_2reg(0xC0, AUX_REG, real_src_reg));
> > /* Attempt to swap in new value */
> > err = emit_atomic(&prog, BPF_CMPXCHG,
> > dst_reg, AUX_REG, insn->off,
> > @@ -1381,7 +1382,7 @@ st: if (is_imm8(insn->off))
> > */
> > EMIT2(X86_JNE, -(prog - branch_target) - 2);
> > /* Return the pre-modification value */
> > - emit_mov_reg(&prog, is64, src_reg, BPF_REG_0);
> > + emit_mov_reg(&prog, is64, real_src_reg, BPF_REG_0);
> > /* Restore R0 after clobbering RAX */
> > emit_mov_reg(&prog, true, BPF_REG_0, BPF_REG_AX);
>
> [...]
>
> >
> > base-commit: 5e1d40b75ed85ecd76347273da17e5da195c3e96
> > --
> > 2.30.0.478.g8a0d178c01-goog
> >
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] bpf: x86: Fix BPF_FETCH atomic and/or/xor with r0 as src
2021-02-16 10:33 ` Brendan Jackman
@ 2021-02-16 11:53 ` KP Singh
0 siblings, 0 replies; 4+ messages in thread
From: KP Singh @ 2021-02-16 11:53 UTC (permalink / raw)
To: Brendan Jackman
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Ilya Leoshkevich
On Tue, Feb 16, 2021 at 11:33 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Mon, 15 Feb 2021 at 22:09, KP Singh <kpsingh@kernel.org> wrote:
> >
> > On Mon, Feb 15, 2021 at 5:00 PM Brendan Jackman <jackmanb@google.com> wrote:
> > >
> > > This code generates a CMPXCHG loop in order to implement atomic_fetch
> > > bitwise operations. Because CMPXCHG is hard-coded to use rax (which
> > > holds the BPF r0 value), it saves the _real_ r0 value into the
> > > internal "ax" temporary register and restores it once the loop is
> > > complete.
> > >
> > > In the middle of the loop, the actual bitwise operation is performed
> > > using src_reg. The bug occurs when src_reg is r0: as described above,
> > > r0 has been clobbered and the real r0 value is in the ax register.
> > >
> > > Therefore, perform this operation on the ax register instead, when
> > > src_reg is r0.
> > >
> > > Fixes: 981f94c3e921 ("bpf: Add bitwise atomic instructions")
> > > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > > ---
> > > arch/x86/net/bpf_jit_comp.c | 7 +++---
> > > .../selftests/bpf/verifier/atomic_and.c | 23 +++++++++++++++++++
> > > 2 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 79e7a0ec1da5..0c9edfe42340 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -1349,6 +1349,7 @@ st: if (is_imm8(insn->off))
> > > insn->imm == (BPF_XOR | BPF_FETCH)) {
> > > u8 *branch_target;
> > > bool is64 = BPF_SIZE(insn->code) == BPF_DW;
> > > + u32 real_src_reg = src_reg == BPF_REG_0 ? BPF_REG_AX : src_reg;
> >
> > I think it would be more readable as:
> >
> > u32 real_src_reg = src_reg;
> >
> > /* Add a comment here why this is needed */
> > if (src_reg == BPF_REG_0)
> > real_src_reg = BPF_REG_AX;
>
> Yes good idea - actually if I put it next to the relevant mov:
>
> /* Will need RAX as a CMPXCHG operand so save R0 */
> emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0)
> if (src_reg == BPF_REG_0)
> real_src_reg = BPF_REG_AX;
>
> I don't think it even needs a comment - what do you think?
Yeah moving it there makes sense and you already have a comment there.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-16 11:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-15 16:00 [PATCH bpf-next] bpf: x86: Fix BPF_FETCH atomic and/or/xor with r0 as src Brendan Jackman
2021-02-15 21:09 ` KP Singh
2021-02-16 10:33 ` Brendan Jackman
2021-02-16 11:53 ` KP Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox