Re: [PATCH bpf-next v2 2/6] resolve_btfids: Process KF_ARENA_* flags in resolve_btfids

On 8/6/26 12:12 PM, Eduard Zingerman wrote:
> On Wed, 2026-08-05 at 16:06 -0700, Ihor Solodrai wrote:
> 
> ...
> 
>> +static s32 add_arena_tagged_proto(struct btf *btf, struct kfunc *kfunc)
>> +{
>> +	const struct btf_type *func = btf__type_by_id(btf, kfunc->btf_id);
>> +	u32 proto_id = func->type;
>> +	const struct btf_type *proto = btf__type_by_id(btf, proto_id);
>> +	const struct btf_param *params = btf_params(proto);
>> +	u32 nr_params = btf_vlen(proto);
>> +	s32 arg0_type_id = nr_params > 0 ? (s32)params[0].type : -1;
>> +	s32 arg1_type_id = nr_params > 1 ? (s32)params[1].type : -1;
>> +	s32 new_proto_id, id, param_type_id;
>> +	s32 ret_type_id = proto->type;
>> +	const char *name;
>> +	int err;
>> +
>> +	if (kfunc->flags & KF_ARENA_RET) {
>> +		id = arena_tag_ptr(btf, ret_type_id);
>> +		if (id < 0) {
>> +			pr_err("ERROR: resolve_btfids: kfunc %s: KF_ARENA_RET but return type is not a pointer\n",
>> +			       kfunc->name);
>> +			return id;
>> +		}
>> +		ret_type_id = id;
>> +	}
>> +
>> +	if (kfunc->flags & KF_ARENA_ARG1) {
> 
> Nit: let's avoid the copy paste and make this code prepared for the
>      __arena suffixes by moving the logic inside the parameter
>      processing loop below:
> 
>      for (i in params) {
>        bool add_tag = false;
> 
>        param_type_id = params[i].type;
>        switch(i) { 0: add_tag = kfunc->flags & KF_ARENA_ARG1; break; ... }
>        if (add_tag)
>           param_type_id = arena_tag_ptr(btf, param_type_id);
>        if (param_type_id < 0)
>          ...
>      }

Makes sense. Will do.

> 
>> +		if (nr_params < 1) {
>> +			pr_err("ERROR: resolve_btfids: kfunc %s: KF_ARENA_ARG1 but it has no argument 1\n",
>> +			       kfunc->name);
>> +			return -EINVAL;
>> +		}
>> +		id = arena_tag_ptr(btf, arg0_type_id);
>> +		if (id < 0) {
>> +			pr_err("ERROR: resolve_btfids: kfunc %s: KF_ARENA_ARG1 but argument 1 is not a pointer\n",
>> +			       kfunc->name);
> 
> Nit: not a pointer is not the only error condition, btf__add_*()
>      functions might fail as well, maybe just push pr_err() down
>      to the arena_tag_ptr()?

I guess the question is how much details do we want from the error
messages here. Since this is a part of kernel build pipeline that can
block it, I'd err on the side of more details.

I'll see if I can simplify this though.

> 
>> +			return id;
>> +		}
>> +		arg0_type_id = id;
>> +	}
>> +
>> +	if (kfunc->flags & KF_ARENA_ARG2) {
>> +		if (nr_params < 2) {
>> +			pr_err("ERROR: resolve_btfids: kfunc %s: KF_ARENA_ARG2 but it has no argument 2\n",
>> +			       kfunc->name);
>> +			return -EINVAL;
>> +		}
>> +		id = arena_tag_ptr(btf, arg1_type_id);
>> +		if (id < 0) {
>> +			pr_err("ERROR: resolve_btfids: kfunc %s: KF_ARENA_ARG2 but argument 2 is not a pointer\n",
>> +			       kfunc->name);
>> +			return id;
>> +		}
>> +		arg1_type_id = id;
>> +	}
>> +
>> +	new_proto_id = btf__add_func_proto(btf, ret_type_id);
>> +	if (new_proto_id < 0) {
>> +		pr_err("ERROR: resolve_btfids: kfunc %s: failed to add a func proto to BTF\n",
>> +		       kfunc->name);
>> +		return new_proto_id;
>> +	}
>> +
>> +	for (u32 i = 0; i < nr_params; i++) {
>> +		proto = btf__type_by_id(btf, proto_id);
>> +		params = btf_params(proto);
> 
> Nit: these two do not need to be in the loop body.

They do, because btf__add_func_param() below may move the proto
pointer, no?

> 
>> +		name = btf__name_by_offset(btf, params[i].name_off);
>> +
>> +		switch (i) {
>> +		case 0:
>> +			param_type_id = arg0_type_id;
>> +			break;
>> +		case 1:
>> +			param_type_id = arg1_type_id;
>> +			break;
>> +		default:
>> +			param_type_id = params[i].type;
>> +			break;
>> +		}
>> +
>> +		err = btf__add_func_param(btf, name ?: "", param_type_id);
>> +		if (err < 0) {
>> +			pr_err("ERROR: resolve_btfids: kfunc %s: failed to add a proto param to BTF\n",
>> +			       kfunc->name);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	pr_debug("added arena-tagged proto for kfunc %s: %d\n", kfunc->name, new_proto_id);
>> +
>> +	return new_proto_id;
>> +}
> 
> ...