Hi Daniel,
[...]
>
> 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?
In total there are 57 verifier/*.c files remaining. I've inspected each and came
up with the following categories:
- tool limitation;
- simplistic;
- asm parser issue;
- pseudo-call instructions;
- custom macro;
- `.fill_helper`;
- libbpf discards junk;
- small log buffer.
Categories are described in the following sections, per-test summary is in the end.
The files that are not 'simplistic', not '.fill_helper' and not 'libbpf discards junk'
could be migrated with some additional work.
This gives ~40 files to migrate and ~17 to leave as-is or migrate only partially.
> 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?
Looks like it is the case. E.g. for invalid instructions I can get away with
some junk using the following trick:
asm volatile (".8byte %[raw_insn];"
:
: __imm_insn(raw_insn, BPF_RAW_INSN(0, 0, 0, 0, 0))
: __clobber_all);
But some junk is still rejected by libbpf.
# Migration tool limitations (23 files)
The tool does not know how to migrate tests that use specific features:
- test description field `.expected_attach_type`;
- test description field `.fixup_prog1`, `.fixup_prog2`;
- test description field `.retvals` and `.data`, this also requires support on
`test_loader.c` side;
- `BPF_ENDIAN(BPF_TO_**, BPF_REG_0, 16)` instruction;
- `BPF_SK_LOOKUP` macro;
- `#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__` blocks;
I will update migration tool to handle the cases above.
# 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,
},
# LLVM assembler parser issues (10 files)
There are parsing issues with some constructs, for example:
r1 = atomic_fetch_xor((u64 *)(r10 - 8), r1);
Produces the following diagnostic:
progs/verifier_b2_atomic_xor.c:45:1: error: unexpected token
r1 = atomic_fetch_xor((u64 *)(r10 - 8), r1); \n\
^
<inline asm>:7:40: note: instantiated into assembly here
r1 = atomic_fetch_xor((u64 *)(r10 - 8), r1);
^
I checked LLVM tests and the syntax seems to be correct (at-least this syntax is
used by disassembler). So far I know that the following constructs are affected:
- `atomic_fetch_and`
- `atomic_fetch_cmpxchg`
- `atomic_fetch_or`
- `atomic_fetch_xchg`
- `atomic_fetch_xor`
Requires further investigation and a fix in LLVM.
# 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.
# Custom macro definitions (5 files)
Some tests define and use custom macro, e.g. atomic_fetch.c:
#define __ATOMIC_FETCH_OP_TEST(src_reg, dst_reg, operand1, op, operand2, expect) ...
__ATOMIC_FETCH_OP_TEST(BPF_REG_1, BPF_REG_2, 1, BPF_ADD | BPF_FETCH, 2, 3),
__ATOMIC_FETCH_OP_TEST(BPF_REG_0, BPF_REG_1, 1, BPF_ADD | BPF_FETCH, 2, 3),
// ...
Such tests would have to be migrated manually (the tool still can translate
portions of the test).
# `.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).
# 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)
{
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.
# Per-test summary
- atomic_and - asm parser issue;
- atomic_bounds - asm parser issue;
- atomic_cmpxchg - asm parser issue;
- atomic_fetch - asm parser issue, tool limitation;
- atomic_fetch_add - asm parser issue, uses macro;
- atomic_invalid - simplistic, uses macro;
- atomic_or - asm parser issue;
- atomic_xchg - asm parser issue;
- atomic_xor - asm parser issue;
- basic - simplistic, libbpf discards junk;
- basic_call - simplistic;
- basic_instr - tool limitation;
- basic_stx_ldx - simplistic;
- bounds - small log buffer;
- bpf_get_stack - tool limitation;
- bpf_loop_inline - uses macro, uses .fill_helper;
- bpf_st_mem - asm parser issue;
- btf_ctx_access - tool limitation;
- calls - tool limitation, pseudo call;
- ctx - simplistic, tool limitation;
- ctx_sk_lookup - simplistic, tool limitation;
- ctx_skb - simplistic, tool limitation;
- d_path - tool limitation;
- dead_code - pseudo call;
- direct_packet_access - pseudo call;
- direct_value_access - pseudo call;
- event_output - uses macro;
- jeq_infer_not_null - ok;
- jit - uses .fill_helper;
- jmp32 - tool limitation;
- jset - asm parser issue, tool limitation;
- jump - pseudo call;
- junk_insn - simplistic, libbpf discards junk;
- ld_abs - uses .fill_helper;
- ld_dw - simplistic, uses .fill_helper;
- ld_imm64 - simplistic, junk rejected by bpftool
- loops1 - small log buffer, pseudo call;
- lwt - tool limitation;
- map_in_map - ok (needs __msg update because of BPF_ST_MEM rewrite);
- map_kptr - strange runtime issue, requires further investigation;
- map_ptr_mixing - tool limitation;
- perf_event_sample_period - simplistic, tool limitation;
- precise - pseudo call, needs __msg update because of BPF_ST_MEM rewrite;
- prevent_map_lookup - tool limitation;
- ref_tracking - tool limitation;
- regalloc - pseudo call
- runtime_jit - tool limitation;
- scale - simplistic, uses .fill_helper;
- search_pruning - tool limitation;
- sleepable - simplistic, tool limitation;
- sock - ok (needs __msg update because of BPF_ST_MEM rewrite);
- spin_lock - pseudo call;
- subreg - tool limitation;
- unpriv - tool limitation;
- value_illegal_alu - tool limitation;
- value_ptr_arith - tool limitation;
- wide_access - simplistic, uses macro;
Thanks,
Eduard
