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.
