Re: [PATCH bpf-next v8 3/4] bpf: Create argument information for nullable arguments.

[Kui-Feng Lee <sinquersw@xxxxxxxxx>]

On 2/11/24 11:49, Martin KaFai Lau wrote:
> On 2/8/24 6:37 PM, thinker.li@xxxxxxxxx wrote:
> > +/* Prepare argument info for every nullable argument of a member of a
> > + * struct_ops type.
> > + *
> > + * Initialize a struct bpf_struct_ops_arg_info according to type info of
> > + * the arguments of a stub function. (Check kCFI for more information 
> > about
> > + * stub functions.)
> > + *
> > + * Each member in the struct_ops type has a struct 
> > bpf_struct_ops_arg_info
> > + * to provide an array of struct bpf_ctx_arg_aux, which in turn provides
> > + * the information that used by the verifier to check the arguments 
> > of the
> > + * BPF struct_ops program assigned to the member. Here, we only care 
> > about
> > + * the arguments that are marked as __nullable.
> > + *
> > + * The array of struct bpf_ctx_arg_aux is eventually assigned to
> > + * prog->aux->ctx_arg_info of BPF struct_ops programs and passed to the
> > + * verifier. (See check_struct_ops_btf_id())
> > + *
> > + * arg_info->info will be the list of struct bpf_ctx_arg_aux if 
> > success. If
> > + * fails, it will be kept untouched.
> > + */
> > +static int prepare_arg_info(struct btf *btf,
> > +                const char *st_ops_name,
> > +                const char *member_name,
> > +                const struct btf_type *func_proto,
> > +                struct bpf_struct_ops_arg_info *arg_info)
> > +{
> > +    const struct btf_type *stub_func_proto, *pointed_type;
> > +    const struct btf_param *stub_args, *args;
> > +    struct bpf_ctx_arg_aux *info, *info_buf;
> > +    u32 nargs, arg_no, info_cnt = 0;
> > +    s32 arg_btf_id;
> > +    int offset;
> > +
> > +    stub_func_proto = find_stub_func_proto(btf, st_ops_name, 
> > member_name);
> > +    if (!stub_func_proto)
> > +        return 0;
> > +
> > +    /* Check if the number of arguments of the stub function is the same
> > +     * as the number of arguments of the function pointer.
> > +     */
> > +    nargs = btf_type_vlen(func_proto);
> > +    if (nargs != btf_type_vlen(stub_func_proto)) {
> > +        pr_warn("the number of arguments of the stub function %s__%s 
> > does not match the number of arguments of the member %s of struct %s\n",
> > +            st_ops_name, member_name, member_name, st_ops_name);
> > +        return -EINVAL;
> > +    }
> > +
> > +    args = btf_params(func_proto);
> > +    stub_args = btf_params(stub_func_proto);
> > +
> > +    info_buf = kcalloc(nargs, sizeof(*info_buf), GFP_KERNEL);
> > +    if (!info_buf)
> > +        return -ENOMEM;
> > +
> > +    /* Prepare info for every nullable argument */
> > +    info = info_buf;
> > +    for (arg_no = 0; arg_no < nargs; arg_no++) {
> > +        /* Skip arguments that is not suffixed with
> > +         * "__nullable".
> > +         */
> > +        if (!btf_param_match_suffix(btf, &stub_args[arg_no],
> > +                        MAYBE_NULL_SUFFIX))
> > +            continue;
> > +
> > +        /* Should be a pointer to struct */
> > +        pointed_type = btf_type_resolve_ptr(btf,
> > +                            args[arg_no].type,
> > +                            &arg_btf_id);
> > +        if (!pointed_type ||
> > +            !btf_type_is_struct(pointed_type)) {
> > +            pr_warn("stub function %s__%s has %s tagging to an 
> > unsupported type\n",
> > +                st_ops_name, member_name, MAYBE_NULL_SUFFIX);
> > +            goto err_out;
> > +        }
>
> We briefly talked about this and compiler can probably catch any arg
> type inconsistency between the stub func_proto and the original func_proto.
>
> I still think it is better to be strict at the
> beginning and ensure the "stub_args" type is the same as the original 
> "args"
> type. It is to bar any type inconsistency going forward on the __nullable
> tagged argument (e.g. changing the original func_proto but forgot to
> change the stub func_proto).
>
> We can revisit in the future if the following type comparison does not 
> work well.
>
>                  if (args[arg_no].type != stub_args[arg_no].type) {
>              pr_warn("arg#%u type in stub func_proto %s__%s does not 
> match with its original func_proto\n",
>                  arg_no, st_ops_name, member_name);
>              goto err_out;
>                  }


Agree!

> > +
> > +        offset = btf_ctx_arg_offset(btf, func_proto, arg_no);
> > +        if (offset < 0) {
> > +            pr_warn("stub function %s__%s has an invalid trampoline 
> > ctx offset for arg#%u\n",
> > +                st_ops_name, member_name, arg_no);
> > +            goto err_out;
> > +        }
> > +
> > +        /* Fill the information of the new argument */
> > +        info->reg_type =
> > +            PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> > +        info->btf_id = arg_btf_id;
> > +        info->btf = btf;
> > +        info->offset = offset;
> > +
> > +        info++;
> > +        info_cnt++;
> > +    }
> > +
> > +    if (info_cnt) {
> > +        arg_info->info = info_buf;
> > +        arg_info->cnt = info_cnt;
> > +    } else {
> > +        kfree(info_buf);
> > +    }
> > +
> > +    return 0;
> > +
> > +err_out:
> > +    kfree(info_buf);
> > +
> > +    return -EINVAL;
> > +}
> > +
> > +/* Clean up the arg_info in a struct bpf_struct_ops_desc. */
> > +void bpf_struct_ops_desc_release(struct bpf_struct_ops_desc 
> > *st_ops_desc)
> > +{
> > +    struct bpf_struct_ops_arg_info *arg_info;
> > +    int i;
> > +
> > +    arg_info = st_ops_desc->arg_info;
> > +    if (!arg_info)
>
> nit. I think this check is unnecessary ?
>
> If the above two comments make sense to you, I can make the adjustment. 
> No need to resend.


Agree!

> Patch 4 lgtm.
>
> > +        return;
> > +
> > +    for (i = 0; i < btf_type_vlen(st_ops_desc->type); i++)
> > +        kfree(arg_info[i].info);
> > +
> > +    kfree(arg_info);
> > +}
> > +