On Thu, Nov 9, 2023 at 9:31 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
I take it back:
Summary: 748 PASSED, 0 SKIPPED, 43 FAILED
I'm not touching this. Some other time, or maybe someone else.
