On Mon, Nov 11, 2024 at 2:19 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
Ohh. I missed that decrement is local to __mark_chain_precision().
Original version is probably better than when it passes start/end around.
This hist_cnt approach is harder to read.
So pls resend with typo fixed.
