| Message ID | 20220428211059.4065379-2-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynamic pointers | expand |
Hi Joanne, On Thu, Apr 28, 2022 at 02:10:54PM -0700, Joanne Koong wrote: > @@ -5523,7 +5517,6 @@ static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } } > static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > - [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, > [ARG_CONST_SIZE] = &scalar_types, > [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > @@ -5537,7 +5530,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, > [ARG_PTR_TO_MEM] = &mem_types, > - [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > [ARG_PTR_TO_INT] = &int_ptr_types, > [ARG_PTR_TO_LONG] = &int_ptr_types, > @@ -5711,8 +5703,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > return -EACCES; > } Could (or should) this go in a separate patch? Even with removing ARG_PTR_TO_UNINIT_MAP_VALUE, it seems like this could stand on its own. Not sure what the norm is for how granular to split patches for bpf, so even if it could go in its own patch, feel free to disregard if you think splitting this off is excessive / unnecessary. > - } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE || > - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) { > + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) { > if (type_may_be_null(arg_type) && register_is_null(reg)) > return 0; > > @@ -5811,7 +5801,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > verbose(env, "invalid map_ptr to access map->value\n"); > return -EACCES; > } > - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE); > + meta->raw_mode = arg_type & MEM_UNINIT; Given that we're stashing in a bool here, should this be: meta->raw_mode = (arg_type & MEM_UNINIT) != 0; > err = check_helper_mem_access(env, regno, > meta->map_ptr->value_size, false, > meta); > @@ -5838,11 +5828,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > return -EACCES; > } else if (arg_type == ARG_PTR_TO_FUNC) { > meta->subprogno = reg->subprogno; > - } else if (arg_type_is_mem_ptr(arg_type)) { > + } else if (base_type(arg_type) == ARG_PTR_TO_MEM) { > /* The access to this pointer is only checked when we hit the > * next is_mem_size argument below. > */ > - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM); > + meta->raw_mode = arg_type & MEM_UNINIT; Same here. > } else if (arg_type_is_mem_size(arg_type)) { > bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO); > > @@ -6189,9 +6179,9 @@ static bool check_raw_mode_ok(const struct bpf_func_proto *fn) > static bool check_args_pair_invalid(enum bpf_arg_type arg_curr, > enum bpf_arg_type arg_next) > { > - return (arg_type_is_mem_ptr(arg_curr) && > + return (base_type(arg_curr) == ARG_PTR_TO_MEM && > !arg_type_is_mem_size(arg_next)) || > - (!arg_type_is_mem_ptr(arg_curr) && > + (base_type(arg_curr) != ARG_PTR_TO_MEM && > arg_type_is_mem_size(arg_next)); > } What do you think about this as a possibly more concise way to express that the curr and next args differ? return (base_type(arg_curr) == ARG_PTR_TO_MEM) != arg_type_is_mem_size(arg_next); Looks great overall! Thanks, David
On Fri, May 06, 2022 at 12:09:37PM -0700, Joanne Koong wrote: > I think the bpf philosophy leans more towards conflating related-ish > patches into the same patchset. I think this patch could be its own > stand-alone patchset, but it's also related to the dynptr patchset in that > dynptrs need it to properly describe its initialization helper functions. > I'm happy to submit this as its own patchset though if that is preferred :) Totally up to you, if that's the BPF convention then that's fine with me. > > > - } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE || > > > - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) { > > > + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) { > > > if (type_may_be_null(arg_type) && register_is_null(reg)) > > > return 0; > > > > > > @@ -5811,7 +5801,7 @@ static int check_func_arg(struct bpf_verifier_env > > *env, u32 arg, > > > verbose(env, "invalid map_ptr to access > > map->value\n"); > > > return -EACCES; > > > } > > > - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE); > > > + meta->raw_mode = arg_type & MEM_UNINIT; > > > > Given that we're stashing in a bool here, should this be: > > > > meta->raw_mode = (arg_type & MEM_UNINIT) != 0; > > > I think just arg_type & MEM_UNINIT is okay because it implicitly converts > from 1 -> true, 0 -> false. This is the convention that's used elsewhere in > the linux codebase as well Yeah I think functionally it will work just fine as is. I saw that a few other places in verifier.c use operators that explicitly make the result 0 or 1, e.g.: 14699 14700 env->strict_alignment = !!(attr->prog_flags & BPF_F_STRICT_ALIGNMENT); But the compiler will indeed implicitly convert any nonzero value to 1 if it's stored in a bool, so it's not necessary for correctness. It looks like the kernel style guide also implies that using the extra operators isn't necessary, so I think we can leave it as you have it now: https://www.kernel.org/doc/html/latest/process/coding-style.html#using-bool > > What do you think about this as a possibly more concise way to express that > > the curr and next args differ? > > > > return (base_type(arg_curr) == ARG_PTR_TO_MEM) != > > arg_type_is_mem_size(arg_next); > > > I was trying to decide between this and the more verbose expression above > and ultimately went with the more verbose expression because it seemed more > readable to me. But I don't feel strongly :) I'm cool with either one I don't feel strongly either, if you think your way is more readable then don't feel obligated to change it. Thanks, David
On Thu, Apr 28, 2022 at 2:12 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > Instead of having uninitialized versions of arguments as separate > bpf_arg_types (eg ARG_PTR_TO_UNINIT_MEM as the uninitialized version > of ARG_PTR_TO_MEM), we can instead use MEM_UNINIT as a bpf_type_flag > modifier to denote that the argument is uninitialized. > > Doing so cleans up some of the logic in the verifier. We no longer > need to do two checks against an argument type (eg "if > (base_type(arg_type) == ARG_PTR_TO_MEM || base_type(arg_type) == > ARG_PTR_TO_UNINIT_MEM)"), since uninitialized and initialized > versions of the same argument type will now share the same base type. > > In the near future, MEM_UNINIT will be used by dynptr helper functions > as well. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- LGTM, see minor suggestion below Acked-by: Andrii Nakryiko <andrii@kernel.org> > include/linux/bpf.h | 17 +++++++++-------- > kernel/bpf/helpers.c | 4 ++-- > kernel/bpf/verifier.c | 26 ++++++++------------------ > 3 files changed, 19 insertions(+), 28 deletions(-) > [...] > @@ -6189,9 +6179,9 @@ static bool check_raw_mode_ok(const struct bpf_func_proto *fn) > static bool check_args_pair_invalid(enum bpf_arg_type arg_curr, > enum bpf_arg_type arg_next) > { > - return (arg_type_is_mem_ptr(arg_curr) && > + return (base_type(arg_curr) == ARG_PTR_TO_MEM && > !arg_type_is_mem_size(arg_next)) || > - (!arg_type_is_mem_ptr(arg_curr) && > + (base_type(arg_curr) != ARG_PTR_TO_MEM && > arg_type_is_mem_size(arg_next)); trying to parse this check I realized that it can be written as != (basically it's a XOR, both conditions are either true or both are false) return (base_type(arg_curr) == ARG_PTR_TO_MEM) != arg_type_is_mem_size(arg_next); > } > > @@ -6203,7 +6193,7 @@ static bool check_arg_pair_ok(const struct bpf_func_proto *fn) > * helper function specification. > */ > if (arg_type_is_mem_size(fn->arg1_type) || > - arg_type_is_mem_ptr(fn->arg5_type) || > + base_type(fn->arg5_type) == ARG_PTR_TO_MEM || > check_args_pair_invalid(fn->arg1_type, fn->arg2_type) || > check_args_pair_invalid(fn->arg2_type, fn->arg3_type) || > check_args_pair_invalid(fn->arg3_type, fn->arg4_type) || > -- > 2.30.2 >
On Fri, May 6, 2022 at 1:32 PM David Vernet <void@manifault.com> wrote: > > On Fri, May 06, 2022 at 12:09:37PM -0700, Joanne Koong wrote: > > I think the bpf philosophy leans more towards conflating related-ish > > patches into the same patchset. I think this patch could be its own > > stand-alone patchset, but it's also related to the dynptr patchset in that > > dynptrs need it to properly describe its initialization helper functions. > > I'm happy to submit this as its own patchset though if that is preferred :) > > Totally up to you, if that's the BPF convention then that's fine with me. You meant - [ARG_PTR_TO_UNINIT_MEM] = &mem_types, parts as stand-alone patch? That would be invalid on its own without adding MEM_UNINT, so would potentially break bisection. So no, it shouldn't be a stand-alone patch. Each patch has to be logically separate but not causing any regressions in behavior, compilation, selftest, etc. So, for example, while we normally put selftests into separate tests, if kernel change breaks selftests, selftests have to be fixed in the same patch to avoid having any point where bisection can detect the breakage. > > > > > > - } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE || > > > > - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) { > > > > + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) { > > > > if (type_may_be_null(arg_type) && register_is_null(reg)) > > > > return 0; > > > > > > > > @@ -5811,7 +5801,7 @@ static int check_func_arg(struct bpf_verifier_env > > > *env, u32 arg, > > > > verbose(env, "invalid map_ptr to access > > > map->value\n"); > > > > return -EACCES; > > > > } > > > > - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE); > > > > + meta->raw_mode = arg_type & MEM_UNINIT; > > > > > > Given that we're stashing in a bool here, should this be: > > > > > > meta->raw_mode = (arg_type & MEM_UNINIT) != 0; > > > > > I think just arg_type & MEM_UNINIT is okay because it implicitly converts > > from 1 -> true, 0 -> false. This is the convention that's used elsewhere in > > the linux codebase as well > > Yeah I think functionally it will work just fine as is. I saw that a few > other places in verifier.c use operators that explicitly make the result 0 > or 1, e.g.: > > 14699 > 14700 env->strict_alignment = !!(attr->prog_flags & BPF_F_STRICT_ALIGNMENT); > > But the compiler will indeed implicitly convert any nonzero value to 1 if > it's stored in a bool, so it's not necessary for correctness. It looks like > the kernel style guide also implies that using the extra operators isn't > necessary, so I think we can leave it as you have it now: > https://www.kernel.org/doc/html/latest/process/coding-style.html#using-bool Yeah, the above example is rather unusual, I'd say. We do !!(bool_expr) only when we want to assign that to integer (not bool) variable/field as 0 or 1. Otherwise it's a well-defined compiler conversion rule for any non-zero value to be true during bool conversion. > > > > What do you think about this as a possibly more concise way to express that > > > the curr and next args differ? > > > > > > return (base_type(arg_curr) == ARG_PTR_TO_MEM) != > > > arg_type_is_mem_size(arg_next); > > > > > I was trying to decide between this and the more verbose expression above > > and ultimately went with the more verbose expression because it seemed more > > readable to me. But I don't feel strongly :) I'm cool with either one > > I don't feel strongly either, if you think your way is more readable then > don't feel obligated to change it. > Heh, this also caught my eye. It's subjective, but inequality is shorter and more readable (even in terms of the logic it expresses). But it's fine either way with me. > Thanks, > David
On Fri, May 06, 2022 at 03:46:19PM -0700, Andrii Nakryiko wrote: > You meant > > - [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > parts as stand-alone patch? That would be invalid on its own without > adding MEM_UNINT, so would potentially break bisection. So no, it > shouldn't be a stand-alone patch. Each patch has to be logically > separate but not causing any regressions in behavior, compilation, > selftest, etc. So, for example, while we normally put selftests into > separate tests, if kernel change breaks selftests, selftests have to > be fixed in the same patch to avoid having any point where bisection > can detect the breakage. Thanks for clarifying, Andrii. I misunderstood the existing verifier code while I was reviewing the diff and mistakenly thought it was safe to remove these lines without MEM_UNINIT. Apologies for the noise / confusion.
On Fri, May 6, 2022 at 3:46 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, May 6, 2022 at 1:32 PM David Vernet <void@manifault.com> wrote: > > > > On Fri, May 06, 2022 at 12:09:37PM -0700, Joanne Koong wrote: > > > I think the bpf philosophy leans more towards conflating related-ish > > > patches into the same patchset. I think this patch could be its own > > > stand-alone patchset, but it's also related to the dynptr patchset in that > > > dynptrs need it to properly describe its initialization helper functions. > > > I'm happy to submit this as its own patchset though if that is preferred :) > > > > Totally up to you, if that's the BPF convention then that's fine with me. > > You meant > > - [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > parts as stand-alone patch? That would be invalid on its own without > adding MEM_UNINT, so would potentially break bisection. So no, it > shouldn't be a stand-alone patch. Each patch has to be logically > separate but not causing any regressions in behavior, compilation, > selftest, etc. So, for example, while we normally put selftests into > separate tests, if kernel change breaks selftests, selftests have to > be fixed in the same patch to avoid having any point where bisection > can detect the breakage. > Ah okay, I thought we were talking about having all of the first patch be its standalone patch. sorry for the confusion. > > > > > > > > > > - } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE || > > > > > - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) { > > > > > + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) { > > > > > if (type_may_be_null(arg_type) && register_is_null(reg)) > > > > > return 0; > > > > > > > > > > @@ -5811,7 +5801,7 @@ static int check_func_arg(struct bpf_verifier_env > > > > *env, u32 arg, > > > > > verbose(env, "invalid map_ptr to access > > > > map->value\n"); > > > > > return -EACCES; > > > > > } > > > > > - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE); > > > > > + meta->raw_mode = arg_type & MEM_UNINIT; > > > > > > > > Given that we're stashing in a bool here, should this be: > > > > > > > > meta->raw_mode = (arg_type & MEM_UNINIT) != 0; > > > > > > > I think just arg_type & MEM_UNINIT is okay because it implicitly converts > > > from 1 -> true, 0 -> false. This is the convention that's used elsewhere in > > > the linux codebase as well > > > > Yeah I think functionally it will work just fine as is. I saw that a few > > other places in verifier.c use operators that explicitly make the result 0 > > or 1, e.g.: > > > > 14699 > > 14700 env->strict_alignment = !!(attr->prog_flags & BPF_F_STRICT_ALIGNMENT); > > > > But the compiler will indeed implicitly convert any nonzero value to 1 if > > it's stored in a bool, so it's not necessary for correctness. It looks like > > the kernel style guide also implies that using the extra operators isn't > > necessary, so I think we can leave it as you have it now: > > https://www.kernel.org/doc/html/latest/process/coding-style.html#using-bool > > Yeah, the above example is rather unusual, I'd say. We do > !!(bool_expr) only when we want to assign that to integer (not bool) > variable/field as 0 or 1. Otherwise it's a well-defined compiler > conversion rule for any non-zero value to be true during bool > conversion. > > > > > > > What do you think about this as a possibly more concise way to express that > > > > the curr and next args differ? > > > > > > > > return (base_type(arg_curr) == ARG_PTR_TO_MEM) != > > > > arg_type_is_mem_size(arg_next); > > > > > > > I was trying to decide between this and the more verbose expression above > > > and ultimately went with the more verbose expression because it seemed more > > > readable to me. But I don't feel strongly :) I'm cool with either one > > > > I don't feel strongly either, if you think your way is more readable then > > don't feel obligated to change it. > > > > Heh, this also caught my eye. It's subjective, but inequality is > shorter and more readable (even in terms of the logic it expresses). > But it's fine either way with me. Since both of you think the inequality way is more readable, I will change it to inequality for v4 then :) > > > Thanks, > > David
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index be94833d390a..d0c167865504 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -389,7 +389,9 @@ enum bpf_type_flag { */ PTR_UNTRUSTED = BIT(6 + BPF_BASE_TYPE_BITS), - __BPF_TYPE_LAST_FLAG = PTR_UNTRUSTED, + MEM_UNINIT = BIT(7 + BPF_BASE_TYPE_BITS), + + __BPF_TYPE_LAST_FLAG = MEM_UNINIT, }; /* Max number of base types. */ @@ -408,16 +410,11 @@ enum bpf_arg_type { ARG_CONST_MAP_PTR, /* const argument used as pointer to bpf_map */ ARG_PTR_TO_MAP_KEY, /* pointer to stack used as map key */ ARG_PTR_TO_MAP_VALUE, /* pointer to stack used as map value */ - ARG_PTR_TO_UNINIT_MAP_VALUE, /* pointer to valid memory used to store a map value */ - /* the following constraints used to prototype bpf_memcmp() and other - * functions that access data on eBPF program stack + /* Used to prototype bpf_memcmp() and other functions that access data + * on eBPF program stack */ ARG_PTR_TO_MEM, /* pointer to valid memory (stack, packet, map value) */ - ARG_PTR_TO_UNINIT_MEM, /* pointer to memory does not need to be initialized, - * helper function must fill all bytes or clear - * them in error case. - */ ARG_CONST_SIZE, /* number of bytes accessed from memory */ ARG_CONST_SIZE_OR_ZERO, /* number of bytes accessed from memory or 0 */ @@ -449,6 +446,10 @@ enum bpf_arg_type { ARG_PTR_TO_ALLOC_MEM_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_ALLOC_MEM, ARG_PTR_TO_STACK_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_STACK, ARG_PTR_TO_BTF_ID_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_BTF_ID, + /* pointer to memory does not need to be initialized, helper function must fill + * all bytes or clear them in error case. + */ + ARG_PTR_TO_UNINIT_MEM = MEM_UNINIT | ARG_PTR_TO_MEM, /* This must be the last entry. Its purpose is to ensure the enum is * wide enough to hold the higher bits reserved for bpf_type_flag. diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 3e709fed5306..8a2398ac14c2 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -103,7 +103,7 @@ const struct bpf_func_proto bpf_map_pop_elem_proto = { .gpl_only = false, .ret_type = RET_INTEGER, .arg1_type = ARG_CONST_MAP_PTR, - .arg2_type = ARG_PTR_TO_UNINIT_MAP_VALUE, + .arg2_type = ARG_PTR_TO_MAP_VALUE | MEM_UNINIT, }; BPF_CALL_2(bpf_map_peek_elem, struct bpf_map *, map, void *, value) @@ -116,7 +116,7 @@ const struct bpf_func_proto bpf_map_peek_elem_proto = { .gpl_only = false, .ret_type = RET_INTEGER, .arg1_type = ARG_CONST_MAP_PTR, - .arg2_type = ARG_PTR_TO_UNINIT_MAP_VALUE, + .arg2_type = ARG_PTR_TO_MAP_VALUE | MEM_UNINIT, }; const struct bpf_func_proto bpf_get_prandom_u32_proto = { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 813f6ee80419..4565684839f1 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5378,12 +5378,6 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, return 0; } -static bool arg_type_is_mem_ptr(enum bpf_arg_type type) -{ - return base_type(type) == ARG_PTR_TO_MEM || - base_type(type) == ARG_PTR_TO_UNINIT_MEM; -} - static bool arg_type_is_mem_size(enum bpf_arg_type type) { return type == ARG_CONST_SIZE || @@ -5523,7 +5517,6 @@ static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } } static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, - [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, [ARG_CONST_SIZE] = &scalar_types, [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, @@ -5537,7 +5530,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, [ARG_PTR_TO_MEM] = &mem_types, - [ARG_PTR_TO_UNINIT_MEM] = &mem_types, [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, [ARG_PTR_TO_INT] = &int_ptr_types, [ARG_PTR_TO_LONG] = &int_ptr_types, @@ -5711,8 +5703,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, return -EACCES; } - if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE || - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) { + if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) { err = resolve_map_arg_type(env, meta, &arg_type); if (err) return err; @@ -5798,8 +5789,7 @@ 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 || - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) { + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) { if (type_may_be_null(arg_type) && register_is_null(reg)) return 0; @@ -5811,7 +5801,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, verbose(env, "invalid map_ptr to access map->value\n"); return -EACCES; } - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE); + meta->raw_mode = arg_type & MEM_UNINIT; err = check_helper_mem_access(env, regno, meta->map_ptr->value_size, false, meta); @@ -5838,11 +5828,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, return -EACCES; } else if (arg_type == ARG_PTR_TO_FUNC) { meta->subprogno = reg->subprogno; - } else if (arg_type_is_mem_ptr(arg_type)) { + } else if (base_type(arg_type) == ARG_PTR_TO_MEM) { /* The access to this pointer is only checked when we hit the * next is_mem_size argument below. */ - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM); + meta->raw_mode = arg_type & MEM_UNINIT; } else if (arg_type_is_mem_size(arg_type)) { bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO); @@ -6189,9 +6179,9 @@ static bool check_raw_mode_ok(const struct bpf_func_proto *fn) static bool check_args_pair_invalid(enum bpf_arg_type arg_curr, enum bpf_arg_type arg_next) { - return (arg_type_is_mem_ptr(arg_curr) && + return (base_type(arg_curr) == ARG_PTR_TO_MEM && !arg_type_is_mem_size(arg_next)) || - (!arg_type_is_mem_ptr(arg_curr) && + (base_type(arg_curr) != ARG_PTR_TO_MEM && arg_type_is_mem_size(arg_next)); } @@ -6203,7 +6193,7 @@ static bool check_arg_pair_ok(const struct bpf_func_proto *fn) * helper function specification. */ if (arg_type_is_mem_size(fn->arg1_type) || - arg_type_is_mem_ptr(fn->arg5_type) || + base_type(fn->arg5_type) == ARG_PTR_TO_MEM || check_args_pair_invalid(fn->arg1_type, fn->arg2_type) || check_args_pair_invalid(fn->arg2_type, fn->arg3_type) || check_args_pair_invalid(fn->arg3_type, fn->arg4_type) ||
Instead of having uninitialized versions of arguments as separate bpf_arg_types (eg ARG_PTR_TO_UNINIT_MEM as the uninitialized version of ARG_PTR_TO_MEM), we can instead use MEM_UNINIT as a bpf_type_flag modifier to denote that the argument is uninitialized. Doing so cleans up some of the logic in the verifier. We no longer need to do two checks against an argument type (eg "if (base_type(arg_type) == ARG_PTR_TO_MEM || base_type(arg_type) == ARG_PTR_TO_UNINIT_MEM)"), since uninitialized and initialized versions of the same argument type will now share the same base type. In the near future, MEM_UNINIT will be used by dynptr helper functions as well. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> --- include/linux/bpf.h | 17 +++++++++-------- kernel/bpf/helpers.c | 4 ++-- kernel/bpf/verifier.c | 26 ++++++++------------------ 3 files changed, 19 insertions(+), 28 deletions(-)