On Mon, Nov 11, 2024 at 1:56 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Mon, Nov 11, 2024 at 1:53 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, Nov 11, 2024 at 10:46 AM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Wed, Oct 30, 2024 at 4:51 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > > >
> > > > Async callback state enqueing, while logically detached from parent
> > >
> > > typo. enqueuing
> >
> > yep, tricky word :) should be "enqueueing", fixed
> >
> > >
> > > > -static int get_prev_insn_idx(struct bpf_verifier_state *st, int i,
> > > > - u32 *history)
> > > > +static int get_prev_insn_idx(const struct bpf_verifier_env *env,
> > > > + struct bpf_verifier_state *st,
> > > > + int insn_idx, u32 hist_start, u32 *hist_endp)
> > > > {
> > > > - u32 cnt = *history;
> > > > + u32 hist_end = *hist_endp;
> > > > + u32 cnt = hist_end - hist_start;
> > > >
> > > > - if (i == st->first_insn_idx) {
> > > > + if (insn_idx == st->first_insn_idx) {
> > > > if (cnt == 0)
> > > > return -ENOENT;
> > > > - if (cnt == 1 && st->jmp_history[0].idx == i)
> > > > + if (cnt == 1 && env->insn_hist[hist_end - 1].idx == insn_idx)
> > > > return -ENOENT;
> > > > }
> > >
> > > I think the above bit would be easier to understand if it was
> > > env->insn_hist[hist_start].
> > >
> > > When cnt==1 it's the same as hist_end-1, but it took me more time
> > > to grok that part. With [hist_start] would have been easier.
> > > Not a big deal.
> >
> > yep, I agree. Originally I didn't pass hist_start directly, so I would
> > have to use st->insn_hist_start, and it felt too verbose. But now
> > that's not a problem, I'll use hist_start everywhere.
> >
> > >
> > > Another minor suggestion...
> > > wouldn't it be cleaner to take hist_start/end from 'st' both
> > > in get_prev_insn_idx() and in get_insn_hist_entry() ?
> > >
> > > So that __mark_chain_precision() doesn't need to reach out into
> > > details of 'st' just to pass hist_start/end values into other helpers.
> >
> > Note that for get_prev_insn_idx() we modify (but only locally!)
> > hist_end, as we process instruction history for the currently
> > processed state (we do a virtual stack pop for each entry). So we
> > can't just use st->insn_hist_end, we need a local copy for hist_end
> > that will be updated without touching the actual insn_hist_end. That's
> > the reason I have `u32 hist_end = st->insn_hist_end;`, to pass
> > &hist_end into get_prev_insn_idx().
> >
> > Having said that, if you prefer, I can fetch insn_hist_{start, end}
> > from st, always, but then maintain local hist_cnt as input argument
> > for get_insn_hist_enrty() and in/out argument for get_prev_insn_idx().
> > Would you prefer that? something like below:
> >
>
> Argh, gmail messed this up. See [0] for better formatting.
>
> [0] https://gist.github.com/anakryiko/25228b0ae2760f78b7ae7f0160faa5c1
I had a tiny bug in get_prev_insns_idx(), fixing which makes
get_prev_insn_idx() a bit verbose if using this hist_cnt approach:
static int get_prev_insn_idx(const struct bpf_verifier_env *env,
struct bpf_verifier_state *st,
int insn_idx, u32 *hist_cntp)
{
u32 cnt = *hist_cntp;
if (insn_idx == st->first_insn_idx) {
if (cnt == 0)
return -ENOENT;
if (cnt == 1 && env->insn_hist[st->insn_hist_start].idx == insn_idx)
return -ENOENT;
}
if (cnt && env->insn_hist[st->insn_hist_start + hist_cnt - 1].idx ==
insn_idx) {
*hist_cntp = cnt - 1;
return env->insn_hist[st->insn_hist_start + hist_cnt - 1].prev_idx;
} else {
return insn_idx - 1;
}
}
I mean that `env->insn_hist[st->insn_hist_start + hist_cnt - 1]` in
last if/else. But let me know which way you prefer it anyways.
