Re: [PATCH bpf-next v3 2/2] selftests/bpf: Add ldsx selftests for ldsx and subreg compare

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-07-12 at 16:44 -0700, Yonghong Song wrote:

Note: it feels like these test cases should be a part of the
      reg_bounds.c:test_reg_bounds_crafted(), e.g.:

  diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
  index eb74363f9f70..4918414f8e36 100644
  --- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
  +++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
  @@ -2108,6 +2108,7 @@ static struct subtest_case crafted_cases[] = {
          {S32, U32, {(u32)S32_MIN, 0}, {0, 0}},
          {S32, U32, {(u32)S32_MIN, 0}, {(u32)S32_MIN, (u32)S32_MIN}},
          {S32, U32, {(u32)S32_MIN, S32_MAX}, {S32_MAX, S32_MAX}},
  +       {S64, U32, {0x0, 0x1f}, {0xffffffff80000000ULL, 0x00000000ffffffffULL}},
   };
   
   /* Go over crafted hard-coded cases. This is fast, so we do it as part of

Which produces the following log:

  VERIFIER LOG:
  ========================
  ...
  21: (ae) if w6 < w7 goto pc+3         ; R6=scalar(id=1,smin=0xffffffff80000000,smax=0xffffffff)
                                          R7=scalar(id=2,smin=smin32=0,smax=umax=smax32=umax32=31,var_off=(0x0; 0x1f))
  ...  
  from 21 to 25: ... R6=scalar(id=1,smin=smin32=0,smax=umax=smax32=umax32=30,var_off=(0x0; 0x1f))
                     R7=scalar(id=2,smin=umin=smin32=umin32=1,smax=umax=smax32=umax32=31,var_off=(0x0; 0x1f)
  25: ... R6=scalar(id=1,smin=smin32=0,smax=umax=smax32=umax32=30,var_off=(0x0; 0x1f))
          R7=scalar(id=2,smin=umin=smin32=umin32=1,smax=umax=smax32=umax32=31,var_off=(0x0; 0x1f))
  ...

However, this would require adjustments to reg_bounds.c logic to
include this new range rule.

[...]

> +SEC("socket")
> +__description("LDSX, S8, subreg compare")
                     ^^^ ^^^

Nit: test_progs parsing logic for -t option is too simplistic to
     understand comas inside description, for example here is happens
     after the command below:

       # ./test_progs-cpuv4 -t "verifier_ldsx/LDSX, S8, subreg compare"
       #455/1   verifier_ldsx/LDSX, S8:OK
       #455/2   verifier_ldsx/LDSX, S8 @unpriv:OK
       #455/3   verifier_ldsx/LDSX, S16:OK
       #455/4   verifier_ldsx/LDSX, S16 @unpriv:OK
       #455/5   verifier_ldsx/LDSX, S32:OK
       ...

     As far as I understand, this happens because test_progs tries to
     match words LDSX, S8 and "subreg compare" separately.

     This does not happen when comas are not used in the description
     (or if description is omitted in favor of the C function name of the test
      (it is not possible to do -t verifier_ldsx/ldsx_s8_subreg_compare
       if __description is provided)).

> +__success __success_unpriv
> +__naked void ldsx_s8_subreg_compare(void)
> +{
> +	asm volatile (
> +	"call %[bpf_get_prandom_u32];"
> +	"*(u64 *)(r10 - 8) = r0;"
> +	"w6 = w0;"
> +	"if w6 > 0x1f goto l0_%=;"
> +	"r7 = *(s8 *)(r10 - 8);"
> +	"if w7 > w6 goto l0_%=;"
> +	"r1 = 0;"
> +	"*(u64 *)(r10 - 8) = r1;"
> +	"r2 = r10;"
> +	"r2 += -8;"
> +	"r1 = %[map_hash_48b] ll;"
> +	"call %[bpf_map_lookup_elem];"
> +	"if r0 == 0 goto l0_%=;"
> +	"r0 += r7;"
> +	"*(u64 *)(r0 + 0) = 1;"
> +"l0_%=:"
> +	"r0 = 0;"
> +	"exit;"
> +	:
> +	: __imm(bpf_get_prandom_u32),
> +	  __imm_addr(map_hash_48b),
> +	  __imm(bpf_map_lookup_elem)
> +	: __clobber_all);
> +}

[...]