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:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 15245206d883..34bed6be7449 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3586,10 +3586,14 @@ static int push_insn_history(struct
bpf_verifier_env *env, struct bpf_verifier_s
}
static struct bpf_insn_hist_entry *get_insn_hist_entry(struct
bpf_verifier_env *env,
- u32 hist_start,
u32 hist_end, int insn_idx)
+ struct
bpf_verifier_state *st,
+ u32 hist_cnt,
int insn_idx)
{
- if (hist_end > hist_start && env->insn_hist[hist_end - 1].idx
== insn_idx)
- return &env->insn_hist[hist_end - 1];
+ u32 hist_idx = st->insn_hist_start + hist_cnt - 1;
+
+ if (hist_cnt > 0 && env->insn_hist[hist_idx].idx == insn_idx)
+ return &env->insn_hist[hist_idx];
+
return NULL;
}
@@ -3608,21 +3612,20 @@ static struct bpf_insn_hist_entry
*get_insn_hist_entry(struct bpf_verifier_env *
*/
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)
+ int insn_idx, u32 *hist_cntp)
{
- u32 hist_end = *hist_endp;
- u32 cnt = hist_end - hist_start;
+ u32 cnt = *hist_cntp;
if (insn_idx == st->first_insn_idx) {
if (cnt == 0)
return -ENOENT;
- if (cnt == 1 && env->insn_hist[hist_start].idx == insn_idx)
+ if (cnt == 1 &&
env->insn_hist[st->insn_hist_start].idx == insn_idx)
return -ENOENT;
}
- if (cnt && env->insn_hist[hist_start].idx == insn_idx) {
- (*hist_endp)--;
- return env->insn_hist[hist_start].prev_idx;
+ if (cnt && env->insn_hist[st->insn_hist_start].idx == insn_idx) {
+ *hist_cntp = cnt - 1;
+ return env->insn_hist[st->insn_hist_start].prev_idx;
} else {
return insn_idx - 1;
}
@@ -4378,8 +4381,7 @@ static int __mark_chain_precision(struct
bpf_verifier_env *env, int regno)
for (;;) {
DECLARE_BITMAP(mask, 64);
- u32 hist_start = st->insn_hist_start;
- u32 hist_end = st->insn_hist_end;
+ u32 hist_cnt = st->insn_hist_end - st->insn_hist_start;
struct bpf_insn_hist_entry *hist;
if (env->log.level & BPF_LOG_LEVEL2) {
@@ -4419,7 +4421,7 @@ static int __mark_chain_precision(struct
bpf_verifier_env *env, int regno)
err = 0;
skip_first = false;
} else {
- hist = get_insn_hist_entry(env,
hist_start, hist_end, i);
+ hist = get_insn_hist_entry(env, st,
hist_cnt, i);
err = backtrack_insn(env, i,
subseq_idx, hist, bt);
}
if (err == -ENOTSUPP) {
@@ -4436,7 +4438,7 @@ static int __mark_chain_precision(struct
bpf_verifier_env *env, int regno)
*/
return 0;
subseq_idx = i;
- i = get_prev_insn_idx(env, st, i, hist_start,
&hist_end);
+ i = get_prev_insn_idx(env, st, i, &hist_cnt);
if (i == -ENOENT)
break;
if (i >= env->prog->len) {
