Re: [PATCH bpf v2 2/2] bpf: new verifier tests for stack access

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2023-11-27 at 22:15 -0500, Andrei Matei wrote:
> On Mon, Nov 27, 2023 at 8:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Sat, 2023-11-25 at 20:50 -0500, Andrei Matei wrote:
> > > This patch adds tests for the previous patch, checking the tracking of
> > > the maximum stack size and checking that accesses to uninit stack memory
> > > are allowed.
> > > 
> > > They are a separate patch for review purposes; whoever merges them can
> > > consider squashing.
> > > 
> > > Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> > > ---
> > 
> > I think the strategy now is to add new tests using inline assembly,
> > e.g. as a part of verifier_* tests in test_progs.
> 
> Thanks, I'll try that. I see in some of the verifier tests that you
> have converted to test_progs hints that they were converted
> "automatically". I'm curious what tool you've used, if any.

I wrote a small tree-sitter based tool for this:
https://github.com/eddyz87/verifier-tests-migrator

> Do you have any thoughts on how a test could assert the maximum stack
> depth? test_verifier has access to the verifier verifier log and
> parses it out of there; do you know how I could achieve the same in
> test_progs?

Could be done like in the patch below (set log level so that stack
depth is printed, match stack depth message):

diff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
index 069c3f91705c..e85ac95c8dd3 100644
--- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
@@ -27,7 +27,7 @@ __naked void stack_out_of_bounds(void)
 
 SEC("socket")
 __description("uninitialized stack1")
-__success
+__success __log_level(4) __msg("stack depth 8")
 __failure_unpriv __msg_unpriv("invalid indirect read from stack")
 __naked void uninitialized_stack1(void)
 {

> For the curiosity of someone who doesn't know  much about this code
> base - how come we moved from test_verifier, which seems a bit more
> unit-test-y, do the higher level test_progs? Is it because of the
> nicer assembly syntax?

Yes, ability to use assembly syntax is the main (sole?) driver.
In fact, tests that use annotations from bpf_misc.h and are registered
in tools/testing/selftests/bpf/prog_tests/verifier.c provide almost
the same functionality as test_verifier:
- interface:
  - select test to run using filter, e.g.:
    ./test_progs -a 'verifier_basic_stack/uninitialized stack1'
    ./test_progs -a 'verifier_basic_stack/uninitialized stack1 @unpriv'
  - see verifier log for the test, e.g.:
    ./test_progs -vvv -a 'verifier_basic_stack/uninitialized stack1'
- test expectations:
  - use __success / __failure to mark expected test outcome;
  - use __msg to search for expected messages in the log;
  - use __log_level to specify log level when program is loaded;
  - use __flags to enable additional knobs, e.g. BPF_F_TEST_STATE_FREQ;
  - use __retval if test program has to be executed;
  - use _unpriv suffix to specify any of the above for when test is
    executed in unprivileged mode.
  See comments in tools/testing/selftests/bpf/progs/bpf_misc.h for details.
- plus clang generates BTF and CO-RE relocations for us,
  with test_verifier one has to write these things "by hand"
  (which is only truly needed in tests for CO-RE/BTF).

The only drawback is compilation time, because all test_progs/*.c
files depend on all *.bpf.o files. Locally I mitigate this by using
ccache: make CC='ccache clang' LLVM=1 -j14 test_progs .
I probably need to look at what could be done in the makefile one day.

Unfortunately neither test_verifier nor test_progs are true unit tests.