Messages in this thread | | | Date | Fri, 15 Jul 2022 21:29:07 -0700 | Subject | Re: [PATCH bpf-next v6 05/23] bpf/verifier: allow kfunc to return an allocated mem | From | Yonghong Song <> |
| |
On 7/12/22 7:58 AM, Benjamin Tissoires wrote: > When a kfunc is not returning a pointer to a struct but to a plain type, > we can consider it is a valid allocated memory assuming that: > - one of the arguments is either called rdonly_buf_size or > rdwr_buf_size > - and this argument is a const from the caller point of view > > We can then use this parameter as the size of the allocated memory. > > The memory is either read-only or read-write based on the name > of the size parameter.
If I understand correctly, this permits a kfunc like int *kfunc(..., int rdonly_buf_size); ... int *p = kfunc(..., 20); so the 'p' points to a memory buffer with size 20.
This looks like a strange interface although probably there is a valid reason for this as I didn't participated in earlier discussions.
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > changes in v6: > - code review from Kartikeya: > - remove comment change that had no reasons to be > - remove handling of PTR_TO_MEM with kfunc releases > - introduce struct bpf_kfunc_arg_meta > - do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match > - reverted most of the changes in verifier.c > - make sure kfunc acquire is using a struct pointer, not just a plain > pointer > - also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free > the allocated memory > > changes in v5: > - updated PTR_TO_MEM comment in btf.c to match upstream > - make it read-only or read-write based on the name of size > > new in v4 > --- > include/linux/bpf.h | 10 ++++++- > include/linux/btf.h | 12 ++++++++ > kernel/bpf/btf.c | 67 ++++++++++++++++++++++++++++++++++++++++--- > kernel/bpf/verifier.c | 49 +++++++++++++++++++++++-------- > 4 files changed, 121 insertions(+), 17 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 2b21f2a3452f..5b8eadb6e7bc 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1916,12 +1916,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, > const char *func_name, > struct btf_func_model *m); > > +struct bpf_kfunc_arg_meta { > + u64 r0_size; > + bool r0_rdonly; > + int ref_obj_id; > + bool multiple_ref_obj_id; > +}; > + > struct bpf_reg_state; > int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > struct bpf_reg_state *regs); > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > - struct bpf_reg_state *regs); > + struct bpf_reg_state *regs, > + struct bpf_kfunc_arg_meta *meta); > int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, > struct bpf_reg_state *reg); > int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog, > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 1bfed7fa0428..31da4273c2ec 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -420,4 +420,16 @@ static inline int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dt > } > #endif > > +static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t) > +{ > + /* t comes in already as a pointer */ > + t = btf_type_by_id(btf, t->type); > + > + /* allow const */ > + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) > + t = btf_type_by_id(btf, t->type); > + > + return btf_type_is_struct(t); > +} > + > #endif > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 4423045b8ff3..552d7bc05a0c 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6168,10 +6168,36 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, > return true; > } > > +static bool btf_is_kfunc_arg_mem_size(const struct btf *btf, > + const struct btf_param *arg, > + const struct bpf_reg_state *reg, > + const char *name) > +{ > + int len, target_len = strlen(name); > + const struct btf_type *t; > + const char *param_name; > + > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > + return false; > + > + param_name = btf_name_by_offset(btf, arg->name_off); > + if (str_is_empty(param_name)) > + return false; > + len = strlen(param_name); > + if (len != target_len) > + return false; > + if (strncmp(param_name, name, target_len))
strcmp(param_name, name) is enough. len == target_len and both len and target_len is computed from strlen(...).
> + return false; > + > + return true; > +} > + > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > struct bpf_reg_state *regs, > - bool ptr_to_mem_ok) > + bool ptr_to_mem_ok, > + struct bpf_kfunc_arg_meta *kfunc_meta) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > struct bpf_verifier_log *log = &env->log; > @@ -6225,6 +6251,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > if (btf_type_is_scalar(t)) { > + if (is_kfunc && kfunc_meta) { > + bool is_buf_size = false; > + > + /* check for any const scalar parameter of name "rdonly_buf_size" > + * or "rdwr_buf_size" > + */ > + if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > + "rdonly_buf_size")) { > + kfunc_meta->r0_rdonly = true; > + is_buf_size = true; > + } else if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > + "rdwr_buf_size")) > + is_buf_size = true; > + > + if (is_buf_size) { > + if (kfunc_meta->r0_size) { > + bpf_log(log, "2 or more rdonly/rdwr_buf_size parameters for kfunc"); > + return -EINVAL; > + } > + > + kfunc_meta->r0_size = reg->var_off.value;
Did we check 'reg' is a constant somewhere?
> + } > + } > + > if (reg->type == SCALAR_VALUE) > continue; > bpf_log(log, "R%d is not a scalar\n", regno); > @@ -6246,6 +6296,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > if (ret < 0) > return ret; > > + /* kptr_get is only valid for kfunc */ > + if (kfunc_meta && reg->ref_obj_id) { > + /* check for any one ref_obj_id to keep track of memory */ > + if (kfunc_meta->ref_obj_id) > + kfunc_meta->multiple_ref_obj_id = true; > + kfunc_meta->ref_obj_id = reg->ref_obj_id; > + } > + > /* kptr_get is only true for kfunc */ > if (i == 0 && kptr_get) { > struct bpf_map_value_off_desc *off_desc; > @@ -6441,7 +6499,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > return -EINVAL; > > is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global); > + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL); > > /* Compiler optimizations can remove arguments from static functions > * or mismatched type can be passed into a global function. [...]
| |