On Thu, Nov 9, 2023 at 10:20 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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?
I'll add a single line helper function just to not be PITA, but I
don't think it's worth it. There are two places we do this, one next
to the other within the same function. This helper is just going to
add mental overhead and won't really help us with anything.
>
> [...]
>
> > > > +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.
>
I added it, and I hate it. It's just a visual noise. Feels too
paranoid, I'll probably drop it.
> [...]
>
> > > > @@ -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()?
Note that we set insn_flags only for cases where we do an actual
register spill (save_register_state calls for non-fake registers). If
register spill is possible from a helper call somehow, we'll be in
much bigger trouble elsewhere.
>
> > [...]
> >
> > trimming is good
>
> Sigh... sorry, really tried to trim everything today.
