On Thu, Nov 9, 2023 at 3:18 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Nov 9, 2023 at 9:20 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Wed, 2023-11-08 at 15:11 -0800, Andrii Nakryiko wrote:
> > > Fix an edge case in __mark_chain_precision() which prematurely stops
> > > backtracking instructions in a state if it happens that state's first
> > > and last instruction indexes are the same. This situations doesn't
> > > necessarily mean that there were no instructions simulated in a state,
> > > but rather that we starting from the instruction, jumped around a bit,
> > > and then ended up at the same instruction before checkpointing or
> > > marking precision.
> > >
> > > To distinguish between these two possible situations, we need to consult
> > > jump history. If it's empty or contain a single record "bridging" parent
> > > state and first instruction of processed state, then we indeed
> > > backtracked all instructions in this state. But if history is not empty,
> > > we are definitely not done yet.
> > >
> > > Move this logic inside get_prev_insn_idx() to contain it more nicely.
> > > Use -ENOENT return code to denote "we are out of instructions"
> > > situation.
> > >
> > > This bug was exposed by verifier_cfg.c's bounded_recursion subtest, once
> >
> > Note: verifier_cfg.c should be verifier_loops1.c
>
> fixed
>
> >
> > > the next fix in this patch set is applied.
> > >
> > > Fixes: b5dc0163d8fd ("bpf: precise scalar_value tracking")
> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >
> > Funny how nobody noticed this bug for so long, I looked at exactly
> > this code today while going through your other patch-set and no alarm
> > bells rang in my head.
>
> tell me about it, I spent 3 hours with gdb and tons of extra verifier
> logging before I found the issue
>
> >
> > I think that this case needs a dedicated test case that would check
> > precision tracking log.
>
> ok, will add
>
But I will say that it would be much better if verifier/precise.c was
converted to embedded assembly... Let's see if we can somehow
negotiate completing test_verifier conversion? ;)
> >
> > Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
