On 2/15/21 11:24 PM, Ilya Leoshkevich wrote:

On Mon, 2021-02-15 at 23:20 +0100, Daniel Borkmann wrote:

quoted

On 2/15/21 6:12 PM, Brendan Jackman wrote:

quoted

As pointed out by Ilya and explained in the new comment, there's a
discrepancy between x86 and BPF CMPXCHG semantics: BPF always loads
the value from memory into r0, while x86 only does so when r0 and
the
value in memory are different.

At first this might sound like pure semantics, but it makes a real
difference when the comparison is 32-bit, since the load will
zero-extend r0/rax.

The fix is to explicitly zero-extend rax after doing such a
CMPXCHG.

Note that this doesn't generate totally optimal code: at one of
emit_atomic's callsites (where BPF_{AND,OR,XOR} | BPF_FETCH are
implemented), the new mov is superfluous because there's already a
mov generated afterwards that will zero-extend r0. We could avoid
this unnecessary mov by just moving the new logic outside of
emit_atomic. But I think it's simpler to keep emit_atomic as a unit
of correctness (it generates the correct x86 code for a certain set
of BPF instructions, no further knowledge is needed to use it
correctly).

Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
   arch/x86/net/bpf_jit_comp.c                   | 10 +++++++
   .../selftests/bpf/verifier/atomic_cmpxchg.c   | 25
++++++++++++++++++
   .../selftests/bpf/verifier/atomic_or.c        | 26
+++++++++++++++++++
   3 files changed, 61 insertions(+)

diff --git a/arch/x86/net/bpf_jit_comp.c

b/arch/x86/net/bpf_jit_comp.c
index 79e7a0ec1da5..7919d5c54164 100644

--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c

@@ -834,6 +834,16 @@ static int emit_atomic(u8 **pprog, u8

atomic_op,
   
         emit_insn_suffix(&prog, dst_reg, src_reg, off);
   
+       if (atomic_op == BPF_CMPXCHG && bpf_size == BPF_W) {
+               /*
+                * BPF_CMPXCHG unconditionally loads into R0, which
means it
+                * zero-extends 32-bit values. However x86 CMPXCHG
doesn't do a
+                * load if the comparison is successful. Therefore
zero-extend
+                * explicitly.
+                */
+               emit_mov_reg(&prog, false, BPF_REG_0, BPF_REG_0);

How does the situation look on other archs when they need to
implement this in future?
Mainly asking whether it would be better to instead to move this
logic into the verifier
instead, so it'll be consistent across all archs.

I have exactly the same check in my s390 wip patch.
So having a common solution would be great.

We do rewrites for various cases like div/mod handling, perhaps would be
best to emit an explicit BPF_MOV32_REG(insn->dst_reg, insn->dst_reg) there,
see the fixup_bpf_calls().