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 <[email protected]>
---
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))
--
2.42.0
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 <[email protected]>
> ---
> 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/[email protected]/
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 <[email protected]>
> > ---
> > 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/[email protected]/
>
On 01/03/2024 08:05 AM, John Fastabend wrote:
> 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.
...
>>> Signed-off-by: Tiezhu Yang <[email protected]>
>>> ---
>>> 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.
I am not quite sure whether to ignore this patch, but the state of
this patch is "Changes Requested" [1], I guess I should send v2 as
Eduard suggested.
[1]
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
Thanks,
Tiezhu