RE: [PATCH v3 bpf-next 4/4] selftests/bpf: Test may_goto

[John Fastabend <john.fastabend@gmail.com>]

Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> Add tests for may_goto instruction via cond_break macro.
> 
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  tools/testing/selftests/bpf/DENYLIST.s390x    |  1 +
>  .../bpf/progs/verifier_iterating_callbacks.c  | 72 ++++++++++++++++++-
>  2 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/DENYLIST.s390x b/tools/testing/selftests/bpf/DENYLIST.s390x
> index 1a63996c0304..c6c31b960810 100644
> --- a/tools/testing/selftests/bpf/DENYLIST.s390x
> +++ b/tools/testing/selftests/bpf/DENYLIST.s390x
> @@ -3,3 +3,4 @@
>  exceptions				 # JIT does not support calling kfunc bpf_throw				       (exceptions)
>  get_stack_raw_tp                         # user_stack corrupted user stack                                             (no backchain userspace)
>  stacktrace_build_id                      # compare_map_keys stackid_hmap vs. stackmap err -2 errno 2                   (?)
> +verifier_iter/cond_break
> diff --git a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
> index 5905e036e0ea..8476dc47623f 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
> @@ -1,8 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
> -
> -#include <linux/bpf.h>
> -#include <bpf/bpf_helpers.h>
>  #include "bpf_misc.h"
> +#include "bpf_experimental.h"
>  
>  struct {
>  	__uint(type, BPF_MAP_TYPE_ARRAY);
> @@ -239,4 +237,72 @@ int bpf_loop_iter_limit_nested(void *unused)
>  	return 1000 * a + b + c;
>  }
>  
> +#define ARR_SZ 1000000
> +int zero;
> +char arr[ARR_SZ];
> +
> +SEC("socket")
> +__success __retval(0xd495cdc0)
> +int cond_break1(const void *ctx)
> +{
> +	unsigned int i;
> +	unsigned int sum = 0;
> +
> +	for (i = zero; i < ARR_SZ; cond_break, i++)
> +		sum += i;
> +	for (i = zero; i < ARR_SZ; i++) {
> +		barrier_var(i);
> +		sum += i + arr[i];
> +		cond_break;
> +	}
> +
> +	return sum;
> +}
> +
> +SEC("socket")
> +__success __retval(999000000)
> +int cond_break2(const void *ctx)
> +{
> +	int i, j;
> +	int sum = 0;
> +
> +	for (i = zero; i < 1000; cond_break, i++)
> +		for (j = zero; j < 1000; j++) {
> +			sum += i + j;
> +			cond_break;
> +		}
> +
> +	return sum;
> +}
> +
> +static __noinline int loop(void)
> +{
> +	int i, sum = 0;
> +
> +	for (i = zero; i <= 1000000; i++, cond_break)
> +		sum += i;
> +
> +	return sum;
> +}
> +
> +SEC("socket")
> +__success __retval(0x6a5a2920)
> +int cond_break3(const void *ctx)
> +{
> +	return loop();
> +}
> +
> +SEC("socket")
> +__success __retval(0x800000) /* BPF_MAX_LOOPS */
> +int cond_break4(const void *ctx)
> +{
> +	int cnt = 0;
> +
> +	for (;;) {
> +		cond_break;
> +		cnt++;
> +	}
> +	return cnt;
> +}

I found this test illustrative to show how the cond_break which
is to me "feels" like a global hidden iterator appears to not
be reinitialized across calls?

 static __noinline int full_loop(void)
 {
         int cnt = 0;
 
         for (;;) {
                 cond_break;
                 cnt++;
         }
 
         for (;;) {
                 cond_break;
                 cnt++;
         }
 
         bpf_printk("cnt==%d\n", cnt);
         return cnt;
 }

 SEC("socket")
 __success __retval(16777216)
 int cond_break5(const void *ctx)
 {
         int cnt = 0;
  
         for (;;) {
                 cond_break;
                 cnt++;
         }
  
         cnt += full_loop();
  
         for (;;) {
                 cond_break;
                 cnt++;
         }
         return cnt;
 }
  
This fails with,

do_prog_test_run:PASS:bpf_prog_test_run 0 nsec
run_subtest:FAIL:654 Unexpected retval: 8388608 != 16777216
#430/15  verifier_iterating_callbacks/cond_break5:FAIL
#430     verifier_iterating_callbacks:FAIL

;       cnt += full_loop();                                         
     118:       18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
     120:       b4 02 00 00 0d 00 00 00 w2 = 13
     121:       bc 73 00 00 00 00 00 00 w3 = w7
     122:       85 00 00 00 06 00 00 00 call 6
;                                                            

I guess this is by design but I sort of expected each
call to have its own context. It does make some sense to
limit main and all calls to a max loop count so not
complaining. Maybe consider adding the test? I at least
thought it helped.

> +
>  char _license[] SEC("license") = "GPL";
> -- 
> 2.34.1
> 
>