On Mon, 2021-03-01 at 12:02 +0100, Brendan Jackman wrote:
> On Fri, 26 Feb 2021 at 22:31, Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> wrote:
[...]
> > @@ -11006,9 +11026,10 @@ static int
> > opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> > for (i = 0; i < len; i++) {
> > int adj_idx = i + delta;
> > struct bpf_insn insn;
> > - u8 load_reg;
> > + int load_reg;
> >
> > insn = insns[adj_idx];
> > + load_reg = insn_def_regno(&insn);
>
> Nit: Might as well save a line by squashing this into the
> declaration.
Will do.
[...]
> > @@ -11049,22 +11070,9 @@ static int
> > opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> > if (!bpf_jit_needs_zext())
> > continue;
> >
> > - /* zext_dst means that we want to zero-extend
> > whatever register
> > - * the insn defines, which is dst_reg most of the
> > time, with
> > - * the notable exception of BPF_STX + BPF_ATOMIC +
> > BPF_FETCH.
> > - */
> > - if (BPF_CLASS(insn.code) == BPF_STX &&
> > - BPF_MODE(insn.code) == BPF_ATOMIC) {
> > - /* BPF_STX + BPF_ATOMIC insns without
> > BPF_FETCH do not
> > - * define any registers, therefore zext_dst
> > cannot be
> > - * set.
> > - */
> > - if (WARN_ON(!(insn.imm & BPF_FETCH)))
> > - return -EINVAL;
> > - load_reg = insn.imm == BPF_CMPXCHG ?
> > BPF_REG_0
> > - :
> > insn.src_reg;
> > - } else {
> > - load_reg = insn.dst_reg;
> > + if (WARN_ON_ONCE(load_reg == -1)) {
> > + verbose(env, "zext_dst is set, but no reg
> > is defined\n");
>
> Let's add the string "verifier bug." to the beginning of this message
> (this is done elsewhere too). Hopefully the only person that ever
> sees
> this message would be someone who's hacking on the verifier, but even
> for them it could be a significant time-saver.
OK.
[...]
> Overall LGTM, thanks. It seems like without this patch, the cmpxchg
> test I added in [1] should fail on the s390 JIT, and this patch
> should
> fix it. Is that correct? If so could you add the test to this patch?
> (I guess you ought to paste in my Signed-off-by)
>
> [1]
> https://lore.kernel.org/bpf/44d680a0c40fc9dddf1b2bf4e78bd75b76dc4061.camel@xxxxxxxxxxxxx/T/#mf6546406db03c6ca473a29cdf3bde7ddeeedf1a1
For this to work, my implementation of atomics needs to be merged
(and I haven't posted it yet). I propose to keep your tests in your
patch, merge this commit first, then your zext patch with tests, then
my atomics patch.
