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

Message ID	20230325025524.144043-1-eddyz87@gmail.com (mailing list archive)
Headers	show Return-Path: <bpf-owner@vger.kernel.org> From: Eduard Zingerman <eddyz87@gmail.com> To: bpf@vger.kernel.org, ast@kernel.org Cc: andrii@kernel.org, daniel@iogearbox.net, martin.lau@linux.dev, kernel-team@fb.com, yhs@fb.com, Eduard Zingerman <eddyz87@gmail.com> Subject: [PATCH bpf-next 00/43] First set of verifier/*.c migrated to inline assembly Date: Sat, 25 Mar 2023 04:54:41 +0200 Message-Id: <20230325025524.144043-1-eddyz87@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	First set of verifier/.c migrated to inline assembly \| expand [bpf-next,00/43] First set of verifier/.c migrated to inline assembly [bpf-next,01/43] selftests/bpf: Report program name on parse_test_spec error [bpf-next,02/43] selftests/bpf: __imm_insn & __imm_const macro for bpf_misc.h [bpf-next,03/43] selftests/bpf: Unprivileged tests for test_loader.c [bpf-next,04/43] selftests/bpf: Tests execution support for test_loader.c [bpf-next,05/43] selftests/bpf: prog_tests entry point for migrated test_verifier tests [bpf-next,06/43] selftests/bpf: verifier/and.c converted to inline assembly [bpf-next,07/43] selftests/bpf: verifier/array_access.c converted to inline assembly [bpf-next,08/43] selftests/bpf: verifier/basic_stack.c converted to inline assembly [bpf-next,09/43] selftests/bpf: verifier/bounds_deduction.c converted to inline assembly [bpf-next,10/43] selftests/bpf: verifier/bounds_mix_sign_unsign.c converted to inline assembly [bpf-next,11/43] selftests/bpf: verifier/cfg.c converted to inline assembly [bpf-next,12/43] selftests/bpf: verifier/cgroup_inv_retcode.c converted to inline assembly [bpf-next,13/43] selftests/bpf: verifier/cgroup_skb.c converted to inline assembly [bpf-next,14/43] selftests/bpf: verifier/cgroup_storage.c converted to inline assembly [bpf-next,15/43] selftests/bpf: verifier/const_or.c converted to inline assembly [bpf-next,16/43] selftests/bpf: verifier/ctx_sk_msg.c converted to inline assembly [bpf-next,17/43] selftests/bpf: verifier/direct_stack_access_wraparound.c converted to inline assem… [bpf-next,18/43] selftests/bpf: verifier/div0.c converted to inline assembly [bpf-next,19/43] selftests/bpf: verifier/div_overflow.c converted to inline assembly [bpf-next,20/43] selftests/bpf: verifier/helper_access_var_len.c converted to inline assembly [bpf-next,21/43] selftests/bpf: verifier/helper_packet_access.c converted to inline assembly [bpf-next,22/43] selftests/bpf: verifier/helper_restricted.c converted to inline assembly [bpf-next,23/43] selftests/bpf: verifier/helper_value_access.c converted to inline assembly [bpf-next,24/43] selftests/bpf: verifier/int_ptr.c converted to inline assembly [bpf-next,25/43] selftests/bpf: verifier/ld_ind.c converted to inline assembly [bpf-next,26/43] selftests/bpf: verifier/leak_ptr.c converted to inline assembly [bpf-next,27/43] selftests/bpf: verifier/map_ptr.c converted to inline assembly [bpf-next,28/43] selftests/bpf: verifier/map_ret_val.c converted to inline assembly [bpf-next,29/43] selftests/bpf: verifier/masking.c converted to inline assembly [bpf-next,30/43] selftests/bpf: verifier/meta_access.c converted to inline assembly [bpf-next,31/43] selftests/bpf: verifier/raw_stack.c converted to inline assembly [bpf-next,32/43] selftests/bpf: verifier/raw_tp_writable.c converted to inline assembly [bpf-next,33/43] selftests/bpf: verifier/ringbuf.c converted to inline assembly [bpf-next,34/43] selftests/bpf: verifier/spill_fill.c converted to inline assembly [bpf-next,35/43] selftests/bpf: verifier/stack_ptr.c converted to inline assembly [bpf-next,36/43] selftests/bpf: verifier/uninit.c converted to inline assembly [bpf-next,37/43] selftests/bpf: verifier/value_adj_spill.c converted to inline assembly [bpf-next,38/43] selftests/bpf: verifier/value.c converted to inline assembly [bpf-next,39/43] selftests/bpf: verifier/value_or_null.c converted to inline assembly [bpf-next,40/43] selftests/bpf: verifier/var_off.c converted to inline assembly [bpf-next,41/43] selftests/bpf: verifier/xadd.c converted to inline assembly [bpf-next,42/43] selftests/bpf: verifier/xdp.c converted to inline assembly

Eduard Zingerman March 25, 2023, 2:54 a.m. UTC

This is a follow up for RFC [1]. It migrates a first batch of 38
verifier/*.c tests to inline assembly and use of ./test_progs for
actual execution. The migration is done by a python script (see [2]).

Each migrated verifier/xxx.c file is mapped to progs/verifier_xxx.c
plus an entry in the prog_tests/verifier.c. One patch per each file.

A few patches at the beginning of the patch-set extend test_loader
with necessary functionality, mainly:
- support for tests execution in unprivileged mode;
- support for test runs for test programs.

Migrated tests could be selected for execution using the following filter:

  ./test_progs -a verifier_*
  
An example of the migrated test:

  SEC("xdp")
  __description("XDP pkt read, pkt_data' > pkt_end, corner case, good access")
  __success __retval(0) __flag(BPF_F_ANY_ALIGNMENT)
  __naked void end_corner_case_good_access_1(void)
  {
          asm volatile ("                                 \
          r2 = *(u32*)(r1 + %[xdp_md_data]);              \
          r3 = *(u32*)(r1 + %[xdp_md_data_end]);          \
          r1 = r2;                                        \
          r1 += 8;                                        \
          if r1 > r3 goto l0_%=;                          \
          r0 = *(u64*)(r1 - 8);                           \
  l0_%=:  r0 = 0;                                         \
          exit;                                           \
  "       :
          : __imm_const(xdp_md_data, offsetof(struct xdp_md, data)),
            __imm_const(xdp_md_data_end, offsetof(struct xdp_md, data_end))
          : __clobber_all);
  }

Changes compared to RFC:
- test_loader.c is extended to support test program runs;
- capabilities handling now matches behavior of test_verifier;
- BPF_ST_MEM instructions are automatically replaced by BPF_STX_MEM
  instructions to overcome current clang limitations;
- tests styling updates according to RFC feedback;
- 38 migrated files are included instead of 1.

I used the following means for testing:
- migration tool itself has a set of self-tests;
- migrated tests are passing;
- manually compared each old/new file side-by-side.

While doing side-by-side comparison I've noted a few defects in the
original tests:
- and.c:
  - One of the jump targets is off by one;
  - BPF_ST_MEM wrong OFF/IMM ordering;
- array_access.c:
  - BPF_ST_MEM wrong OFF/IMM ordering;
- value_or_null.c:
  - BPF_ST_MEM wrong OFF/IMM ordering.

These defects would be addressed separately.

[1] RFC
    https://lore.kernel.org/bpf/20230123145148.2791939-1-eddyz87@gmail.com/
[2] Migration tool
    https://github.com/eddyz87/verifier-tests-migrator

Eduard Zingerman (43):
  selftests/bpf: Report program name on parse_test_spec error
  selftests/bpf: __imm_insn & __imm_const macro for bpf_misc.h
  selftests/bpf: Unprivileged tests for test_loader.c
  selftests/bpf: Tests execution support for test_loader.c
  selftests/bpf: prog_tests entry point for migrated test_verifier tests
  selftests/bpf: verifier/and.c converted to inline assembly
  selftests/bpf: verifier/array_access.c converted to inline assembly
  selftests/bpf: verifier/basic_stack.c converted to inline assembly
  selftests/bpf: verifier/bounds_deduction.c converted to inline
    assembly
  selftests/bpf: verifier/bounds_mix_sign_unsign.c converted to inline
    assembly
  selftests/bpf: verifier/cfg.c converted to inline assembly
  selftests/bpf: verifier/cgroup_inv_retcode.c converted to inline
    assembly
  selftests/bpf: verifier/cgroup_skb.c converted to inline assembly
  selftests/bpf: verifier/cgroup_storage.c converted to inline assembly
  selftests/bpf: verifier/const_or.c converted to inline assembly
  selftests/bpf: verifier/ctx_sk_msg.c converted to inline assembly
  selftests/bpf: verifier/direct_stack_access_wraparound.c converted to
    inline assembly
  selftests/bpf: verifier/div0.c converted to inline assembly
  selftests/bpf: verifier/div_overflow.c converted to inline assembly
  selftests/bpf: verifier/helper_access_var_len.c converted to inline
    assembly
  selftests/bpf: verifier/helper_packet_access.c converted to inline
    assembly
  selftests/bpf: verifier/helper_restricted.c converted to inline
    assembly
  selftests/bpf: verifier/helper_value_access.c converted to inline
    assembly
  selftests/bpf: verifier/int_ptr.c converted to inline assembly
  selftests/bpf: verifier/ld_ind.c converted to inline assembly
  selftests/bpf: verifier/leak_ptr.c converted to inline assembly
  selftests/bpf: verifier/map_ptr.c converted to inline assembly
  selftests/bpf: verifier/map_ret_val.c converted to inline assembly
  selftests/bpf: verifier/masking.c converted to inline assembly
  selftests/bpf: verifier/meta_access.c converted to inline assembly
  selftests/bpf: verifier/raw_stack.c converted to inline assembly
  selftests/bpf: verifier/raw_tp_writable.c converted to inline assembly
  selftests/bpf: verifier/ringbuf.c converted to inline assembly
  selftests/bpf: verifier/spill_fill.c converted to inline assembly
  selftests/bpf: verifier/stack_ptr.c converted to inline assembly
  selftests/bpf: verifier/uninit.c converted to inline assembly
  selftests/bpf: verifier/value_adj_spill.c converted to inline assembly
  selftests/bpf: verifier/value.c converted to inline assembly
  selftests/bpf: verifier/value_or_null.c converted to inline assembly
  selftests/bpf: verifier/var_off.c converted to inline assembly
  selftests/bpf: verifier/xadd.c converted to inline assembly
  selftests/bpf: verifier/xdp.c converted to inline assembly
  selftests/bpf: verifier/xdp_direct_packet_access.c converted to inline
    assembly

 tools/testing/selftests/bpf/Makefile          |   10 +-
 tools/testing/selftests/bpf/autoconf_helper.h |    9 +
 .../selftests/bpf/prog_tests/verifier.c       |  106 +
 tools/testing/selftests/bpf/progs/bpf_misc.h  |   42 +
 .../selftests/bpf/progs/verifier_and.c        |  107 +
 .../bpf/progs/verifier_array_access.c         |  529 +++++
 .../bpf/progs/verifier_basic_stack.c          |  100 +
 .../bpf/progs/verifier_bounds_deduction.c     |  171 ++
 .../progs/verifier_bounds_mix_sign_unsign.c   |  554 ++++++
 .../selftests/bpf/progs/verifier_cfg.c        |  100 +
 .../bpf/progs/verifier_cgroup_inv_retcode.c   |   89 +
 .../selftests/bpf/progs/verifier_cgroup_skb.c |  227 +++
 .../bpf/progs/verifier_cgroup_storage.c       |  308 +++
 .../selftests/bpf/progs/verifier_const_or.c   |   82 +
 .../selftests/bpf/progs/verifier_ctx_sk_msg.c |  228 +++
 .../verifier_direct_stack_access_wraparound.c |   56 +
 .../selftests/bpf/progs/verifier_div0.c       |  213 ++
 .../bpf/progs/verifier_div_overflow.c         |  144 ++
 .../progs/verifier_helper_access_var_len.c    |  825 ++++++++
 .../bpf/progs/verifier_helper_packet_access.c |  550 ++++++
 .../bpf/progs/verifier_helper_restricted.c    |  279 +++
 .../bpf/progs/verifier_helper_value_access.c  | 1245 ++++++++++++
 .../selftests/bpf/progs/verifier_int_ptr.c    |  157 ++
 .../selftests/bpf/progs/verifier_ld_ind.c     |  110 ++
 .../selftests/bpf/progs/verifier_leak_ptr.c   |   92 +
 .../selftests/bpf/progs/verifier_map_ptr.c    |  159 ++
 .../bpf/progs/verifier_map_ret_val.c          |  110 ++
 .../selftests/bpf/progs/verifier_masking.c    |  410 ++++
 .../bpf/progs/verifier_meta_access.c          |  284 +++
 .../selftests/bpf/progs/verifier_raw_stack.c  |  371 ++++
 .../bpf/progs/verifier_raw_tp_writable.c      |   50 +
 .../selftests/bpf/progs/verifier_ringbuf.c    |  131 ++
 .../selftests/bpf/progs/verifier_spill_fill.c |  374 ++++
 .../selftests/bpf/progs/verifier_stack_ptr.c  |  484 +++++
 .../selftests/bpf/progs/verifier_uninit.c     |   61 +
 .../selftests/bpf/progs/verifier_value.c      |  158 ++
 .../bpf/progs/verifier_value_adj_spill.c      |   78 +
 .../bpf/progs/verifier_value_or_null.c        |  288 +++
 .../selftests/bpf/progs/verifier_var_off.c    |  349 ++++
 .../selftests/bpf/progs/verifier_xadd.c       |  124 ++
 .../selftests/bpf/progs/verifier_xdp.c        |   24 +
 .../progs/verifier_xdp_direct_packet_access.c | 1722 +++++++++++++++++
 tools/testing/selftests/bpf/test_loader.c     |  536 ++++-
 tools/testing/selftests/bpf/test_verifier.c   |   25 +-
 tools/testing/selftests/bpf/unpriv_helpers.c  |   26 +
 tools/testing/selftests/bpf/unpriv_helpers.h  |    7 +
 tools/testing/selftests/bpf/verifier/and.c    |   68 -
 .../selftests/bpf/verifier/array_access.c     |  379 ----
 .../selftests/bpf/verifier/basic_stack.c      |   64 -
 .../selftests/bpf/verifier/bounds_deduction.c |  136 --
 .../bpf/verifier/bounds_mix_sign_unsign.c     |  411 ----
 tools/testing/selftests/bpf/verifier/cfg.c    |   73 -
 .../bpf/verifier/cgroup_inv_retcode.c         |   72 -
 .../selftests/bpf/verifier/cgroup_skb.c       |  197 --
 .../selftests/bpf/verifier/cgroup_storage.c   |  220 ---
 .../testing/selftests/bpf/verifier/const_or.c |   60 -
 .../selftests/bpf/verifier/ctx_sk_msg.c       |  181 --
 .../verifier/direct_stack_access_wraparound.c |   40 -
 tools/testing/selftests/bpf/verifier/div0.c   |  184 --
 .../selftests/bpf/verifier/div_overflow.c     |  110 --
 .../bpf/verifier/helper_access_var_len.c      |  650 -------
 .../bpf/verifier/helper_packet_access.c       |  460 -----
 .../bpf/verifier/helper_restricted.c          |  196 --
 .../bpf/verifier/helper_value_access.c        |  953 ---------
 .../testing/selftests/bpf/verifier/int_ptr.c  |  161 --
 tools/testing/selftests/bpf/verifier/ld_ind.c |   72 -
 .../testing/selftests/bpf/verifier/leak_ptr.c |   67 -
 .../testing/selftests/bpf/verifier/map_ptr.c  |   99 -
 .../selftests/bpf/verifier/map_ret_val.c      |   65 -
 .../testing/selftests/bpf/verifier/masking.c  |  322 ---
 .../selftests/bpf/verifier/meta_access.c      |  235 ---
 .../selftests/bpf/verifier/raw_stack.c        |  305 ---
 .../selftests/bpf/verifier/raw_tp_writable.c  |   35 -
 .../testing/selftests/bpf/verifier/ringbuf.c  |   95 -
 .../selftests/bpf/verifier/spill_fill.c       |  345 ----
 .../selftests/bpf/verifier/stack_ptr.c        |  359 ----
 tools/testing/selftests/bpf/verifier/uninit.c |   39 -
 tools/testing/selftests/bpf/verifier/value.c  |  104 -
 .../selftests/bpf/verifier/value_adj_spill.c  |   43 -
 .../selftests/bpf/verifier/value_or_null.c    |  220 ---
 .../testing/selftests/bpf/verifier/var_off.c  |  291 ---
 tools/testing/selftests/bpf/verifier/xadd.c   |   97 -
 tools/testing/selftests/bpf/verifier/xdp.c    |   14 -
 .../bpf/verifier/xdp_direct_packet_access.c   | 1468 --------------
 84 files changed, 11994 insertions(+), 9000 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/autoconf_helper.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/verifier.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_and.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_array_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_basic_stack.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_bounds_deduction.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_bounds_mix_sign_unsign.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_cfg.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_cgroup_skb.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_cgroup_storage.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_const_or.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_ctx_sk_msg.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_direct_stack_access_wraparound.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_div0.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_div_overflow.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_helper_access_var_len.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_helper_packet_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_helper_restricted.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_helper_value_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_int_ptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_ld_ind.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_leak_ptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_map_ptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_map_ret_val.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_masking.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_meta_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_raw_stack.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_raw_tp_writable.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_ringbuf.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_spill_fill.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_uninit.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_value.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_value_adj_spill.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_value_or_null.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_var_off.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_xadd.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_xdp.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_xdp_direct_packet_access.c
 create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.c
 create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.h
 delete mode 100644 tools/testing/selftests/bpf/verifier/and.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/array_access.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/basic_stack.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/bounds_deduction.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/bounds_mix_sign_unsign.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/cfg.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/cgroup_inv_retcode.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/cgroup_skb.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/cgroup_storage.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/const_or.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/ctx_sk_msg.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/direct_stack_access_wraparound.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/div0.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/div_overflow.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/helper_access_var_len.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/helper_packet_access.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/helper_restricted.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/helper_value_access.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/int_ptr.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/ld_ind.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/leak_ptr.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/map_ptr.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/map_ret_val.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/masking.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/meta_access.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/raw_stack.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/raw_tp_writable.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/ringbuf.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/spill_fill.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/stack_ptr.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/uninit.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/value.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/value_adj_spill.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/value_or_null.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/var_off.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/xadd.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/xdp.c
 delete mode 100644 tools/testing/selftests/bpf/verifier/xdp_direct_packet_access.c

Stanislav Fomichev March 25, 2023, 3:23 a.m. UTC | #1

On 03/25, Eduard Zingerman wrote:
> This is a follow up for RFC [1]. It migrates a first batch of 38
> verifier/*.c tests to inline assembly and use of ./test_progs for
> actual execution. The migration is done by a python script (see [2]).

Jesus Christ, 43 patches on a Friday night (^_^)
Unless I get bored on the weekend, will get to them Monday morning
(or unless Alexei/Andrii beat me to it; I see they were commenting
on the RFC).

> Each migrated verifier/xxx.c file is mapped to progs/verifier_xxx.c
> plus an entry in the prog_tests/verifier.c. One patch per each file.

> A few patches at the beginning of the patch-set extend test_loader
> with necessary functionality, mainly:
> - support for tests execution in unprivileged mode;
> - support for test runs for test programs.

> Migrated tests could be selected for execution using the following filter:

>    ./test_progs -a verifier_*

> An example of the migrated test:

>    SEC("xdp")
>    __description("XDP pkt read, pkt_data' > pkt_end, corner case, good  
> access")
>    __success __retval(0) __flag(BPF_F_ANY_ALIGNMENT)
>    __naked void end_corner_case_good_access_1(void)
>    {
>            asm volatile ("                                 \
>            r2 = *(u32*)(r1 + %[xdp_md_data]);              \
>            r3 = *(u32*)(r1 + %[xdp_md_data_end]);          \
>            r1 = r2;                                        \
>            r1 += 8;                                        \
>            if r1 > r3 goto l0_%=;                          \
>            r0 = *(u64*)(r1 - 8);                           \
>    l0_%=:  r0 = 0;                                         \
>            exit;                                           \
>    "       :
>            : __imm_const(xdp_md_data, offsetof(struct xdp_md, data)),
>              __imm_const(xdp_md_data_end, offsetof(struct xdp_md,  
> data_end))
>            : __clobber_all);
>    }

Are those '\' at the end required? Can we do regular string coalescing
like the following below?

asm volatile(
	"r2 = *(u32*)(r1 + %[xdp_md_data]);"
	"r3 = *(u32*)(r1 + %[xdp_md_data_end]);"
	"r1 = r2;"
	...
);

Or is asm directive somehow special?

> Changes compared to RFC:
> - test_loader.c is extended to support test program runs;
> - capabilities handling now matches behavior of test_verifier;
> - BPF_ST_MEM instructions are automatically replaced by BPF_STX_MEM
>    instructions to overcome current clang limitations;
> - tests styling updates according to RFC feedback;
> - 38 migrated files are included instead of 1.

> I used the following means for testing:
> - migration tool itself has a set of self-tests;
> - migrated tests are passing;
> - manually compared each old/new file side-by-side.

> While doing side-by-side comparison I've noted a few defects in the
> original tests:
> - and.c:
>    - One of the jump targets is off by one;
>    - BPF_ST_MEM wrong OFF/IMM ordering;
> - array_access.c:
>    - BPF_ST_MEM wrong OFF/IMM ordering;
> - value_or_null.c:
>    - BPF_ST_MEM wrong OFF/IMM ordering.

> These defects would be addressed separately.

> [1] RFC
>       
> https://lore.kernel.org/bpf/20230123145148.2791939-1-eddyz87@gmail.com/
> [2] Migration tool
>      https://github.com/eddyz87/verifier-tests-migrator

> Eduard Zingerman (43):
>    selftests/bpf: Report program name on parse_test_spec error
>    selftests/bpf: __imm_insn & __imm_const macro for bpf_misc.h
>    selftests/bpf: Unprivileged tests for test_loader.c
>    selftests/bpf: Tests execution support for test_loader.c
>    selftests/bpf: prog_tests entry point for migrated test_verifier tests
>    selftests/bpf: verifier/and.c converted to inline assembly
>    selftests/bpf: verifier/array_access.c converted to inline assembly
>    selftests/bpf: verifier/basic_stack.c converted to inline assembly
>    selftests/bpf: verifier/bounds_deduction.c converted to inline
>      assembly
>    selftests/bpf: verifier/bounds_mix_sign_unsign.c converted to inline
>      assembly
>    selftests/bpf: verifier/cfg.c converted to inline assembly
>    selftests/bpf: verifier/cgroup_inv_retcode.c converted to inline
>      assembly
>    selftests/bpf: verifier/cgroup_skb.c converted to inline assembly
>    selftests/bpf: verifier/cgroup_storage.c converted to inline assembly
>    selftests/bpf: verifier/const_or.c converted to inline assembly
>    selftests/bpf: verifier/ctx_sk_msg.c converted to inline assembly
>    selftests/bpf: verifier/direct_stack_access_wraparound.c converted to
>      inline assembly
>    selftests/bpf: verifier/div0.c converted to inline assembly
>    selftests/bpf: verifier/div_overflow.c converted to inline assembly
>    selftests/bpf: verifier/helper_access_var_len.c converted to inline
>      assembly
>    selftests/bpf: verifier/helper_packet_access.c converted to inline
>      assembly
>    selftests/bpf: verifier/helper_restricted.c converted to inline
>      assembly
>    selftests/bpf: verifier/helper_value_access.c converted to inline
>      assembly
>    selftests/bpf: verifier/int_ptr.c converted to inline assembly
>    selftests/bpf: verifier/ld_ind.c converted to inline assembly
>    selftests/bpf: verifier/leak_ptr.c converted to inline assembly
>    selftests/bpf: verifier/map_ptr.c converted to inline assembly
>    selftests/bpf: verifier/map_ret_val.c converted to inline assembly
>    selftests/bpf: verifier/masking.c converted to inline assembly
>    selftests/bpf: verifier/meta_access.c converted to inline assembly
>    selftests/bpf: verifier/raw_stack.c converted to inline assembly
>    selftests/bpf: verifier/raw_tp_writable.c converted to inline assembly
>    selftests/bpf: verifier/ringbuf.c converted to inline assembly
>    selftests/bpf: verifier/spill_fill.c converted to inline assembly
>    selftests/bpf: verifier/stack_ptr.c converted to inline assembly
>    selftests/bpf: verifier/uninit.c converted to inline assembly
>    selftests/bpf: verifier/value_adj_spill.c converted to inline assembly
>    selftests/bpf: verifier/value.c converted to inline assembly
>    selftests/bpf: verifier/value_or_null.c converted to inline assembly
>    selftests/bpf: verifier/var_off.c converted to inline assembly
>    selftests/bpf: verifier/xadd.c converted to inline assembly
>    selftests/bpf: verifier/xdp.c converted to inline assembly
>    selftests/bpf: verifier/xdp_direct_packet_access.c converted to inline
>      assembly

>   tools/testing/selftests/bpf/Makefile          |   10 +-
>   tools/testing/selftests/bpf/autoconf_helper.h |    9 +
>   .../selftests/bpf/prog_tests/verifier.c       |  106 +
>   tools/testing/selftests/bpf/progs/bpf_misc.h  |   42 +
>   .../selftests/bpf/progs/verifier_and.c        |  107 +
>   .../bpf/progs/verifier_array_access.c         |  529 +++++
>   .../bpf/progs/verifier_basic_stack.c          |  100 +
>   .../bpf/progs/verifier_bounds_deduction.c     |  171 ++
>   .../progs/verifier_bounds_mix_sign_unsign.c   |  554 ++++++
>   .../selftests/bpf/progs/verifier_cfg.c        |  100 +
>   .../bpf/progs/verifier_cgroup_inv_retcode.c   |   89 +
>   .../selftests/bpf/progs/verifier_cgroup_skb.c |  227 +++
>   .../bpf/progs/verifier_cgroup_storage.c       |  308 +++
>   .../selftests/bpf/progs/verifier_const_or.c   |   82 +
>   .../selftests/bpf/progs/verifier_ctx_sk_msg.c |  228 +++
>   .../verifier_direct_stack_access_wraparound.c |   56 +
>   .../selftests/bpf/progs/verifier_div0.c       |  213 ++
>   .../bpf/progs/verifier_div_overflow.c         |  144 ++
>   .../progs/verifier_helper_access_var_len.c    |  825 ++++++++
>   .../bpf/progs/verifier_helper_packet_access.c |  550 ++++++
>   .../bpf/progs/verifier_helper_restricted.c    |  279 +++
>   .../bpf/progs/verifier_helper_value_access.c  | 1245 ++++++++++++
>   .../selftests/bpf/progs/verifier_int_ptr.c    |  157 ++
>   .../selftests/bpf/progs/verifier_ld_ind.c     |  110 ++
>   .../selftests/bpf/progs/verifier_leak_ptr.c   |   92 +
>   .../selftests/bpf/progs/verifier_map_ptr.c    |  159 ++
>   .../bpf/progs/verifier_map_ret_val.c          |  110 ++
>   .../selftests/bpf/progs/verifier_masking.c    |  410 ++++
>   .../bpf/progs/verifier_meta_access.c          |  284 +++
>   .../selftests/bpf/progs/verifier_raw_stack.c  |  371 ++++
>   .../bpf/progs/verifier_raw_tp_writable.c      |   50 +
>   .../selftests/bpf/progs/verifier_ringbuf.c    |  131 ++
>   .../selftests/bpf/progs/verifier_spill_fill.c |  374 ++++
>   .../selftests/bpf/progs/verifier_stack_ptr.c  |  484 +++++
>   .../selftests/bpf/progs/verifier_uninit.c     |   61 +
>   .../selftests/bpf/progs/verifier_value.c      |  158 ++
>   .../bpf/progs/verifier_value_adj_spill.c      |   78 +
>   .../bpf/progs/verifier_value_or_null.c        |  288 +++
>   .../selftests/bpf/progs/verifier_var_off.c    |  349 ++++
>   .../selftests/bpf/progs/verifier_xadd.c       |  124 ++
>   .../selftests/bpf/progs/verifier_xdp.c        |   24 +
>   .../progs/verifier_xdp_direct_packet_access.c | 1722 +++++++++++++++++
>   tools/testing/selftests/bpf/test_loader.c     |  536 ++++-
>   tools/testing/selftests/bpf/test_verifier.c   |   25 +-
>   tools/testing/selftests/bpf/unpriv_helpers.c  |   26 +
>   tools/testing/selftests/bpf/unpriv_helpers.h  |    7 +
>   tools/testing/selftests/bpf/verifier/and.c    |   68 -
>   .../selftests/bpf/verifier/array_access.c     |  379 ----
>   .../selftests/bpf/verifier/basic_stack.c      |   64 -
>   .../selftests/bpf/verifier/bounds_deduction.c |  136 --
>   .../bpf/verifier/bounds_mix_sign_unsign.c     |  411 ----
>   tools/testing/selftests/bpf/verifier/cfg.c    |   73 -
>   .../bpf/verifier/cgroup_inv_retcode.c         |   72 -
>   .../selftests/bpf/verifier/cgroup_skb.c       |  197 --
>   .../selftests/bpf/verifier/cgroup_storage.c   |  220 ---
>   .../testing/selftests/bpf/verifier/const_or.c |   60 -
>   .../selftests/bpf/verifier/ctx_sk_msg.c       |  181 --
>   .../verifier/direct_stack_access_wraparound.c |   40 -
>   tools/testing/selftests/bpf/verifier/div0.c   |  184 --
>   .../selftests/bpf/verifier/div_overflow.c     |  110 --
>   .../bpf/verifier/helper_access_var_len.c      |  650 -------
>   .../bpf/verifier/helper_packet_access.c       |  460 -----
>   .../bpf/verifier/helper_restricted.c          |  196 --
>   .../bpf/verifier/helper_value_access.c        |  953 ---------
>   .../testing/selftests/bpf/verifier/int_ptr.c  |  161 --
>   tools/testing/selftests/bpf/verifier/ld_ind.c |   72 -
>   .../testing/selftests/bpf/verifier/leak_ptr.c |   67 -
>   .../testing/selftests/bpf/verifier/map_ptr.c  |   99 -
>   .../selftests/bpf/verifier/map_ret_val.c      |   65 -
>   .../testing/selftests/bpf/verifier/masking.c  |  322 ---
>   .../selftests/bpf/verifier/meta_access.c      |  235 ---
>   .../selftests/bpf/verifier/raw_stack.c        |  305 ---
>   .../selftests/bpf/verifier/raw_tp_writable.c  |   35 -
>   .../testing/selftests/bpf/verifier/ringbuf.c  |   95 -
>   .../selftests/bpf/verifier/spill_fill.c       |  345 ----
>   .../selftests/bpf/verifier/stack_ptr.c        |  359 ----
>   tools/testing/selftests/bpf/verifier/uninit.c |   39 -
>   tools/testing/selftests/bpf/verifier/value.c  |  104 -
>   .../selftests/bpf/verifier/value_adj_spill.c  |   43 -
>   .../selftests/bpf/verifier/value_or_null.c    |  220 ---
>   .../testing/selftests/bpf/verifier/var_off.c  |  291 ---
>   tools/testing/selftests/bpf/verifier/xadd.c   |   97 -
>   tools/testing/selftests/bpf/verifier/xdp.c    |   14 -
>   .../bpf/verifier/xdp_direct_packet_access.c   | 1468 --------------
>   84 files changed, 11994 insertions(+), 9000 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/autoconf_helper.h
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/verifier.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_and.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_array_access.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_basic_stack.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_bounds_deduction.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_bounds_mix_sign_unsign.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_cfg.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_cgroup_skb.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_cgroup_storage.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_const_or.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_ctx_sk_msg.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_direct_stack_access_wraparound.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_div0.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_div_overflow.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_helper_access_var_len.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_helper_packet_access.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_helper_restricted.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_helper_value_access.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_int_ptr.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_ld_ind.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_leak_ptr.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_map_ptr.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_map_ret_val.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_masking.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_meta_access.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_raw_stack.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_raw_tp_writable.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_ringbuf.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_spill_fill.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_uninit.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_value.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_value_adj_spill.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_value_or_null.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_var_off.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_xadd.c
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_xdp.c
>   create mode 100644  
> tools/testing/selftests/bpf/progs/verifier_xdp_direct_packet_access.c
>   create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.c
>   create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.h
>   delete mode 100644 tools/testing/selftests/bpf/verifier/and.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/array_access.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/basic_stack.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/bounds_deduction.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/bounds_mix_sign_unsign.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/cfg.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/cgroup_inv_retcode.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/cgroup_skb.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/cgroup_storage.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/const_or.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/ctx_sk_msg.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/direct_stack_access_wraparound.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/div0.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/div_overflow.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/helper_access_var_len.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/helper_packet_access.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/helper_restricted.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/helper_value_access.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/int_ptr.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/ld_ind.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/leak_ptr.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/map_ptr.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/map_ret_val.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/masking.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/meta_access.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/raw_stack.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/raw_tp_writable.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/ringbuf.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/spill_fill.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/stack_ptr.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/uninit.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/value.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/value_adj_spill.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/value_or_null.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/var_off.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/xadd.c
>   delete mode 100644 tools/testing/selftests/bpf/verifier/xdp.c
>   delete mode 100644  
> tools/testing/selftests/bpf/verifier/xdp_direct_packet_access.c

> --
> 2.40.0

Eduard Zingerman March 25, 2023, 12:20 p.m. UTC | #2

On Fri, 2023-03-24 at 20:23 -0700, Stanislav Fomichev wrote:
> On 03/25, Eduard Zingerman wrote:
> > This is a follow up for RFC [1]. It migrates a first batch of 38
> > verifier/*.c tests to inline assembly and use of ./test_progs for
> > actual execution. The migration is done by a python script (see [2]).
> 
> Jesus Christ, 43 patches on a Friday night (^_^)
> Unless I get bored on the weekend, will get to them Monday morning
> (or unless Alexei/Andrii beat me to it; I see they were commenting
> on the RFC).

Alexei and Andrii wanted this to be organized like one patch-set with
patch per migrated file. I actually found the side-by-side comparison
process to be quite painful, took me ~1.5 days to go through all
migrated files. So, I can split this in smaller batches if that would
make others life easier.

Also the last patch:

  selftests/bpf: verifier/xdp_direct_packet_access.c converted to inline assembly

was blocked because it is too large. I'll split it in two in the v2
(but will wait for some feedback first).
 
[...]

> Are those '\' at the end required? Can we do regular string coalescing
> like the following below?
> 
> asm volatile(
> 	"r2 = *(u32*)(r1 + %[xdp_md_data]);"
> 	"r3 = *(u32*)(r1 + %[xdp_md_data_end]);"
> 	"r1 = r2;"
> 	...
> );
> 
> Or is asm directive somehow special?

Strings coalescing would work, I updated the script to support both
modes, here is an example of verifier/and.c converted this way
(caution, that test missuses BPF_ST_MEM and has a wrong jump, the
 translation is fine):

https://gist.github.com/eddyz87/8725b9140098e1311ca5186c6ee73855

It was my understanding from the RFC feedback that this "lighter" way
is preferable and we already have some tests written like that.
Don't have a strong opinion on this topic.

Pros for '\':
- it indeed looks lighter;
- labels could be "inline", like in the example above "l0_%=: r0 = 0;".
Cons for '\':
- harder to edit (although, at-least for emacs, I plan to solve this
  using https://github.com/twlz0ne/separedit.el);
- no syntax highlighting for comments.

Pros for string coalescing:
- easier to edit w/o special editor support;
- syntax highlighting for comments.
Cons for string coalescing:
- a bit harder to read;
- "inline" labels look super weird, so each labels takes a full line.

[...]

Stanislav Fomichev March 25, 2023, 4:16 p.m. UTC | #3

On Sat, Mar 25, 2023 at 5:20 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2023-03-24 at 20:23 -0700, Stanislav Fomichev wrote:
> > On 03/25, Eduard Zingerman wrote:
> > > This is a follow up for RFC [1]. It migrates a first batch of 38
> > > verifier/*.c tests to inline assembly and use of ./test_progs for
> > > actual execution. The migration is done by a python script (see [2]).
> >
> > Jesus Christ, 43 patches on a Friday night (^_^)
> > Unless I get bored on the weekend, will get to them Monday morning
> > (or unless Alexei/Andrii beat me to it; I see they were commenting
> > on the RFC).
>
> Alexei and Andrii wanted this to be organized like one patch-set with
> patch per migrated file. I actually found the side-by-side comparison
> process to be quite painful, took me ~1.5 days to go through all
> migrated files. So, I can split this in smaller batches if that would
> make others life easier.
>
> Also the last patch:
>
>   selftests/bpf: verifier/xdp_direct_packet_access.c converted to inline assembly
>
> was blocked because it is too large. I'll split it in two in the v2
> (but will wait for some feedback first).

Oh, sorry, I was joking and didn't mean this as a call to split it up.
Was mostly trying to set the expectation that I'll be slacking off on
the weekend :-)
(plus it seems like Alexei/Andrii would like a take a look anyway)

> [...]
>
> > Are those '\' at the end required? Can we do regular string coalescing
> > like the following below?
> >
> > asm volatile(
> >       "r2 = *(u32*)(r1 + %[xdp_md_data]);"
> >       "r3 = *(u32*)(r1 + %[xdp_md_data_end]);"
> >       "r1 = r2;"
> >       ...
> > );
> >
> > Or is asm directive somehow special?
>
> Strings coalescing would work, I updated the script to support both
> modes, here is an example of verifier/and.c converted this way
> (caution, that test missuses BPF_ST_MEM and has a wrong jump, the
>  translation is fine):
>
> https://gist.github.com/eddyz87/8725b9140098e1311ca5186c6ee73855
>
> It was my understanding from the RFC feedback that this "lighter" way
> is preferable and we already have some tests written like that.
> Don't have a strong opinion on this topic.

Ack, I'm obviously losing a bunch of context here :-(
I like coalescing better, but if the original suggestion was to use
this lighter way, I'll keep that in mind while reviewing.

> Pros for '\':
> - it indeed looks lighter;
> - labels could be "inline", like in the example above "l0_%=: r0 = 0;".
> Cons for '\':
> - harder to edit (although, at-least for emacs, I plan to solve this
>   using https://github.com/twlz0ne/separedit.el);
> - no syntax highlighting for comments.
>
> Pros for string coalescing:
> - easier to edit w/o special editor support;
> - syntax highlighting for comments.
> Cons for string coalescing:
> - a bit harder to read;
> - "inline" labels look super weird, so each labels takes a full line.
>
> [...]

Alexei Starovoitov March 26, 2023, 1:19 a.m. UTC | #4

On Sat, Mar 25, 2023 at 9:16 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> >
> > It was my understanding from the RFC feedback that this "lighter" way
> > is preferable and we already have some tests written like that.
> > Don't have a strong opinion on this topic.
>
> Ack, I'm obviously losing a bunch of context here :-(
> I like coalescing better, but if the original suggestion was to use
> this lighter way, I'll keep that in mind while reviewing.

I still prefer the clean look of the tests, so I've applied this set.

But I'm not going to insist that this is the only style developers
should use moving forward.
Whoever prefers "" style can use it in the future tests.
I find them harder to read, but oh well.

Ed,
the only small nit I've noticed is that the tests are compiled
for both test_progs and test_progs-no_alu32 though they're in asm.
So we're wasting a bit of CI time running them in both flavors.
Not a big deal and maybe not worth fixing, since they're pretty fast.

patchwork-bot+netdevbpf@kernel.org March 26, 2023, 1:32 a.m. UTC | #5

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Sat, 25 Mar 2023 04:54:41 +0200 you wrote:
> This is a follow up for RFC [1]. It migrates a first batch of 38
> verifier/*.c tests to inline assembly and use of ./test_progs for
> actual execution. The migration is done by a python script (see [2]).
> 
> Each migrated verifier/xxx.c file is mapped to progs/verifier_xxx.c
> plus an entry in the prog_tests/verifier.c. One patch per each file.
> 
> [...]

Here is the summary with links:
  - [bpf-next,01/43] selftests/bpf: Report program name on parse_test_spec error
    https://git.kernel.org/bpf/bpf-next/c/3e5329e193f4
  - [bpf-next,02/43] selftests/bpf: __imm_insn & __imm_const macro for bpf_misc.h
    https://git.kernel.org/bpf/bpf-next/c/207b1ba30191
  - [bpf-next,03/43] selftests/bpf: Unprivileged tests for test_loader.c
    https://git.kernel.org/bpf/bpf-next/c/1d56ade032a4
  - [bpf-next,04/43] selftests/bpf: Tests execution support for test_loader.c
    https://git.kernel.org/bpf/bpf-next/c/19a8e06f5f91
  - [bpf-next,05/43] selftests/bpf: prog_tests entry point for migrated test_verifier tests
    https://git.kernel.org/bpf/bpf-next/c/55108621a35e
  - [bpf-next,06/43] selftests/bpf: verifier/and.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/9d0f1568ad5b
  - [bpf-next,07/43] selftests/bpf: verifier/array_access.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/a3c830ae0209
  - [bpf-next,08/43] selftests/bpf: verifier/basic_stack.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/0ccbe4956d6c
  - [bpf-next,09/43] selftests/bpf: verifier/bounds_deduction.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/7605f94b3492
  - [bpf-next,10/43] selftests/bpf: verifier/bounds_mix_sign_unsign.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/b14a702afd0d
  - [bpf-next,11/43] selftests/bpf: verifier/cfg.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/2f2047c22cda
  - [bpf-next,12/43] selftests/bpf: verifier/cgroup_inv_retcode.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/047687a7f494
  - [bpf-next,13/43] selftests/bpf: verifier/cgroup_skb.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/b1b6372535c0
  - [bpf-next,14/43] selftests/bpf: verifier/cgroup_storage.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/8f16f3c07e46
  - [bpf-next,15/43] selftests/bpf: verifier/const_or.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/a2777eaad5d9
  - [bpf-next,16/43] selftests/bpf: verifier/ctx_sk_msg.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/a58475a98903
  - [bpf-next,17/43] selftests/bpf: verifier/direct_stack_access_wraparound.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/84988478fb2c
  - [bpf-next,18/43] selftests/bpf: verifier/div0.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/01a0925531a4
  - [bpf-next,19/43] selftests/bpf: verifier/div_overflow.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/9553de70a841
  - [bpf-next,20/43] selftests/bpf: verifier/helper_access_var_len.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/b37d776b431e
  - [bpf-next,21/43] selftests/bpf: verifier/helper_packet_access.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/fb179fe69e6a
  - [bpf-next,22/43] selftests/bpf: verifier/helper_restricted.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/77aa2563cb44
  - [bpf-next,23/43] selftests/bpf: verifier/helper_value_access.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/ecc424827b77
  - [bpf-next,24/43] selftests/bpf: verifier/int_ptr.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/01481e67dd4d
  - [bpf-next,25/43] selftests/bpf: verifier/ld_ind.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/e29787558066
  - [bpf-next,26/43] selftests/bpf: verifier/leak_ptr.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/583c7ce5be09
  - [bpf-next,27/43] selftests/bpf: verifier/map_ptr.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/caf345cf1207
  - [bpf-next,28/43] selftests/bpf: verifier/map_ret_val.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/05e474ecbb56
  - [bpf-next,29/43] selftests/bpf: verifier/masking.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/ade3f08fc236
  - [bpf-next,30/43] selftests/bpf: verifier/meta_access.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/65428312e38d
  - [bpf-next,31/43] selftests/bpf: verifier/raw_stack.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/5a77a01f3320
  - [bpf-next,32/43] selftests/bpf: verifier/raw_tp_writable.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/18cdc2b531fb
  - [bpf-next,33/43] selftests/bpf: verifier/ringbuf.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/b7e4203086eb
  - [bpf-next,34/43] selftests/bpf: verifier/spill_fill.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/f4fe3cfe6c3a
  - [bpf-next,35/43] selftests/bpf: verifier/stack_ptr.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/edff37b2f28f
  - [bpf-next,36/43] selftests/bpf: verifier/uninit.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/ab839a581946
  - [bpf-next,37/43] selftests/bpf: verifier/value_adj_spill.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/033914942da4
  - [bpf-next,38/43] selftests/bpf: verifier/value.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/8f59e87a3bc6
  - [bpf-next,39/43] selftests/bpf: verifier/value_or_null.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/d330528617b7
  - [bpf-next,40/43] selftests/bpf: verifier/var_off.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/d15f5b68b63a
  - [bpf-next,41/43] selftests/bpf: verifier/xadd.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/a8036aea2d4f
  - [bpf-next,42/43] selftests/bpf: verifier/xdp.c converted to inline assembly
    https://git.kernel.org/bpf/bpf-next/c/ffb515c933a9

You are awesome, thank you!

Andrii Nakryiko March 27, 2023, 3:15 a.m. UTC | #6

On Sat, Mar 25, 2023 at 6:19 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sat, Mar 25, 2023 at 9:16 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > >
> > > It was my understanding from the RFC feedback that this "lighter" way
> > > is preferable and we already have some tests written like that.
> > > Don't have a strong opinion on this topic.
> >
> > Ack, I'm obviously losing a bunch of context here :-(
> > I like coalescing better, but if the original suggestion was to use
> > this lighter way, I'll keep that in mind while reviewing.
>
> I still prefer the clean look of the tests, so I've applied this set.
>
> But I'm not going to insist that this is the only style developers
> should use moving forward.
> Whoever prefers "" style can use it in the future tests.

Great, because I found out in practice that inability to add comments
to the manually written asm code is a pretty big limitation. I do like
the lightweight feel of this unquoted style as well, but practically
we'll probably have to live with both styles.

> I find them harder to read, but oh well.
>
> Ed,
> the only small nit I've noticed is that the tests are compiled
> for both test_progs and test_progs-no_alu32 though they're in asm.
> So we're wasting a bit of CI time running them in both flavors.
> Not a big deal and maybe not worth fixing, since they're pretty fast.

Alexei Starovoitov March 27, 2023, 3:57 a.m. UTC | #7

On Sun, Mar 26, 2023 at 8:16 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Mar 25, 2023 at 6:19 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Sat, Mar 25, 2023 at 9:16 AM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > >
> > > > It was my understanding from the RFC feedback that this "lighter" way
> > > > is preferable and we already have some tests written like that.
> > > > Don't have a strong opinion on this topic.
> > >
> > > Ack, I'm obviously losing a bunch of context here :-(
> > > I like coalescing better, but if the original suggestion was to use
> > > this lighter way, I'll keep that in mind while reviewing.
> >
> > I still prefer the clean look of the tests, so I've applied this set.
> >
> > But I'm not going to insist that this is the only style developers
> > should use moving forward.
> > Whoever prefers "" style can use it in the future tests.
>
> Great, because I found out in practice that inability to add comments
> to the manually written asm code is a pretty big limitation.

What do you mean by "inability" ?
The comments can be added. See verifier_and.c
        r0 &= 0xFFFF1234;                               \
        /* Upper bits are unknown but AND above masks out 1 zero'ing
lower bits */\
        if w0 < 1 goto l0_%=;                           \

Eduard Zingerman March 27, 2023, 11:26 a.m. UTC | #8

On Sun, 2023-03-26 at 20:57 -0700, Alexei Starovoitov wrote:
> On Sun, Mar 26, 2023 at 8:16 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > 
> > On Sat, Mar 25, 2023 at 6:19 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > 
> > > On Sat, Mar 25, 2023 at 9:16 AM Stanislav Fomichev <sdf@google.com> wrote:
> > > > 
> > > > > 
> > > > > It was my understanding from the RFC feedback that this "lighter" way
> > > > > is preferable and we already have some tests written like that.
> > > > > Don't have a strong opinion on this topic.
> > > > 
> > > > Ack, I'm obviously losing a bunch of context here :-(
> > > > I like coalescing better, but if the original suggestion was to use
> > > > this lighter way, I'll keep that in mind while reviewing.
> > > 
> > > I still prefer the clean look of the tests, so I've applied this set.
> > > 
> > > But I'm not going to insist that this is the only style developers
> > > should use moving forward.
> > > Whoever prefers "" style can use it in the future tests.
> > 
> > Great, because I found out in practice that inability to add comments
> > to the manually written asm code is a pretty big limitation.
> 
> What do you mean by "inability" ?
> The comments can be added. See verifier_and.c
>         r0 &= 0xFFFF1234;                               \
>         /* Upper bits are unknown but AND above masks out 1 zero'ing
> lower bits */\
>         if w0 < 1 goto l0_%=;                           \

Yes, /* ... */ work as expected.
// work as well, but one has to be careful, because without \n the full
string after first // would be a comment.

Andrii Nakryiko March 27, 2023, 4:35 p.m. UTC | #9

On Sun, Mar 26, 2023 at 8:57 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Mar 26, 2023 at 8:16 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sat, Mar 25, 2023 at 6:19 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Sat, Mar 25, 2023 at 9:16 AM Stanislav Fomichev <sdf@google.com> wrote:
> > > >
> > > > >
> > > > > It was my understanding from the RFC feedback that this "lighter" way
> > > > > is preferable and we already have some tests written like that.
> > > > > Don't have a strong opinion on this topic.
> > > >
> > > > Ack, I'm obviously losing a bunch of context here :-(
> > > > I like coalescing better, but if the original suggestion was to use
> > > > this lighter way, I'll keep that in mind while reviewing.
> > >
> > > I still prefer the clean look of the tests, so I've applied this set.
> > >
> > > But I'm not going to insist that this is the only style developers
> > > should use moving forward.
> > > Whoever prefers "" style can use it in the future tests.
> >
> > Great, because I found out in practice that inability to add comments
> > to the manually written asm code is a pretty big limitation.
>
> What do you mean by "inability" ?
> The comments can be added. See verifier_and.c
>         r0 &= 0xFFFF1234;                               \
>         /* Upper bits are unknown but AND above masks out 1 zero'ing
> lower bits */\
>         if w0 < 1 goto l0_%=;                           \

My bad. I remembered that there were problems with comments in Eduards
previous revision and concluded that they don't work in this
"\-delimited mode". Especially that online documentation for GCC or
Clang didn't explicitly say that they support /* */ comments in asm
blocks (as far as I could google that).

So now I know it's possible, thanks. I still find it very tedious to
do manually, so I appreciate the flexibility in allowing to do
""-delimited style for new programs.

Just to explain where I'm coming from. I took one asm program I have
locally and converted it to a new style. It was tedious with all the
tab alignment. Then I realized that one comment block uses too long
lines and wanted to use vim to reformat them, and that doesn't work
with those '\' delimiters at the end (I didn't have such a problem
with the original style). So that turned into more tedious work. So
for something that needs iteration and adjustments, ""-delimited style
gives more flexibility. See below for reference.

'#' comments are dangerous, btw, they silently ignore everything till
the very end of asm block. No warning or error, just wrong and
incomplete asm is generated, unfortunately.


SEC("?raw_tp")
__failure __log_level(2)
__msg("XXX")
__naked int subprog_result_precise(void)
{
        asm volatile (
                "r6 = 3;"
                /* pass r6 through r1 into subprog to get it back as r0;
                 * this whole chain will have to be marked as precise later
                 */
                "r1 = r6;"
                "call identity_subprog;"
                /* now use subprog's returned value (which is a
                 * r6 -> r1 -> r0 chain), as index into vals array, forcing
                 * all of that to be known precisely
                 */
                "r0 *= 4;"
                "r1 = %[vals];"
                "r1 += r0;"
                /* here r0->r1->r6 chain is forced to be precise and has to be
                 * propagated back to the beginning, including through the
                 * subprog call
                 */
                "r0 = *(u32 *)(r1 + 0);"
                "exit;"
                :
                : __imm_ptr(vals)
                : __clobber_common, "r6"
        );
}

SEC("?raw_tp")
__failure __log_level(2)
__msg("XXX")
__naked int subprog_result_precise2(void)
{
        asm volatile ("
         \
                r6 = 3;
         \
                /* pass r6 through r1 into subprog to get it back as
r0;        \
                 * this whole chain will have to be marked as precise
later     \
                 */
         \
                r1 = r6;
         \
                call identity_subprog;
         \
                /* now use subprog's returned value (which is a
         \
                 * r6 -> r1 -> r0 chain), as index into vals array,
forcing     \
                 * all of that to be known precisely
         \
                 */
         \
                r0 *= 4;
         \
                r1 = %[vals];
         \
                r1 += r0;
         \
                /* here r0->r1->r6 chain is forced to be precise and
has to be  \
                 * propagated back to the beginning, including through
the      \
                 * subprog call
         \
                 */
         \
                r0 = *(u32 *)(r1 + 0);
         \
                exit;
         \
                "
                :
                : __imm_ptr(vals)
                : __clobber_common, "r6"
        );
}

Andrii Nakryiko March 27, 2023, 4:37 p.m. UTC | #10

On Mon, Mar 27, 2023 at 9:35 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sun, Mar 26, 2023 at 8:57 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Sun, Mar 26, 2023 at 8:16 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Sat, Mar 25, 2023 at 6:19 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Sat, Mar 25, 2023 at 9:16 AM Stanislav Fomichev <sdf@google.com> wrote:
> > > > >
> > > > > >
> > > > > > It was my understanding from the RFC feedback that this "lighter" way
> > > > > > is preferable and we already have some tests written like that.
> > > > > > Don't have a strong opinion on this topic.
> > > > >
> > > > > Ack, I'm obviously losing a bunch of context here :-(
> > > > > I like coalescing better, but if the original suggestion was to use
> > > > > this lighter way, I'll keep that in mind while reviewing.
> > > >
> > > > I still prefer the clean look of the tests, so I've applied this set.
> > > >
> > > > But I'm not going to insist that this is the only style developers
> > > > should use moving forward.
> > > > Whoever prefers "" style can use it in the future tests.
> > >
> > > Great, because I found out in practice that inability to add comments
> > > to the manually written asm code is a pretty big limitation.
> >
> > What do you mean by "inability" ?
> > The comments can be added. See verifier_and.c
> >         r0 &= 0xFFFF1234;                               \
> >         /* Upper bits are unknown but AND above masks out 1 zero'ing
> > lower bits */\
> >         if w0 < 1 goto l0_%=;                           \
>
> My bad. I remembered that there were problems with comments in Eduards
> previous revision and concluded that they don't work in this
> "\-delimited mode". Especially that online documentation for GCC or
> Clang didn't explicitly say that they support /* */ comments in asm
> blocks (as far as I could google that).
>
> So now I know it's possible, thanks. I still find it very tedious to
> do manually, so I appreciate the flexibility in allowing to do
> ""-delimited style for new programs.
>
> Just to explain where I'm coming from. I took one asm program I have
> locally and converted it to a new style. It was tedious with all the
> tab alignment. Then I realized that one comment block uses too long
> lines and wanted to use vim to reformat them, and that doesn't work
> with those '\' delimiters at the end (I didn't have such a problem
> with the original style). So that turned into more tedious work. So
> for something that needs iteration and adjustments, ""-delimited style
> gives more flexibility. See below for reference.
>
> '#' comments are dangerous, btw, they silently ignore everything till
> the very end of asm block. No warning or error, just wrong and
> incomplete asm is generated, unfortunately.
>
>
> SEC("?raw_tp")
> __failure __log_level(2)
> __msg("XXX")
> __naked int subprog_result_precise(void)
> {
>         asm volatile (
>                 "r6 = 3;"
>                 /* pass r6 through r1 into subprog to get it back as r0;
>                  * this whole chain will have to be marked as precise later
>                  */
>                 "r1 = r6;"
>                 "call identity_subprog;"
>                 /* now use subprog's returned value (which is a
>                  * r6 -> r1 -> r0 chain), as index into vals array, forcing
>                  * all of that to be known precisely
>                  */
>                 "r0 *= 4;"
>                 "r1 = %[vals];"
>                 "r1 += r0;"
>                 /* here r0->r1->r6 chain is forced to be precise and has to be
>                  * propagated back to the beginning, including through the
>                  * subprog call
>                  */
>                 "r0 = *(u32 *)(r1 + 0);"
>                 "exit;"
>                 :
>                 : __imm_ptr(vals)
>                 : __clobber_common, "r6"
>         );
> }
>
> SEC("?raw_tp")
> __failure __log_level(2)
> __msg("XXX")
> __naked int subprog_result_precise2(void)
> {
>         asm volatile ("
>          \
>                 r6 = 3;
>          \
>                 /* pass r6 through r1 into subprog to get it back as
> r0;        \
>                  * this whole chain will have to be marked as precise
> later     \
>                  */
>          \
>                 r1 = r6;
>          \
>                 call identity_subprog;
>          \
>                 /* now use subprog's returned value (which is a
>          \
>                  * r6 -> r1 -> r0 chain), as index into vals array,
> forcing     \
>                  * all of that to be known precisely
>          \
>                  */
>          \
>                 r0 *= 4;
>          \
>                 r1 = %[vals];
>          \
>                 r1 += r0;
>          \
>                 /* here r0->r1->r6 chain is forced to be precise and
> has to be  \
>                  * propagated back to the beginning, including through
> the      \
>                  * subprog call
>          \
>                  */
>          \
>                 r0 = *(u32 *)(r1 + 0);
>          \
>                 exit;
>          \

Great, Gmail doesn't like this style as well :( Sorry for the visual noise.
>                 "
>                 :
>                 : __imm_ptr(vals)
>                 : __clobber_common, "r6"
>         );
> }

Daniel Borkmann March 28, 2023, 3:48 a.m. UTC | #11

Hi Eduard,

On 3/25/23 3:54 AM, Eduard Zingerman wrote:
> This is a follow up for RFC [1]. It migrates a first batch of 38
> verifier/*.c tests to inline assembly and use of ./test_progs for
> actual execution. The migration is done by a python script (see [2]).
> 
> Each migrated verifier/xxx.c file is mapped to progs/verifier_xxx.c
> plus an entry in the prog_tests/verifier.c. One patch per each file.
> 
> A few patches at the beginning of the patch-set extend test_loader
> with necessary functionality, mainly:
> - support for tests execution in unprivileged mode;
> - support for test runs for test programs.
> 
> Migrated tests could be selected for execution using the following filter:
> 
>    ./test_progs -a verifier_*
>    
> An example of the migrated test:
> 
>    SEC("xdp")
>    __description("XDP pkt read, pkt_data' > pkt_end, corner case, good access")
>    __success __retval(0) __flag(BPF_F_ANY_ALIGNMENT)
>    __naked void end_corner_case_good_access_1(void)
>    {
>            asm volatile ("                                 \
>            r2 = *(u32*)(r1 + %[xdp_md_data]);              \
>            r3 = *(u32*)(r1 + %[xdp_md_data_end]);          \
>            r1 = r2;                                        \
>            r1 += 8;                                        \
>            if r1 > r3 goto l0_%=;                          \
>            r0 = *(u64*)(r1 - 8);                           \
>    l0_%=:  r0 = 0;                                         \
>            exit;                                           \
>    "       :
>            : __imm_const(xdp_md_data, offsetof(struct xdp_md, data)),
>              __imm_const(xdp_md_data_end, offsetof(struct xdp_md, data_end))
>            : __clobber_all);
>    }
> 
> Changes compared to RFC:
> - test_loader.c is extended to support test program runs;
> - capabilities handling now matches behavior of test_verifier;
> - BPF_ST_MEM instructions are automatically replaced by BPF_STX_MEM
>    instructions to overcome current clang limitations;
> - tests styling updates according to RFC feedback;
> - 38 migrated files are included instead of 1.
> 
> I used the following means for testing:
> - migration tool itself has a set of self-tests;
> - migrated tests are passing;
> - manually compared each old/new file side-by-side.
> 
> While doing side-by-side comparison I've noted a few defects in the
> original tests:
> - and.c:
>    - One of the jump targets is off by one;
>    - BPF_ST_MEM wrong OFF/IMM ordering;
> - array_access.c:
>    - BPF_ST_MEM wrong OFF/IMM ordering;
> - value_or_null.c:
>    - BPF_ST_MEM wrong OFF/IMM ordering.
> 
> These defects would be addressed separately.

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?
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?

> [1] RFC
>      https://lore.kernel.org/bpf/20230123145148.2791939-1-eddyz87@gmail.com/
> [2] Migration tool
>      https://github.com/eddyz87/verifier-tests-migrator

Thanks,
Daniel

Eduard Zingerman March 28, 2023, 9:52 p.m. UTC | #12

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

Andrii Nakryiko March 28, 2023, 10:24 p.m. UTC | #13

On Tue, Mar 28, 2023 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> 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.
>

Great, this seems to be covered.

> # 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.

> # 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.
>

+1


> # 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.

>
> # 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).

We can just write these as explicitly programmatically generated
programs, probably. There are just a few of these, shouldn't be a big
deal.

>
> # 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.

>             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.

>
> # 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

Eduard Zingerman March 28, 2023, 10:38 p.m. UTC | #14

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.

[...]
> 
> > # 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?

[...]
> > # `.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?

> > # 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.

[...]

Alexei Starovoitov March 28, 2023, 11:31 p.m. UTC | #15

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.

+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.

Andrii Nakryiko March 29, 2023, 12:07 a.m. UTC | #16

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.
>
> [...]

Andrii Nakryiko March 29, 2023, 12:11 a.m. UTC | #17

On Tue, Mar 28, 2023 at 4:31 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> 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.
>
> +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.

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

Message

Comments