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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/11/24 11:49, Martin KaFai Lau wrote:
> On 2/8/24 6:37 PM, thinker.li@gmail.com 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);
>> +}
>> +
>