On Thu, Nov 9, 2023 at 5:34 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Nov 9, 2023 at 4:26 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > Add a dedicated selftests to try to set up conditions to have a state
> > with same first and last instruction index, but it actually is a loop
> > 3->4->1->2->3. This confuses mark_chain_precision() if verifier doesn't
> > take into account jump history.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > .../selftests/bpf/progs/verifier_precision.c | 40 +++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_precision.c b/tools/testing/selftests/bpf/progs/verifier_precision.c
> > index 193c0f8272d0..6b564d4c0986 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_precision.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_precision.c
> > @@ -91,3 +91,43 @@ __naked int bpf_end_bswap(void)
> > }
> >
> > #endif /* v4 instruction */
> > +
> > +SEC("?raw_tp")
> > +__success __log_level(2)
> > +/*
> > + * Without the bug fix there will be no history between "last_idx 3 first_idx 3"
> > + * and "parent state regs=" lines. "R0_w=6" parts are here to help anchor
> > + * expected log messages to the one specific mark_chain_precision operation.
> > + *
> > + * This is quite fragile: if verifier checkpointing heuristic changes, this
> > + * might need adjusting.
>
> Hmm, but that what
> __flag(BPF_F_TEST_STATE_FREQ)
> supposed to address.
When I was analysing and crafting the test I for some reason assumed I
need to have a jump inside the state that won't trigger state
checkpoint. But I think that's not necessary, just doing conditional
jump and jumping back an instruction or two should do. With that yes,
TEST_STATE_FREQ should be a better way to do this.
>
> > + */
> > +__msg("2: (07) r0 += 1 ; R0_w=6")
> > +__msg("3: (35) if r0 >= 0xa goto pc+1")
> > +__msg("mark_precise: frame0: last_idx 3 first_idx 3 subseq_idx -1")
> > +__msg("mark_precise: frame0: regs=r0 stack= before 2: (07) r0 += 1")
> > +__msg("mark_precise: frame0: regs=r0 stack= before 1: (07) r0 += 1")
> > +__msg("mark_precise: frame0: regs=r0 stack= before 4: (05) goto pc-4")
> > +__msg("mark_precise: frame0: regs=r0 stack= before 3: (35) if r0 >= 0xa goto pc+1")
> > +__msg("mark_precise: frame0: parent state regs= stack=: R0_rw=P4")
> > +__msg("3: R0_w=6")
> > +__naked int state_loop_first_last_equal(void)
> > +{
> > + asm volatile (
> > + "r0 = 0;"
> > + "l0_%=:"
> > + "r0 += 1;"
> > + "r0 += 1;"
>
> That's why you had two ++ ?
> Add state_freq and remove one of them?
