bpf: Tidy up verifier check_func_arg()

PATCH Applied Joanne Koong 5 participants 9 messages Jul 12, 2022 – Jul 13, 2022

Subsystems (2)

BPF BPF core

lore

Share & tools

lore

Joanne Koong

author

2022-07-12 21:07 UTC #0 lore raw

PATCH 1/1 bpf: Tidy up verifier check_func_arg()

This patch does two things:

1. For matching against the arg type, the match should be against the base type of the arg type, since the arg type can have different bpf_type_flags set on it.

2. Uses switch casing to improve readability + efficiency.

Signed-off-by Joanne Koong <[email protected]>

---
 kernel/bpf/verifier.c | 66 +++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 28 deletions(-)

kernel/bpf/verifier.c +38 ~~-28~~

index 328cfab3af60..26e7e787c20a 100644
@@ -5533,17 +5533,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type)
 	       type == ARG_CONST_SIZE_OR_ZERO;
 }
 
-static bool arg_type_is_alloc_size(enum bpf_arg_type type)
-{
-	return type == ARG_CONST_ALLOC_SIZE_OR_ZERO;
-}
-
-static bool arg_type_is_int_ptr(enum bpf_arg_type type)
-{
-	return type == ARG_PTR_TO_INT ||
-	       type == ARG_PTR_TO_LONG;
-}
-
 static bool arg_type_is_release(enum bpf_arg_type type)
 {
 	return type & OBJ_RELEASE;

@@ -5929,7 +5918,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		meta->ref_obj_id = reg->ref_obj_id;
 	}
 
-	if (arg_type == ARG_CONST_MAP_PTR) {
+	switch (base_type(arg_type)) {
+	case ARG_CONST_MAP_PTR:
 		/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
 		if (meta->map_ptr) {
 			/* Use map_uid (which is unique id of inner map) to reject:

@@ -5954,7 +5944,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		}
 		meta->map_ptr = reg->map_ptr;
 		meta->map_uid = reg->map_uid;
-	} else if (arg_type == ARG_PTR_TO_MAP_KEY) {
+		break;
+	case ARG_PTR_TO_MAP_KEY:
 		/* bpf_map_xxx(..., map_ptr, ..., key) call:
 		 * check that [key, key + map->key_size) are within
 		 * stack limits and initialized

@@ -5971,7 +5962,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		err = check_helper_mem_access(env, regno,
 					      meta->map_ptr->key_size, false,
 					      NULL);
-	} else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) {
+		break;
+	case ARG_PTR_TO_MAP_VALUE:
 		if (type_may_be_null(arg_type) && register_is_null(reg))
 			return 0;

@@ -5987,14 +5979,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		err = check_helper_mem_access(env, regno,
 					      meta->map_ptr->value_size, false,
 					      meta);
-	} else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
+		break;
+	case ARG_PTR_TO_PERCPU_BTF_ID:
 		if (!reg->btf_id) {
 			verbose(env, "Helper has invalid btf_id in R%d\n", regno);
 			return -EACCES;
 		}
 		meta->ret_btf = reg->btf;
 		meta->ret_btf_id = reg->btf_id;
-	} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
+		break;
+	case ARG_PTR_TO_SPIN_LOCK:
 		if (meta->func_id == BPF_FUNC_spin_lock) {
 			if (process_spin_lock(env, regno, true))
 				return -EACCES;

@@ -6005,12 +5999,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			verbose(env, "verifier internal error\n");
 			return -EFAULT;
 		}
-	} else if (arg_type == ARG_PTR_TO_TIMER) {
+		break;
+	case ARG_PTR_TO_TIMER:
 		if (process_timer_func(env, regno, meta))
 			return -EACCES;
-	} else if (arg_type == ARG_PTR_TO_FUNC) {
+		break;
+	case ARG_PTR_TO_FUNC:
 		meta->subprogno = reg->subprogno;
-	} else if (base_type(arg_type) == ARG_PTR_TO_MEM) {
+		break;
+	case ARG_PTR_TO_MEM:
 		/* The access to this pointer is only checked when we hit the
 		 * next is_mem_size argument below.
 		 */

@@ -6020,11 +6017,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 						      fn->arg_size[arg], false,
 						      meta);
 		}
-	} else if (arg_type_is_mem_size(arg_type)) {
-		bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
-
-		err = check_mem_size_reg(env, reg, regno, zero_size_allowed, meta);
-	} else if (arg_type_is_dynptr(arg_type)) {
+		break;
+	case ARG_CONST_SIZE:
+		err = check_mem_size_reg(env, reg, regno, false, meta);
+		break;
+	case ARG_CONST_SIZE_OR_ZERO:
+		err = check_mem_size_reg(env, reg, regno, true, meta);
+		break;
+	case ARG_PTR_TO_DYNPTR:
 		if (arg_type & MEM_UNINIT) {
 			if (!is_dynptr_reg_valid_uninit(env, reg)) {
 				verbose(env, "Dynptr has to be an uninitialized dynptr\n");

@@ -6058,21 +6058,28 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 				err_extra, arg + 1);
 			return -EINVAL;
 		}
-	} else if (arg_type_is_alloc_size(arg_type)) {
+		break;
+	case ARG_CONST_ALLOC_SIZE_OR_ZERO:
 		if (!tnum_is_const(reg->var_off)) {
 			verbose(env, "R%d is not a known constant'\n",
 				regno);
 			return -EACCES;
 		}
 		meta->mem_size = reg->var_off.value;
-	} else if (arg_type_is_int_ptr(arg_type)) {
+		break;
+	case ARG_PTR_TO_INT:
+	case ARG_PTR_TO_LONG:
+	{
 		int size = int_ptr_type_to_size(arg_type);
 
 		err = check_helper_mem_access(env, regno, size, false, meta);
 		if (err)
 			return err;
 		err = check_ptr_alignment(env, reg, 0, size, true);
-	} else if (arg_type == ARG_PTR_TO_CONST_STR) {
+		break;
+	}
+	case ARG_PTR_TO_CONST_STR:
+	{
 		struct bpf_map *map = reg->map_ptr;
 		int map_off;
 		u64 map_addr;

@@ -6111,9 +6118,12 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			verbose(env, "string is not zero-terminated\n");
 			return -EINVAL;
 		}
-	} else if (arg_type == ARG_PTR_TO_KPTR) {
+		break;
+	}
+	case ARG_PTR_TO_KPTR:
 		if (process_kptr_func(env, regno, meta))
 			return -EACCES;
+		break;
 	}
 
 	return err;

Signature · 1 line

2.30.2

Hao Luo

2022-07-13 01:20 UTC #3 lore raw

Quoted 10 lines · Joanne Koong

Quoted 8 lines · On Tue, Jul 12, 2022 at 3:44 PM <[email protected]> wrote:
Quoted 5 lines · Joanne Koong
This patch does two things:
1. For matching against the arg type, the match should be against the base type of the arg type, since the arg type can have different bpf_type_flags set on it.

Quoted 6 lines

Quoted 5 lines
Does this need a fixes tag? Something around the following maybe:
Fixes d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.")
?
I will add that tag. Thanks!

Joanne and Stan, IMO this is not necessary. I think this change is a cleanup rather than a fix.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
2. Uses switch casing to improve readability + efficiency.
Signed-off-by Joanne Koong <[email protected]>
---

[...]

Quoted 1 line

Quoted 1 line

Alexei Starovoitov

2022-07-13 01:26 UTC #4 lore raw

Quoted 14 lines · Hao Luo

Quoted 12 lines · Joanne Koong
Quoted 10 lines · On Tue, Jul 12, 2022 at 3:44 PM <[email protected]> wrote:
Quoted 5 lines · Joanne Koong
This patch does two things:
1. For matching against the arg type, the match should be against the base type of the arg type, since the arg type can have different bpf_type_flags set on it.
Does this need a fixes tag? Something around the following maybe:

Quoted 7 lines

Quoted 4 lines
Quoted 3 lines
Fixes d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.")
?
I will add that tag. Thanks!
Joanne and Stan, IMO this is not necessary. I think this change is a cleanup rather than a fix.

I don't see the bug easier.
The helper types that are compared directly as arg_type instead of base_type(arg_type) were all without flags so far. So I don't think the patch changes behavior or fixes anything today. It looks like a good future proofing change though.
Am I missing something?

Hao Luo

Acked-by

2022-07-13 01:39 UTC #5 lore raw

Quoted 24 lines · Alexei Starovoitov

Quoted 14 lines · Hao Luo
Quoted 12 lines · Joanne Koong
Quoted 10 lines · On Tue, Jul 12, 2022 at 3:44 PM <[email protected]> wrote:
Quoted 5 lines · Joanne Koong
This patch does two things:
1. For matching against the arg type, the match should be against the base type of the arg type, since the arg type can have different bpf_type_flags set on it.
Does this need a fixes tag? Something around the following maybe:
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Fixes d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.")
?
I will add that tag. Thanks!
Joanne and Stan, IMO this is not necessary. I think this change is a cleanup rather than a fix.

Quoted 6 lines

I don't see the bug easier.
The helper types that are compared directly as arg_type instead of base_type(arg_type) were all without flags so far. So I don't think the patch changes behavior or fixes anything today. It looks like a good future proofing change though.
Am I missing something?

Agree, I mean adding a fixes tag isn't necessary and the patch is toward a good direction. As long as the selftests pass on this patch, it looks good to me.

Acked-by Hao Luo <[email protected]>

Stanislav Fomichev

2022-07-13 02:10 UTC #6 lore raw

Quoted 34 lines · Hao Luo

Quoted 24 lines · Alexei Starovoitov
Quoted 14 lines · Hao Luo
Quoted 12 lines · Joanne Koong
Quoted 10 lines · On Tue, Jul 12, 2022 at 3:44 PM <[email protected]> wrote:
Quoted 5 lines · Joanne Koong
This patch does two things:
1. For matching against the arg type, the match should be against the base type of the arg type, since the arg type can have different bpf_type_flags set on it.
Does this need a fixes tag? Something around the following maybe:
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Fixes d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.")
?
I will add that tag. Thanks!
Joanne and Stan, IMO this is not necessary. I think this change is a cleanup rather than a fix.
Quoted 6 lines
I don't see the bug easier.
The helper types that are compared directly as arg_type instead of base_type(arg_type) were all without flags so far. So I don't think the patch changes behavior or fixes anything today. It looks like a good future proofing change though.
Am I missing something?

Quoted 5 lines

Agree, I mean adding a fixes tag isn't necessary and the patch is toward a good direction. As long as the selftests pass on this patch, it looks good to me.
Acked-by Hao Luo <[email protected]>

Perfect, thanks for clarifying! It wasn't clear from the description so I started looking for where that base_type() came from.

Joanne Koong

author

2022-07-13 20:09 UTC #7 lore raw

Quoted 36 lines · Stanislav Fomichev

Quoted 34 lines · Hao Luo
Quoted 30 lines · Alexei Starovoitov
Quoted 14 lines · Hao Luo
Quoted 12 lines · Joanne Koong
Quoted 10 lines · On Tue, Jul 12, 2022 at 3:44 PM <[email protected]> wrote:
Quoted 5 lines · Joanne Koong
This patch does two things:
1. For matching against the arg type, the match should be against the base type of the arg type, since the arg type can have different bpf_type_flags set on it.
Does this need a fixes tag? Something around the following maybe:
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Fixes d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.")
?
I will add that tag. Thanks!
Joanne and Stan, IMO this is not necessary. I think this change is a cleanup rather than a fix.
I don't see the bug easier.
The helper types that are compared directly as arg_type instead of base_type(arg_type) were all without flags so far. So I don't think the patch changes behavior or fixes anything today. It looks like a good future proofing change though.
Am I missing something?

Quoted 8 lines

Quoted 5 lines
Agree, I mean adding a fixes tag isn't necessary and the patch is toward a good direction. As long as the selftests pass on this patch, it looks good to me.
Acked-by Hao Luo <[email protected]>
Perfect, thanks for clarifying! It wasn't clear from the description so I started looking for where that base_type() came from.

Great, I will leave it as is then. Thanks for the feedback.

[email protected]

CI Applied

2022-07-13 21:50 UTC #8 lore raw

In reply to Joanne Koong

This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <[email protected]>:

Hello:

This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <[email protected]>:

Quoted 9 lines · On Tue, 12 Jul 2022 14:06:03 -0700 you wrote:

This patch does two things:
1. For matching against the arg type, the match should be against the base type of the arg type, since the arg type can have different bpf_type_flags set on it.
2. Uses switch casing to improve readability + efficiency.
[...]

Here is the summary with links:
- [bpf-next,v1] bpf: Tidy up verifier check_func_arg()

    https://git.kernel.org/bpf/bpf-next/c/8ab4cdcf03d0

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html