diff mbox series

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

Message ID 20231225091830.6094-1-yangtiezhu@loongson.cn (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,v1] bpf: Return -ENOTSUPP if callbacks are not allowed in non-JITed programs | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1127 this patch: 1127
netdev/cc_maintainers warning 8 maintainers not CCed: sdf@google.com haoluo@google.com martin.lau@linux.dev jolsa@kernel.org kpsingh@kernel.org yonghong.song@linux.dev song@kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 1143 this patch: 1143
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1154 this patch: 1154
netdev/checkpatch warning WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc

Commit Message

Tiezhu Yang Dec. 25, 2023, 9:18 a.m. UTC 
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@loongson.cn>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eduard Zingerman Jan. 2, 2024, 4:13 p.m. UTC | #1 
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@loongson.cn>
> ---
>  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@huaweicloud.com/
John Fastabend Jan. 3, 2024, 12:05 a.m. UTC | #2 
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@loongson.cn>
> > ---
> >  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@huaweicloud.com/
>
Tiezhu Yang Jan. 4, 2024, 7:41 a.m. UTC | #3 
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 <yangtiezhu@loongson.cn>
>>> ---
>>>  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/20231225091830.6094-1-yangtiezhu@loongson.cn/

Thanks,
Tiezhu
diff mbox series

Patch

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