Re: [PATCH v2 bpf 3/3] selftests/bpf: add edge case backtracking logic test

Alexei Starovoitov
2023-11-10T01:34:14Z

On Thu, Nov 9, 2023 at 4:26 PM Andrii Nakryiko <andrii@kernel.org> 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@kernel.org>
> ---
> .../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.

> + */
> +__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?