Re: [PATCH v2 bpf] bpf: Account for BPF_FETCH in insn_has_def32()

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Mon, 2021-03-01 at 12:02 +0100, Brendan Jackman wrote:
> On Fri, 26 Feb 2021 at 22:31, Ilya Leoshkevich <iii@linux.ibm.com>
> 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@linux.ibm.com/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.