On Mon, 2021-02-15 at 23:20 +0100, Daniel Borkmann wrote: > On 2/15/21 6:12 PM, Brendan Jackman wrote: > > 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@xxxxxxxxxxxxx> > > Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg") > > Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx> > > --- > > 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. [...]