Re: [PATCH bpf-next v1] bpf: Return -ENOTSUPP if callbacks are not allowed in non-JITed programs

[Eduard Zingerman]

On Mon, 2023-12-25 at 17:18 +0800, Tiezhu Yang wrote:
> If CONFIG_BPF_JIT_ALWAYS_ON is not set and bpf_jit_enable is 0, there
> exist 6 failed tests.
> 
>   [root@linux bpf]# echo 0 > /proc/sys/net/core/bpf_jit_enable
>   [root@linux bpf]# ./test_verifier | grep FAIL
>   #107/p inline simple bpf_loop call FAIL
>   #108/p don't inline bpf_loop call, flags non-zero FAIL
>   #109/p don't inline bpf_loop call, callback non-constant FAIL
>   #110/p bpf_loop_inline and a dead func FAIL
>   #111/p bpf_loop_inline stack locations for loop vars FAIL
>   #112/p inline bpf_loop call in a big program FAIL
>   Summary: 505 PASSED, 266 SKIPPED, 6 FAILED
> 
> The test log shows that callbacks are not allowed in non-JITed programs,
> interpreter doesn't support them yet, thus these tests should be skipped
> if jit is disabled, just return -ENOTSUPP instead of -EINVAL for pseudo
> calls in fixup_call_args().
> 
> With this patch:
> 
>   [root@linux bpf]# echo 0 > /proc/sys/net/core/bpf_jit_enable
>   [root@linux bpf]# ./test_verifier | grep FAIL
>   Summary: 505 PASSED, 272 SKIPPED, 0 FAILED
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>
> ---
>  kernel/bpf/verifier.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a376eb609c41..1c780a893284 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19069,7 +19069,7 @@ static int fixup_call_args(struct bpf_verifier_env *env)
>  			 * have to be rejected, since interpreter doesn't support them yet.
>  			 */
>  			verbose(env, "callbacks are not allowed in non-JITed programs\n");
> -			return -EINVAL;
> +			return -ENOTSUPP;
>  		}
>  
>  		if (!bpf_pseudo_call(insn))

I agree with this change, however I think that it should be consistent.
Quick and non-exhaustive grepping shows that there are 4 places where
"non-JITed" is used in error messages: in check_map_func_compatibility()
and in fixup_call_args().
All these places currently use -EINVAL and should be updated to -ENOTSUPP,
if this change gets a green light.

If the goal is to merely make test_verifier pass when JIT is disabled
there is a different way:
- test_progs has a global variable 'env.jit_enabled' which is used by
  several tests to decide whether to skip or not;
- loop inlining tests could use similar feature, but unfortunately
  test_verifier does not provide similar functionality;
- test_verifier is sort-of in legacy mode, so instead of porting the
  jit_enabled to test_verifier, I think that loop inlining tests
  should be migrated to test_progs. I can do that some time after [1]
  would be landed.
  
[1] https://lore.kernel.org/bpf/20231223104042.1432300-3-houtao@xxxxxxxxxxxxxxx/