On Wed, Dec 7, 2022 at 10:36 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Dec 6, 2022 at 7:17 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Tue, Dec 06, 2022 at 03:33:45PM -0800, Andrii Nakryiko wrote:
> > > Don't mark some instructions as jump points when there are actually no
> > > jumps and instructions are just processed sequentially. Such case is
> > > handled naturally by precision backtracking logic without the need to
> > > update jump history. See get_prev_insn_idx(). It goes back linearly by
> > > one instruction, unless current top of jmp_history is pointing to
> > > current instruction. In such case we use `st->jmp_history[cnt - 1].prev_idx`
> > > to find instruction from which we jumped to the current instruction
> > > non-linearly.
> > >
> > > Also remove both jump and prune point marking for instruction right
> > > after unconditional jumps, as program flow can get to the instruction
> > > right after unconditional jump instruction only if there is a jump to
> > > that instruction from somewhere else in the program. In such case we'll
> > > mark such instruction as prune/jump point because it's a destination of
> > > a jump.
> > >
> > > This change has no changes in terms of number of instructions or states
> > > processes across Cilium and selftests programs.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > ---
> > > kernel/bpf/verifier.c | 34 ++++++++++------------------------
> > > 1 file changed, 10 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index b1feb8d3c42e..4f163a28ab59 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -12228,13 +12228,12 @@ static int visit_func_call_insn(int t, int insn_cnt,
> > > if (ret)
> > > return ret;
> > >
> > > - if (t + 1 < insn_cnt) {
> > > - mark_prune_point(env, t + 1);
> > > - mark_jmp_point(env, t + 1);
> > > - }
> > > + mark_prune_point(env, t + 1);
> > > + /* when we exit from subprog, we need to record non-linear history */
> > > + mark_jmp_point(env, t + 1);
> > > +
> >
> > With this we probably should remove 'insn_cnt' from visit_func_call_insn().
> > and in-turn from visit_insn() as well.
> > Pls consider as a follow up.
>
> Yep, will do, didn't notice it's not needed anymore.
Thanks
> BTW, no one asked why it was ok to drop the `if (t + 1 < insns_cnt)`
> check, I was a bit surprised. But this is because push_insns() already
> validates that t+1 is correct and doesn't go beyond the insns array,
> so this was not needed in the first place.
Correct. I read the code the same way.
I was a bit concerned whether insn_cnt is always equal to env->prog->len.
I thought we're tightening insn_cnt to be the subprog insn_cnt only.
But that doesn't look to be the case.
So looks safe to remove that 'if' and hence insn_cnt is useless too,
since it's the same as env->prog->len.
