Re: [PATCH bpf-next 00/43] First set of verifier/*.c migrated to inline assembly

[Daniel Borkmann <daniel@iogearbox.net>]

Hi Eduard,

On 3/25/23 3:54 AM, Eduard Zingerman wrote:
> This is a follow up for RFC [1]. It migrates a first batch of 38
> verifier/*.c tests to inline assembly and use of ./test_progs for
> actual execution. The migration is done by a python script (see [2]).
> 
> Each migrated verifier/xxx.c file is mapped to progs/verifier_xxx.c
> plus an entry in the prog_tests/verifier.c. One patch per each file.
> 
> A few patches at the beginning of the patch-set extend test_loader
> with necessary functionality, mainly:
> - support for tests execution in unprivileged mode;
> - support for test runs for test programs.
> 
> Migrated tests could be selected for execution using the following filter:
> 
>    ./test_progs -a verifier_*
>    
> An example of the migrated test:
> 
>    SEC("xdp")
>    __description("XDP pkt read, pkt_data' > pkt_end, corner case, good access")
>    __success __retval(0) __flag(BPF_F_ANY_ALIGNMENT)
>    __naked void end_corner_case_good_access_1(void)
>    {
>            asm volatile ("                                 \
>            r2 = *(u32*)(r1 + %[xdp_md_data]);              \
>            r3 = *(u32*)(r1 + %[xdp_md_data_end]);          \
>            r1 = r2;                                        \
>            r1 += 8;                                        \
>            if r1 > r3 goto l0_%=;                          \
>            r0 = *(u64*)(r1 - 8);                           \
>    l0_%=:  r0 = 0;                                         \
>            exit;                                           \
>    "       :
>            : __imm_const(xdp_md_data, offsetof(struct xdp_md, data)),
>              __imm_const(xdp_md_data_end, offsetof(struct xdp_md, data_end))
>            : __clobber_all);
>    }
> 
> Changes compared to RFC:
> - test_loader.c is extended to support test program runs;
> - capabilities handling now matches behavior of test_verifier;
> - BPF_ST_MEM instructions are automatically replaced by BPF_STX_MEM
>    instructions to overcome current clang limitations;
> - tests styling updates according to RFC feedback;
> - 38 migrated files are included instead of 1.
> 
> I used the following means for testing:
> - migration tool itself has a set of self-tests;
> - migrated tests are passing;
> - manually compared each old/new file side-by-side.
> 
> While doing side-by-side comparison I've noted a few defects in the
> original tests:
> - and.c:
>    - One of the jump targets is off by one;
>    - BPF_ST_MEM wrong OFF/IMM ordering;
> - array_access.c:
>    - BPF_ST_MEM wrong OFF/IMM ordering;
> - value_or_null.c:
>    - BPF_ST_MEM wrong OFF/IMM ordering.
> 
> These defects would be addressed separately.

The cover letter describes what this series does, but not /why/ we need it. Would be
useful in future to also have the rationale in here. The migration of the verifier
tests look ok, and probably simplifies things to some degree, it certainly makes the
tests more readable. Is the goal to eventually get rid of test_verifier altogether?
I don't think we fully can do that, e.g. what about verifier testing of invalid insn
encodings or things like invalid jumps into the middle of double insns, invalid call
offsets, etc?

> [1] RFC
>      https://lore.kernel.org/bpf/20230123145148.2791939-1-eddyz87@gmail.com/
> [2] Migration tool
>      https://github.com/eddyz87/verifier-tests-migrator

Thanks,
Daniel