Re: [PATCH bpf-next 2/7] bpf: support non-r10 register spill/fill to/from stack in precision tracking

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2023-11-09 at 09:20 -0800, Andrii Nakryiko wrote:
[...]
> > >  struct bpf_insn_hist_entry {
> > > -     u32 prev_idx;
> > >       u32 idx;
> > > +     /* insn idx can't be bigger than 1 million */
> > > +     u32 prev_idx : 22;
> > > +     /* special flags, e.g., whether insn is doing register stack spill/load */
> > > +     u32 flags : 10;
> > >  };
> > 
> > Nitpick: maybe use separate bit-fields for frameno and spi instead of
> >          flags? Or add dedicated accessor functions?
> 
> I wanted to keep it very uniform so that push_insn_history() doesn't
> know about all such details. It just has "flags". We might use these
> flags for some other use cases, though if we run out of bits we'll
> probably just expand bpf_insn_hist_entry and refactor existing code
> anyways. So, basically, I didn't want to over-engineer this bit too
> much :)

Well, maybe hide "(hist->flags >> INSN_F_SPI_SHIFT) & INSN_F_SPI_MASK"
behind an accessor?

[...]

> > > +static int push_insn_history(struct bpf_verifier_env *env, struct bpf_verifier_state *cur,
> > > +                          int insn_flags)
> > >  {
> > >       struct bpf_insn_hist_entry *p;
> > >       size_t alloc_size;
> > > 
> > > -     if (!is_jmp_point(env, env->insn_idx))
> > > +     /* combine instruction flags if we already recorded this instruction */
> > > +     if (cur->insn_hist_end > cur->insn_hist_start &&
> > > +         (p = &env->insn_hist[cur->insn_hist_end - 1]) &&
> > > +         p->idx == env->insn_idx &&
> > > +         p->prev_idx == env->prev_insn_idx) {
> > > +             p->flags |= insn_flags;
> > 
> > Nitpick: maybe add an assert to check that frameno/spi are not or'ed?
> 
> ok, something like
> 
> WARN_ON_ONCE(p->flags & (INSN_F_STACK_ACCESS | INSN_F_FRAMENOMASK |
> (INSN_F_SPI_MASK << INSN_F_SPI_SHIFT)));
> 
> ?

Something like this, yes.

[...]

> > > @@ -4713,9 +4711,12 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> > > 
> > >               /* Mark slots affected by this stack write. */
> > >               for (i = 0; i < size; i++)
> > > -                     state->stack[spi].slot_type[(slot - i) % BPF_REG_SIZE] =
> > > -                             type;
> > > +                     state->stack[spi].slot_type[(slot - i) % BPF_REG_SIZE] = type;
> > > +             insn_flags = 0; /* not a register spill */
> > >       }
> > > +
> > > +     if (insn_flags)
> > > +             return push_insn_history(env, env->cur_state, insn_flags);
> > 
> > Maybe add a check that insn is BPF_ST or BPF_STX here?
> > Only these cases are supported by backtrack_insn() while
> > check_mem_access() is called from multiple places.
> 
> seems like a wrong place to enforce that check_stack_write_fixed_off()
> is called only for those instructions?

check_stack_write_fixed_off() is called from check_stack_write() which
is called from check_mem_access() which might trigger
check_stack_write_fixed_off() when called with BPF_WRITE flag and
pointer to stack as an argument.
This happens for ST, STX but also in check_helper_call(),
process_iter_arg() (maybe other places).
Speaking of which, should this be handled in backtrack_insn()?

> [...]
> 
> trimming is good

Sigh... sorry, really tried to trim everything today.