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.
> Do you want to get rid of ./test_verifier binary?
> If so, we can move such tests under ./test_progs w/o converting to
> inline assembly.
Ideally, both test_verifier as a separate test runner to unify
everything in test_progs "framework", which is much better integrated
into BPF CI. But it would also be nice to get rid of almost 2k lines
of code in test_verifier.c. But it's "ideally", it depends on how much
new hacky code would be necessary to achieve this. No strong feelings
here.
>
> [...]
> >
> > > # Pseudo-call instructions (9 files)
> > >
> > > An object file might contain several BPF programs plus some functions used from
> > > different programs. In order to load a program from such file, `libbpf` creates
> > > a buffer and copies the program and all functions called from this program into
> > > that buffer. For each visited pseudo-call instruction `libbpf` requires it to
> > > point to a valid function described in ELF header.
> > >
> > > However, this is not how `verifier/*.c` tests are written, for example here is a
> > > translated fragment from `verifier/loops1.c`:
> > >
> > > SEC("tracepoint")
> > > __description("bounded recursion")
> > > __failure __msg("back-edge")
> > > __naked void bounded_recursion(void)
> > > {
> > > asm volatile (" \
> > > r1 = 0; \
> > > call l0_%=; \
> > > exit; \
> > > l0_%=: r1 += 1; \
> > > r0 = r1; \
> > > if r1 < 4 goto l1_%=; \
> > > exit; \
> > > l1_%=: call l0_%=; \
> > > exit; \
> > > " ::: __clobber_all);
> > > }
> > >
> > > There are several possibilities here:
> > > - split such tests into several functions during migration;
> > > - add a special flag for `libbpf` asking to allow such calls;
> > > - Andrii also suggested to try using `.section` directives inside inline
> > > assembly block.
> > >
> > > This requires further investigation, I'll discuss it with Andrii some time later
> > > this week or on Monday.
> >
> > So I did try this a few weeks ago, and yes, you can make this work
> > with assembly directives. Except that DWARF (and thus .BTF and
> > .BTF.ext) information won't be emitted, as it is emitted very
> > painfully and manually by C compiler as explicit assembly directives.
> > But we might work around that by clearing .BTF and .BTF.ext
> > information for such object files, perhaps. So tentatively this should
> > be doable.
>
> Could you please share an example?
I don't think I saved that. But I just looked at what asm Clang
produces from C code with -S argument.
>
> [...]
> > > # `.fill_helper` (5 files)
> > >
> > > Programs for some tests are generated programmatically by specifying
> > > `.fill_helper` function in the test description, e.g. `verifier/scale.c`:
> > >
> > > {
> > > "scale: scale test 1",
> > > .insns = { },
> > > .data = { },
> > > .fill_helper = bpf_fill_scale,
> > > .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> > > .result = ACCEPT,
> > > .retval = 1,
> > > },
> > >
> > > Such tests cannot be migrated
> > > (but sometimes these are not the only tests in the file).
> >
> > We can just write these as explicitly programmatically generated
> > programs, probably. There are just a few of these, shouldn't be a big
> > deal.
>
> You mean move the generating function from test_verifier.c to some
> test under prog_tests/whatever.c, right?
yes, generating function + add bpf_prog_load()-based test around them
>
> > > # libbpf does not like some junk code (3 files)
> > >
> > > `libbpf` (and bpftool) reject some junk instructions intentionally encoded in
> > > the tests, e.g. empty program from `verifier/basic.c`:
> > >
> > > SEC("socket")
> > > __description("empty prog")
> > > __failure __msg("last insn is not an exit or jmp")
> > > __failure_unpriv
> > > __naked void empty_prog(void)
> > > {
> >
> > even if you add some random "r0 = 0" instruction? It won't change the
> > meaning of the test, but should work with libbpf.
>
> Random instruction should work.
>
> >
> > > asm volatile ("" ::: __clobber_all);
> > > }
> > >
> > > # Small log buffer (2 files)
> > >
> > > Currently `test_loader.c` uses 1Mb log buffer, while `test_verifier.c` uses 16Mb
> > > log buffer. There are a few tests (like in `verifier/bounds.c`) that exit with
> > > `-ESPC` for 1Mb buffer.
> > >
> > > I can either bump log buffer size for `test_loader.c` or wait until Andrii's
> > > rotating log implementation lands.
> >
> > Just bump to 16MB, no need to wait on anything.
>
> Will do.
>
> [...]
