On Thu, Nov 9, 2023 at 8:08 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Nov 9, 2023 at 7:41 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Thu, Nov 9, 2023 at 5:26 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Wed, Nov 8, 2023 at 3:12 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > > >
> > > >
> > > > @@ -15622,11 +15619,11 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
> > > > /* conditional jump with two edges */
> > > > mark_prune_point(env, t);
> > > >
> > > > - ret = push_insn(t, t + 1, FALLTHROUGH, env, true);
> > > > + ret = push_insn(t, t + 1, FALLTHROUGH | CONDITIONAL, env);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - return push_insn(t, t + insn->off + 1, BRANCH, env, true);
> > > > + return push_insn(t, t + insn->off + 1, BRANCH | CONDITIONAL, env);
> > >
> > > If I'm reading this correctly, after the first conditional jmp
> > > both fallthrough and branch become sticky with the CONDITIONAL flag.
> > > So all processing after first 'if a == b' insn is running
> > > with loop_ok==true.
> > > If so, all this complexity is not worth it. Let's just remove 'loop_ok' flag.
> >
> > So you mean just always assume loop_ok, right?
>
> yes.
> Since not detecting the loop early only adds more cycles later
> that states_maybe_looping() should catch quickly enough.
>
> > >
> > > Also
> > > if (ret) return ret;
> > > in the above looks like an existing bug.
> > > It probably should be if (ret < 0) return ret;
> >
> > yeah, probably should be error-handling case
>
> I thought that refactoring
> commit 59e2e27d227a ("bpf: Refactor check_cfg to use a structured loop.")
> is responsible...
> but it looks to be an older bug.
No, I think it was indeed 59e2e27d227a that changed this. Previously we had
if (ret == 1)
...
if (ret < 0)
goto err;
/* ret == 0 */
I'll add the fix on top of another fix.
