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

[John Fastabend]

Eduard Zingerman wrote:
> 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.

My preference is to just leave them as is unless its a serious
problem. In this case any userspace has likely already figured
out how to handle these errors so I don't think there is much
value in changing things.

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