| Message ID | 20210212205642.620788-4-me@ubique.spb.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | Add support of pointer to struct in global functions | expand |
On Fri, Feb 12, 2021 at 12:57 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote: > > Add an ability to pass a pointer to a type with known size in arguments > of a global function. Such pointers may be used to overcome the limit on > the maximum number of arguments, avoid expensive and tricky workarounds > and to have multiple output arguments. > > A referenced type may contain pointers but indirect access through them > isn't supported. > > The implementation consists of two parts. If a global function has an > argument that is a pointer to a type with known size then: > > 1) In btf_check_func_arg_match(): check that the corresponding > register points to NULL or to a valid memory region that is large enough > to contain the expected argument's type. > > 2) In btf_prepare_func_args(): set the corresponding register type to > PTR_TO_MEM_OR_NULL and its size to the size of the expected type. > > Only global functions are supported because allowance of pointers for > static functions might break validation. Consider the following > scenario. A static function has a pointer argument. A caller passes > pointer to its stack memory. Because the callee can change referenced > memory verifier cannot longer assume any particular slot type of the > caller's stack memory hence the slot type is changed to SLOT_MISC. If > there is an operation that relies on slot type other than SLOT_MISC then > verifier won't be able to infer safety of the operation. > > When verifier sees a static function that has a pointer argument > different from PTR_TO_CTX then it skips arguments check and continues > with "inline" validation with more information available. The operation > that relies on the particular slot type now succeeds. > > Because global functions were not allowed to have pointer arguments > different from PTR_TO_CTX it's not possible to break existing and valid > code. > > Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru> > --- Acked-by: Andrii Nakryiko <andrii@kernel.org> > include/linux/bpf_verifier.h | 2 ++ > kernel/bpf/btf.c | 55 +++++++++++++++++++++++++++++------- > kernel/bpf/verifier.c | 30 ++++++++++++++++++++ > 3 files changed, 77 insertions(+), 10 deletions(-) > [...]
On Sat, Feb 13, 2021 at 12:56:41AM +0400, Dmitrii Banshchikov wrote: > Add an ability to pass a pointer to a type with known size in arguments > of a global function. Such pointers may be used to overcome the limit on > the maximum number of arguments, avoid expensive and tricky workarounds > and to have multiple output arguments. Thanks a lot for adding this feature and exhaustive tests. It's a massive improvement in function-by-function verification. Hopefully it will increase its adoption. I've applied the set to bpf-next. > @@ -5349,10 +5352,6 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, > goto out; > } > if (btf_type_is_ptr(t)) { > - if (reg->type == SCALAR_VALUE) { > - bpf_log(log, "R%d is not a pointer\n", i + 1); > - goto out; > - } Thanks for nuking this annoying warning along the way. People complained that the verification log for normal static functions contains above inexplicable message. > /* If function expects ctx type in BTF check that caller > * is passing PTR_TO_CTX. > */ > @@ -5367,6 +5366,25 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, > goto out; > continue; > } > + > + if (!is_global) > + goto out; > + > + t = btf_type_skip_modifiers(btf, t->type, NULL); > + > + ref_t = btf_resolve_size(btf, t, &type_size); > + if (IS_ERR(ref_t)) { > + bpf_log(log, > + "arg#%d reference type('%s %s') size cannot be determined: %ld\n", > + i, btf_type_str(t), btf_name_by_offset(btf, t->name_off), > + PTR_ERR(ref_t)); Hopefully one annoying message won't get replaced with this annoying message :) I think the type size should be known most of the time. So it should be fine. > + if (btf_type_is_ptr(t)) { > + if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) { > + reg->type = PTR_TO_CTX; > + continue; > + } Do you think it would make sense to nuke another message in btf_get_prog_ctx_type ? With this newly gained usability of global function the message "arg#0 type is not a struct" is not useful. It was marginally useful in the past. Because global funcs supported ptr_to_ctx only it wasn't seen as often. Now this message probably can simply be removed. wdyt?
On Fri, Feb 12, 2021 at 06:09:37PM -0800, Alexei Starovoitov wrote: > On Sat, Feb 13, 2021 at 12:56:41AM +0400, Dmitrii Banshchikov wrote: > > Add an ability to pass a pointer to a type with known size in arguments > > of a global function. Such pointers may be used to overcome the limit on > > the maximum number of arguments, avoid expensive and tricky workarounds > > and to have multiple output arguments. > > Thanks a lot for adding this feature and exhaustive tests. > It's a massive improvement in function-by-function verification. > Hopefully it will increase its adoption. > I've applied the set to bpf-next. > > > @@ -5349,10 +5352,6 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, > > goto out; > > } > > if (btf_type_is_ptr(t)) { > > - if (reg->type == SCALAR_VALUE) { > > - bpf_log(log, "R%d is not a pointer\n", i + 1); > > - goto out; > > - } > > Thanks for nuking this annoying warning along the way. > People complained that the verification log for normal static functions > contains above inexplicable message. > > > /* If function expects ctx type in BTF check that caller > > * is passing PTR_TO_CTX. > > */ > > @@ -5367,6 +5366,25 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, > > goto out; > > continue; > > } > > + > > + if (!is_global) > > + goto out; > > + > > + t = btf_type_skip_modifiers(btf, t->type, NULL); > > + > > + ref_t = btf_resolve_size(btf, t, &type_size); > > + if (IS_ERR(ref_t)) { > > + bpf_log(log, > > + "arg#%d reference type('%s %s') size cannot be determined: %ld\n", > > + i, btf_type_str(t), btf_name_by_offset(btf, t->name_off), > > + PTR_ERR(ref_t)); > > Hopefully one annoying message won't get replaced with this annoying message :) > I think the type size should be known most of the time. So it should be fine. > > > + if (btf_type_is_ptr(t)) { > > + if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) { > > + reg->type = PTR_TO_CTX; > > + continue; > > + } > > Do you think it would make sense to nuke another message in btf_get_prog_ctx_type ? > With this newly gained usability of global function the message > "arg#0 type is not a struct" > is not useful. > It was marginally useful in the past. Because global funcs supported > ptr_to_ctx only it wasn't seen as often. > Now this message probably can simply be removed. wdyt? Yes, I hit this log message while was working on the patch and it looked confusing but forgot to adjust/remove it. I will prepare patch. Thank you.
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 532c97836d0d..971b33aca13d 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -471,6 +471,8 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt); int check_ctx_reg(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno); +int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, + u32 regno, u32 mem_size); /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */ static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog, diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index bd5d2c563693..2efeb5f4b343 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -5297,9 +5297,10 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, struct bpf_prog *prog = env->prog; struct btf *btf = prog->aux->btf; const struct btf_param *args; - const struct btf_type *t; - u32 i, nargs, btf_id; + const struct btf_type *t, *ref_t; + u32 i, nargs, btf_id, type_size; const char *tname; + bool is_global; if (!prog->aux->func_info) return -EINVAL; @@ -5333,6 +5334,8 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, bpf_log(log, "Function %s has %d > 5 args\n", tname, nargs); goto out; } + + is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; /* check that BTF function arguments match actual types that the * verifier sees. */ @@ -5349,10 +5352,6 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, goto out; } if (btf_type_is_ptr(t)) { - if (reg->type == SCALAR_VALUE) { - bpf_log(log, "R%d is not a pointer\n", i + 1); - goto out; - } /* If function expects ctx type in BTF check that caller * is passing PTR_TO_CTX. */ @@ -5367,6 +5366,25 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, goto out; continue; } + + if (!is_global) + goto out; + + t = btf_type_skip_modifiers(btf, t->type, NULL); + + ref_t = btf_resolve_size(btf, t, &type_size); + if (IS_ERR(ref_t)) { + bpf_log(log, + "arg#%d reference type('%s %s') size cannot be determined: %ld\n", + i, btf_type_str(t), btf_name_by_offset(btf, t->name_off), + PTR_ERR(ref_t)); + goto out; + } + + if (check_mem_reg(env, reg, i + 1, type_size)) + goto out; + + continue; } bpf_log(log, "Unrecognized arg#%d type %s\n", i, btf_kind_str[BTF_INFO_KIND(t->info)]); @@ -5397,7 +5415,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, enum bpf_prog_type prog_type = prog->type; struct btf *btf = prog->aux->btf; const struct btf_param *args; - const struct btf_type *t; + const struct btf_type *t, *ref_t; u32 i, nargs, btf_id; const char *tname; @@ -5470,9 +5488,26 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, reg->type = SCALAR_VALUE; continue; } - if (btf_type_is_ptr(t) && - btf_get_prog_ctx_type(log, btf, t, prog_type, i)) { - reg->type = PTR_TO_CTX; + if (btf_type_is_ptr(t)) { + if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) { + reg->type = PTR_TO_CTX; + continue; + } + + t = btf_type_skip_modifiers(btf, t->type, NULL); + + ref_t = btf_resolve_size(btf, t, ®->mem_size); + if (IS_ERR(ref_t)) { + bpf_log(log, + "arg#%d reference type('%s %s') size cannot be determined: %ld\n", + i, btf_type_str(t), btf_name_by_offset(btf, t->name_off), + PTR_ERR(ref_t)); + return -EINVAL; + } + + reg->type = PTR_TO_MEM_OR_NULL; + reg->id = ++env->id_gen; + continue; } bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n", diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e391ed325249..ed04999ae247 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4272,6 +4272,29 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, } } +int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, + u32 regno, u32 mem_size) +{ + if (register_is_null(reg)) + return 0; + + if (reg_type_may_be_null(reg->type)) { + /* Assuming that the register contains a value check if the memory + * access is safe. Temporarily save and restore the register's state as + * the conversion shouldn't be visible to a caller. + */ + const struct bpf_reg_state saved_reg = *reg; + int rv; + + mark_ptr_not_null_reg(reg); + rv = check_helper_mem_access(env, regno, mem_size, true, NULL); + *reg = saved_reg; + return rv; + } + + return check_helper_mem_access(env, regno, mem_size, true, NULL); +} + /* Implementation details: * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL * Two bpf_map_lookups (even with the same key) will have different reg->id. @@ -11941,6 +11964,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog) mark_reg_known_zero(env, regs, i); else if (regs[i].type == SCALAR_VALUE) mark_reg_unknown(env, regs, i); + else if (regs[i].type == PTR_TO_MEM_OR_NULL) { + const u32 mem_size = regs[i].mem_size; + + mark_reg_known_zero(env, regs, i); + regs[i].mem_size = mem_size; + regs[i].id = ++env->id_gen; + } } } else { /* 1st arg to a function */
Add an ability to pass a pointer to a type with known size in arguments of a global function. Such pointers may be used to overcome the limit on the maximum number of arguments, avoid expensive and tricky workarounds and to have multiple output arguments. A referenced type may contain pointers but indirect access through them isn't supported. The implementation consists of two parts. If a global function has an argument that is a pointer to a type with known size then: 1) In btf_check_func_arg_match(): check that the corresponding register points to NULL or to a valid memory region that is large enough to contain the expected argument's type. 2) In btf_prepare_func_args(): set the corresponding register type to PTR_TO_MEM_OR_NULL and its size to the size of the expected type. Only global functions are supported because allowance of pointers for static functions might break validation. Consider the following scenario. A static function has a pointer argument. A caller passes pointer to its stack memory. Because the callee can change referenced memory verifier cannot longer assume any particular slot type of the caller's stack memory hence the slot type is changed to SLOT_MISC. If there is an operation that relies on slot type other than SLOT_MISC then verifier won't be able to infer safety of the operation. When verifier sees a static function that has a pointer argument different from PTR_TO_CTX then it skips arguments check and continues with "inline" validation with more information available. The operation that relies on the particular slot type now succeeds. Because global functions were not allowed to have pointer arguments different from PTR_TO_CTX it's not possible to break existing and valid code. Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru> --- include/linux/bpf_verifier.h | 2 ++ kernel/bpf/btf.c | 55 +++++++++++++++++++++++++++++------- kernel/bpf/verifier.c | 30 ++++++++++++++++++++ 3 files changed, 77 insertions(+), 10 deletions(-)