On Tue, Nov 15, 2022 at 11:40:50AM IST, Alexei Starovoitov wrote:
> On Tue, Nov 15, 2022 at 12:45:35AM +0530, Kumar Kartikeya Dwivedi wrote:
> > As we continue to add more features, argument types, kfunc flags, and
> > different extensions to kfuncs, the code to verify the correctness of
> > the kfunc prototype wrt the passed in registers has become ad-hoc and
> > ugly to read. To make life easier, and make a very clear split between
> > different stages of argument processing, move all the code into
> > verifier.c and refactor into easier to read helpers and functions.
> >
> > This also makes sharing code within the verifier easier with kfunc
> > argument processing. This will be more and more useful in later patches
> > as we are now moving to implement very core BPF helpers as kfuncs, to
> > keep them experimental before baking into UAPI.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > [...]
> > +/* Returns true if struct is composed of scalars, 4 levels of nesting allowed */
> > +static bool __btf_type_is_scalar_struct(struct bpf_verifier_env *env,
> > + const struct btf *btf,
> > + const struct btf_type *t, int rec)
> > +{
> > + const struct btf_type *member_type;
> > + const struct btf_member *member;
> > + u32 i;
> > +
> > + if (!btf_type_is_struct(t))
> > + return false;
> > +
> > + for_each_member(i, t, member) {
> > + const struct btf_array *array;
> > +
> > + member_type = btf_type_skip_modifiers(btf, member->type, NULL);
> > + if (btf_type_is_struct(member_type)) {
> > + if (rec >= 3) {
> > + verbose(env, "max struct nesting depth exceeded\n");
> > + return false;
> > + }
> > + if (!__btf_type_is_scalar_struct(env, btf, member_type, rec + 1))
> > + return false;
> > + continue;
> > + }
> > + if (btf_type_is_array(member_type)) {
> > + array = btf_array(member_type);
> > + if (!array->nelems)
> > + return false;
> > + member_type = btf_type_skip_modifiers(btf, array->type, NULL);
> > + if (!btf_type_is_scalar(member_type))
> > + return false;
> > + continue;
> > + }
> > + if (!btf_type_is_scalar(member_type))
> > + return false;
> > + }
> > + return true;
> > +}
>
> Deleting the code from the next patch can be combined with this patch,
> since it's a pure code move?
>
> Similar to few funcs that do pure code move... it's better to have them
> in a single patch.
>
> Not sure about 2 patch split strategy in general.
> Not clear whether it helps or hurts the code review.
>
No problem, I can squash both into a single patch.
> > +
> > +
> > +static u32 *reg2btf_ids[__BPF_REG_TYPE_MAX] = {
> > +#ifdef CONFIG_NET
> > + [PTR_TO_SOCKET] = &btf_sock_ids[BTF_SOCK_TYPE_SOCK],
> > + [PTR_TO_SOCK_COMMON] = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> > + [PTR_TO_TCP_SOCK] = &btf_sock_ids[BTF_SOCK_TYPE_TCP],
> > +#endif
> > +};
> > +
> > +enum kfunc_ptr_arg_type {
> > + KF_ARG_PTR_TO_CTX,
> > + KF_ARG_PTR_TO_KPTR_STRONG, /* PTR_TO_KPTR but type specific */
>
> What does the STRONG suffix signify?
>
> PTR_TO_KPTR should always be safe.
> Just to make it different from ARG_PTR_TO_KPTR in kptr_xchg that
> has 'void *' arg and "auto converts" the type?
>
Yes.
> Here STRONG means that the type of the arg should match?
Yes.
>
> I think it's too verbose.
> Just KF_ARG_PTR_TO_KPTR would be clear enough.
> If we ever have another kptr_xchg that is done as kfunc
> we can add KF_ARG_PTR_TO_KPTR_AUTO or some other name that we can bikeshed later.
>
Ack, I'll drop the _STRONG suffix and simply call it ARG_PTR_TO_KPTR.
> > + KF_ARG_PTR_TO_DYNPTR,
> > + KF_ARG_PTR_TO_BTF_ID, /* Also covers reg2btf_ids conversions */
> > + KF_ARG_PTR_TO_MEM,
> > + KF_ARG_PTR_TO_MEM_SIZE, /* Size derived from next argument, skip it */
> > +};
> > +
> > +static enum kfunc_ptr_arg_type
> > +get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
> > + struct bpf_kfunc_call_arg_meta *meta,
> > + const struct btf_type *t, const struct btf_type *ref_t,
> > + const char *ref_tname, const struct btf_param *args,
> > + int argno, int nargs)
> > +{
> > + u32 regno = argno + 1;
> > + struct bpf_reg_state *regs = cur_regs(env);
> > + struct bpf_reg_state *reg = ®s[regno];
> > + bool arg_mem_size = false;
> > +
> > + /* In this function, we verify the kfunc's BTF as per the argument type,
> > + * leaving the rest of the verification with respect to the register
> > + * type to our caller. When a set of conditions hold in the BTF type of
> > + * arguments, we resolve it to a known kfunc_ptr_arg_type.
> > + */
> > + if (btf_get_prog_ctx_type(&env->log, meta->btf, t, resolve_prog_type(env->prog), argno))
> > + return KF_ARG_PTR_TO_CTX;
> > +
> > + if (is_kfunc_arg_kptr_get(meta, argno)) {
> > + if (!btf_type_is_ptr(ref_t)) {
> > + verbose(env, "arg#0 BTF type must be a double pointer for kptr_get kfunc\n");
> > + return -EINVAL;
> > + }
> > + ref_t = btf_type_by_id(meta->btf, ref_t->type);
> > + ref_tname = btf_name_by_offset(meta->btf, ref_t->name_off);
> > + if (!btf_type_is_struct(ref_t)) {
> > + verbose(env, "kernel function %s args#0 pointer type %s %s is not supported\n",
> > + meta->func_name, btf_type_str(ref_t), ref_tname);
> > + return -EINVAL;
> > + }
> > + return KF_ARG_PTR_TO_KPTR_STRONG;
> > + }
> > +
> > + if (is_kfunc_arg_dynptr(meta->btf, &args[argno]))
> > + return KF_ARG_PTR_TO_DYNPTR;
> > +
> > + if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) {
> > + if (!btf_type_is_struct(ref_t)) {
> > + verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n",
> > + meta->func_name, argno, btf_type_str(ref_t), ref_tname);
> > + return -EINVAL;
> > + }
> > + return KF_ARG_PTR_TO_BTF_ID;
> > + }
> > +
> > + if (argno + 1 < nargs && is_kfunc_arg_mem_size(meta->btf, &args[argno + 1], ®s[regno + 1]))
> > + arg_mem_size = true;
> > +
> > + /* This is the catch all argument type of register types supported by
> > + * check_helper_mem_access. However, we only allow when argument type is
> > + * pointer to scalar, or struct composed (recursively) of scalars. When
> > + * arg_mem_size is true, the pointer can be void *.
> > + */
> > + if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(env, meta->btf, ref_t, 0) &&
> > + (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> > + verbose(env, "arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
> > + argno, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
> > + return -EINVAL;
> > + }
> > + return arg_mem_size ? KF_ARG_PTR_TO_MEM_SIZE : KF_ARG_PTR_TO_MEM;
> > +}
> > +
> > +static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env,
> > + struct bpf_reg_state *reg,
> > + const struct btf_type *ref_t,
> > + const char *ref_tname, u32 ref_id,
> > + struct bpf_kfunc_call_arg_meta *meta,
> > + int argno)
> > +{
> > + const struct btf_type *reg_ref_t;
> > + bool strict_type_match = false;
> > + const struct btf *reg_btf;
> > + const char *reg_ref_tname;
> > + u32 reg_ref_id;
> > +
> > + if (reg->type == PTR_TO_BTF_ID) {
> > + reg_btf = reg->btf;
> > + reg_ref_id = reg->btf_id;
> > + } else {
> > + reg_btf = btf_vmlinux;
> > + reg_ref_id = *reg2btf_ids[base_type(reg->type)];
> > + }
> > +
> > + if (is_kfunc_trusted_args(meta) || (is_kfunc_release(meta) && reg->ref_obj_id))
> > + strict_type_match = true;
> > +
> > + reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id, ®_ref_id);
> > + reg_ref_tname = btf_name_by_offset(reg_btf, reg_ref_t->name_off);
> > + if (!btf_struct_ids_match(&env->log, reg_btf, reg_ref_id, reg->off, meta->btf, ref_id, strict_type_match)) {
> > + verbose(env, "kernel function %s args#%d expected pointer to %s %s but R%d has a pointer to %s %s\n",
> > + meta->func_name, argno, btf_type_str(ref_t), ref_tname, argno + 1,
> > + btf_type_str(reg_ref_t), reg_ref_tname);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int process_kf_arg_ptr_to_kptr_strong(struct bpf_verifier_env *env,
> > + struct bpf_reg_state *reg,
> > + const struct btf_type *ref_t,
> > + const char *ref_tname,
> > + struct bpf_kfunc_call_arg_meta *meta,
> > + int argno)
> > +{
> > + struct btf_field *kptr_field;
> > +
> > + /* check_func_arg_reg_off allows var_off for
> > + * PTR_TO_MAP_VALUE, but we need fixed offset to find
> > + * off_desc.
> > + */
> > + if (!tnum_is_const(reg->var_off)) {
> > + verbose(env, "arg#0 must have constant offset\n");
> > + return -EINVAL;
> > + }
> > +
> > + kptr_field = btf_record_find(reg->map_ptr->record, reg->off + reg->var_off.value, BPF_KPTR);
> > + if (!kptr_field || kptr_field->type != BPF_KPTR_REF) {
> > + verbose(env, "arg#0 no referenced kptr at map value offset=%llu\n",
> > + reg->off + reg->var_off.value);
> > + return -EINVAL;
> > + }
> > +
> > + if (!btf_struct_ids_match(&env->log, meta->btf, ref_t->type, 0, kptr_field->kptr.btf,
> > + kptr_field->kptr.btf_id, true)) {
> > + verbose(env, "kernel function %s args#%d expected pointer to %s %s\n",
> > + meta->func_name, argno, btf_type_str(ref_t), ref_tname);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta)
> > +{
> > + const char *func_name = meta->func_name, *ref_tname;
> > + const struct btf *btf = meta->btf;
> > + const struct btf_param *args;
> > + u32 i, nargs;
> > + int ret;
> > +
> > + args = (const struct btf_param *)(meta->func_proto + 1);
> > + nargs = btf_type_vlen(meta->func_proto);
> > + if (nargs > MAX_BPF_FUNC_REG_ARGS) {
> > + verbose(env, "Function %s has %d > %d args\n", func_name, nargs,
> > + MAX_BPF_FUNC_REG_ARGS);
> > + return -EINVAL;
> > + }
> > +
> > + /* Check that BTF function arguments match actual types that the
> > + * verifier sees.
> > + */
> > + for (i = 0; i < nargs; i++) {
> > + struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[i + 1];
> > + const struct btf_type *t, *ref_t, *resolve_ret;
> > + enum bpf_arg_type arg_type = ARG_DONTCARE;
> > + u32 regno = i + 1, ref_id, type_size;
> > + bool is_ret_buf_sz = false;
> > + int kf_arg_type;
> > +
> > + t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> > + if (btf_type_is_scalar(t)) {
> > + if (reg->type != SCALAR_VALUE) {
> > + verbose(env, "R%d is not a scalar\n", regno);
> > + return -EINVAL;
> > + }
> > + if (is_kfunc_arg_ret_buf_size(btf, &args[i], reg, "rdonly_buf_size")) {
> > + meta->r0_rdonly = true;
> > + is_ret_buf_sz = true;
> > + } else if (is_kfunc_arg_ret_buf_size(btf, &args[i], reg, "rdwr_buf_size")) {
> > + is_ret_buf_sz = true;
> > + }
>
> is_kfunc_arg_ret_buf_size() is more generic than its name says so.
> Maybe is_scalar_arg_with_name() ?
>
I named it is_kfunc_scalar_arg_with_name for consistency.
> Also looks wrong to triple check inside it:
> + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> + return false;
> when there was a check above.
>
Right, this was in the original code that I copied over, but it's no longer
needed given where it's called now.
