On Wed, Feb 17, 2021 at 1:50 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 2/16/21 3:19 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. The same issue affects s390.
> >
> > 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. Since this problem affects multiple archs, this is done in
> > the verifier by patching in a BPF_ZEXT_REG instruction after every
> > 32-bit cmpxchg. Any archs that don't need such manual zero-extension
> > can do a look-ahead with insn_is_zext to skip the unnecessary mov.
> >
> > Reported-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
> > Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
> [...]
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 16ba43352a5f..7f4a83d62acc 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -11889,6 +11889,39 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> > return 0;
> > }
> >
> > +/* BPF_CMPXCHG always loads a value into R0, therefore always zero-extends.
> > + * However some archs' equivalent instruction only does this load when the
> > + * comparison is successful. So here we add a BPF_ZEXT_REG after every 32-bit
> > + * CMPXCHG, so that such archs' JITs don't need to deal with the issue. Archs
> > + * that don't face this issue may use insn_is_zext to detect and skip the added
> > + * instruction.
> > + */
> > +static int add_zext_after_cmpxchg(struct bpf_verifier_env *env)
> > +{
> > + struct bpf_insn zext_patch[2] = { [1] = BPF_ZEXT_REG(BPF_REG_0) };
> > + struct bpf_insn *insn = env->prog->insnsi;
> > + int insn_cnt = env->prog->len;
> > + struct bpf_prog *new_prog;
> > + int delta = 0; /* Number of instructions added */
> > + int i;
> > +
> > + for (i = 0; i < insn_cnt; i++, insn++) {
> > + if (insn->code != (BPF_STX | BPF_W | BPF_ATOMIC) || insn->imm != BPF_CMPXCHG)
> > + continue;
> > +
> > + zext_patch[0] = *insn;
> > + new_prog = bpf_patch_insn_data(env, i + delta, zext_patch, 2);
> > + if (!new_prog)
> > + return -ENOMEM;
> > +
> > + delta++;
> > + env->prog = new_prog;
> > + insn = new_prog->insnsi + i + delta;
> > + }
>
> Looks good overall, one small nit ... is it possible to move this into fixup_bpf_calls()
> where we walk the prog insns & handle most of the rewrites already?
Ah, so I thought fixup_bpf_calls was for "calls" but now looking at
the function it does
more than just fixing up calls. I guess we could also rename it and
update the comment
on the function.
- KP
>
> > +
> > + return 0;
> > +}
