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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Mar 28, 2023 at 3:39 PM Eduard Zingerman <eddyz87@gmail.com> 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.
>
> [...]