| Message ID | 20220712210603.123791-1-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8ab4cdcf03d0b060fbf73f76460f199bbd759ff7 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v1] bpf: Tidy up verifier check_func_arg() | expand |
On 07/12, Joanne Koong 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. Does this need a fixes tag? Something around the following maybe: Fixes: d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.") ? > 2. Uses switch casing to improve readability + efficiency. > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > kernel/bpf/verifier.c | 66 +++++++++++++++++++++++++------------------ > 1 file changed, 38 insertions(+), 28 deletions(-) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 328cfab3af60..26e7e787c20a 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -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; > -- > 2.30.2
On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote: > > On 07/12, Joanne Koong 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. > > 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! > > > 2. Uses switch casing to improve readability + efficiency. > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > --- > > kernel/bpf/verifier.c | 66 +++++++++++++++++++++++++------------------ > > 1 file changed, 38 insertions(+), 28 deletions(-) > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 328cfab3af60..26e7e787c20a 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -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; > > -- > > 2.30.2 >
On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote: > > > > On 07/12, Joanne Koong 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. > > > > 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. > > > > > 2. Uses switch casing to improve readability + efficiency. > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > > --- [...] > >
On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote: > > On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote: > > > > > > On 07/12, Joanne Koong 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. > > > > > > 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. 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?
On Tue, Jul 12, 2022 at 6:26 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote: > > > > On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > > > > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote: > > > > > > > > On 07/12, Joanne Koong 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. > > > > > > > > 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. > > 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 <haoluo@google.com>
On Tue, Jul 12, 2022 at 6:39 PM Hao Luo <haoluo@google.com> wrote: > > On Tue, Jul 12, 2022 at 6:26 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote: > > > > > > On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > > > > > > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote: > > > > > > > > > > On 07/12, Joanne Koong 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. > > > > > > > > > > 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. > > > > 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 <haoluo@google.com> Perfect, thanks for clarifying! It wasn't clear from the description so I started looking for where that base_type() came from.
On Tue, Jul 12, 2022 at 7:10 PM Stanislav Fomichev <sdf@google.com> wrote: > > On Tue, Jul 12, 2022 at 6:39 PM Hao Luo <haoluo@google.com> wrote: > > > > On Tue, Jul 12, 2022 at 6:26 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote: > > > > > > > > On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > > > > > > > > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote: > > > > > > > > > > > > On 07/12, Joanne Koong 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. > > > > > > > > > > > > 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. > > > > > > 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 <haoluo@google.com> > > 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.
Hello: This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: 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!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 328cfab3af60..26e7e787c20a 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -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;
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 <joannelkoong@gmail.com> --- kernel/bpf/verifier.c | 66 +++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 28 deletions(-)