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

Commit Message

Tiezhu Yang Jan. 4, 2024, 1:08 p.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]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
  [root@linux bpf]# ./test_verifier | grep FAIL
  #106/p inline simple bpf_loop call FAIL
  #107/p don't inline bpf_loop call, flags non-zero FAIL
  #108/p don't inline bpf_loop call, callback non-constant FAIL
  #109/p bpf_loop_inline and a dead func FAIL
  #110/p bpf_loop_inline stack locations for loop vars FAIL
  #111/p inline bpf_loop call in a big program FAIL
  Summary: 768 PASSED, 15 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]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
  [root@linux bpf]# ./test_verifier | grep FAIL
  Summary: 768 PASSED, 21 SKIPPED, 0 FAILED

Additionally, as Eduard suggested, return -ENOTSUPP instead of -EINVAL
for the other three places where "non-JITed" is used in error messages
to keep consistent.

Signed-off-by: Tiezhu Yang <[e-mail]>
---

v2:
  -- rebase on the latest bpf-next tree.
  -- return -ENOTSUPP instead of -EINVAL for the other three places
     where "non-JITed" is used in error messages to keep consistent.
  -- update the patch subject and commit message.

 kernel/bpf/verifier.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jiri Olsa Jan. 8, 2024, 10:05 a.m. UTC | #1

On Thu, Jan 04, 2024 at 09:08:17PM +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]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>   [root@linux bpf]# ./test_verifier | grep FAIL
>   #106/p inline simple bpf_loop call FAIL
>   #107/p don't inline bpf_loop call, flags non-zero FAIL
>   #108/p don't inline bpf_loop call, callback non-constant FAIL
>   #109/p bpf_loop_inline and a dead func FAIL
>   #110/p bpf_loop_inline stack locations for loop vars FAIL
>   #111/p inline bpf_loop call in a big program FAIL
>   Summary: 768 PASSED, 15 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]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>   [root@linux bpf]# ./test_verifier | grep FAIL
>   Summary: 768 PASSED, 21 SKIPPED, 0 FAILED
> 
> Additionally, as Eduard suggested, return -ENOTSUPP instead of -EINVAL
> for the other three places where "non-JITed" is used in error messages
> to keep consistent.
> 
> Signed-off-by: Tiezhu Yang <[e-mail]>
> ---
> 
> v2:
>   -- rebase on the latest bpf-next tree.
>   -- return -ENOTSUPP instead of -EINVAL for the other three places
>      where "non-JITed" is used in error messages to keep consistent.
>   -- update the patch subject and commit message.
> 
>  kernel/bpf/verifier.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d5f4ff1eb235..99558a5186b2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8908,7 +8908,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
>  			goto error;
>  		if (env->subprog_cnt > 1 && !allow_tail_call_in_subprogs(env)) {
>  			verbose(env, "tail_calls are not allowed in non-JITed programs with bpf-to-bpf calls\n");
> -			return -EINVAL;
> +			return -ENOTSUPP;

FWIW I agree with John review earlier [1], also there's chance (however small)
we could mess up with some app already checking on that

jirka

[1] https://lore.kernel.org/bpf/[e-mail]/

>  		}
>  		break;
>  	case BPF_FUNC_perf_event_read:
> @@ -19069,14 +19069,14 @@ static int fixup_call_args(struct bpf_verifier_env *env)
>  #ifndef CONFIG_BPF_JIT_ALWAYS_ON
>  	if (has_kfunc_call) {
>  		verbose(env, "calling kernel functions are not allowed in non-JITed programs\n");
> -		return -EINVAL;
> +		return -ENOTSUPP;
>  	}
>  	if (env->subprog_cnt > 1 && env->prog->aux->tail_call_reachable) {
>  		/* When JIT fails the progs with bpf2bpf calls and tail_calls
>  		 * have to be rejected, since interpreter doesn't support them yet.
>  		 */
>  		verbose(env, "tail_calls are not allowed in non-JITed programs with bpf-to-bpf calls\n");
> -		return -EINVAL;
> +		return -ENOTSUPP;
>  	}
>  	for (i = 0; i < prog->len; i++, insn++) {
>  		if (bpf_pseudo_func(insn)) {
> @@ -19084,7 +19084,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
> 
>

Daniel Borkmann Jan. 8, 2024, 3:27 p.m. UTC | #2

On 1/8/24 11:05 AM, Jiri Olsa wrote:
> On Thu, Jan 04, 2024 at 09:08:17PM +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]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>>    [root@linux bpf]# ./test_verifier | grep FAIL
>>    #106/p inline simple bpf_loop call FAIL
>>    #107/p don't inline bpf_loop call, flags non-zero FAIL
>>    #108/p don't inline bpf_loop call, callback non-constant FAIL
>>    #109/p bpf_loop_inline and a dead func FAIL
>>    #110/p bpf_loop_inline stack locations for loop vars FAIL
>>    #111/p inline bpf_loop call in a big program FAIL
>>    Summary: 768 PASSED, 15 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]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>>    [root@linux bpf]# ./test_verifier | grep FAIL
>>    Summary: 768 PASSED, 21 SKIPPED, 0 FAILED
>>
>> Additionally, as Eduard suggested, return -ENOTSUPP instead of -EINVAL
>> for the other three places where "non-JITed" is used in error messages
>> to keep consistent.
>>
>> Signed-off-by: Tiezhu Yang <[e-mail]>
>> ---
>>
>> v2:
>>    -- rebase on the latest bpf-next tree.
>>    -- return -ENOTSUPP instead of -EINVAL for the other three places
>>       where "non-JITed" is used in error messages to keep consistent.
>>    -- update the patch subject and commit message.
>>
>>   kernel/bpf/verifier.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d5f4ff1eb235..99558a5186b2 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8908,7 +8908,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
>>   			goto error;
>>   		if (env->subprog_cnt > 1 && !allow_tail_call_in_subprogs(env)) {
>>   			verbose(env, "tail_calls are not allowed in non-JITed programs with bpf-to-bpf calls\n");
>> -			return -EINVAL;
>> +			return -ENOTSUPP;
> 
> FWIW I agree with John review earlier [1], also there's chance (however small)
> we could mess up with some app already checking on that

+1, the ship on this has sailed unfortunately. Tiezhu, it would be good if you could
update the selftest handling instead.

> jirka
> 
> [1] https://lore.kernel.org/bpf/[e-mail]/

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d5f4ff1eb235..99558a5186b2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8908,7 +8908,7 @@  static int check_map_func_compatibility(struct bpf_verifier_env *env,
 			goto error;
 		if (env->subprog_cnt > 1 && !allow_tail_call_in_subprogs(env)) {
 			verbose(env, "tail_calls are not allowed in non-JITed programs with bpf-to-bpf calls\n");
-			return -EINVAL;
+			return -ENOTSUPP;
 		}
 		break;
 	case BPF_FUNC_perf_event_read:
@@ -19069,14 +19069,14 @@  static int fixup_call_args(struct bpf_verifier_env *env)
 #ifndef CONFIG_BPF_JIT_ALWAYS_ON
 	if (has_kfunc_call) {
 		verbose(env, "calling kernel functions are not allowed in non-JITed programs\n");
-		return -EINVAL;
+		return -ENOTSUPP;
 	}
 	if (env->subprog_cnt > 1 && env->prog->aux->tail_call_reachable) {
 		/* When JIT fails the progs with bpf2bpf calls and tail_calls
 		 * have to be rejected, since interpreter doesn't support them yet.
 		 */
 		verbose(env, "tail_calls are not allowed in non-JITed programs with bpf-to-bpf calls\n");
-		return -EINVAL;
+		return -ENOTSUPP;
 	}
 	for (i = 0; i < prog->len; i++, insn++) {
 		if (bpf_pseudo_func(insn)) {
@@ -19084,7 +19084,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))