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.
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.
