On Tue, Mar 28, 2023 at 4:31 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Mar 28, 2023 at 3:39 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Tue, 2023-03-28 at 15:24 -0700, Andrii Nakryiko wrote:
> > [...]
> > >
> > > > # Simplistic tests (14 files)
> > > >
> > > > Some tests are just simplistic and it is not clear if moving those to inline
> > > > assembly really makes sense, for example, here is `basic_call.c`:
> > > >
> > > > {
> > > > "invalid call insn1",
> > > > .insns = {
> > > > BPF_RAW_INSN(BPF_JMP | BPF_CALL | BPF_X, 0, 0, 0, 0),
> > > > BPF_EXIT_INSN(),
> > > > },
> > > > .errstr = "unknown opcode 8d",
> > > > .result = REJECT,
> > > > },
> > > >
> > >
> > > For tests like this we can have a simple ELF parser/loader that
> > > doesn't use bpf_object__open() functionality. It's not too hard to
> > > just find all the FUNC ELF symbols and fetch corresponding raw
> > > instructions. Assumption here is that we can take those assembly
> > > instructions as is, of course. If there are some map references and
> > > such, this won't work.
> >
> > Custom elf parser/loader is interesting.
> > However, also consider how such tests look in assembly:
> >
> > SEC("socket")
> > __description("invalid call insn1")
> > __failure __msg("unknown opcode 8d")
> > __failure_unpriv
> > __naked void invalid_call_insn1(void)
> > {
> > asm volatile (" \
> > .8byte %[raw_insn]; \
> > exit; \
> > " :
> > : __imm_insn(raw_insn, BPF_RAW_INSN(BPF_JMP | BPF_CALL | BPF_X, 0, 0, 0, 0))
> > : __clobber_all);
> > }
> >
> > I'd say that original is better.
>
> +1
>
> > Do you want to get rid of ./test_verifier binary?
>
> All this work looks like a diminishing return.
> It's ok to keep test_verifier around.
> All new asm test can already go into test_progs and in some rare cases
> test_verifier will be a better home for them.
I definitely don't want us to go crazy and just reimplement
test_verifier.c inside test_progs, of course. But I do see value of
getting rid of test_verifier as a separate test runner (and hopefully
most of 1.7K lines of code in test_verifier.c). I do agree that these
bad/raw instructions are not the most readable alternative, though.
But taking those few tests with invalid instructions and patterns
(using BPF_RAW_INSN() macro and others) and writing them as explicit
test_progs' test with bpf_prog_load() seems like a better alternative.
test_progs has much better integration with BPF CI, and not having to
remember to run test_verifier locally seems like a win.
