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

[Martin KaFai Lau <martin.lau@linux.dev>]

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;
                 }

> +
> +		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.

Patch 4 lgtm.

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