Re: [PATCHv7 bpf-next 19/29] libbpf: Add btf_type_is_traceable_func function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] btf_get_type_size() fails to handle union types, leading to the incorrect rejection of functions with valid union arguments.
--

commit 3f5d4e8440eaf929ad94e8774deb7fce495d30c8
Author: Jiri Olsa <jolsa@kernel.org>

libbpf: Add btf_type_is_traceable_func function

This commit adds the btf_type_is_traceable_func function to libbpf to
perform the same checks as the kernel's btf_distill_func_proto, preventing
attachment on unsupported functions. It exports this function via
libbpf_internal.h for use in benchmark tests.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ab2071fdd3e8b..c659865ec2def 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> +static int btf_get_type_size(const struct btf *btf, __u32 type_id,
> +			     const struct btf_type **ret_type)
> +{
> +	const struct btf_type *t;
> +	int i;
> +
> +	*ret_type = btf__type_by_id(btf, 0);
> +	if (!type_id)
> +		return 0;
> +	t = btf__type_by_id(btf, type_id);
> +	for (i = 0; i < MAX_RESOLVE_DEPTH && t && btf_type_is_modifier(t); i++)
> +		t = btf__type_by_id(btf, t->type);
> +	if (!t || i == MAX_RESOLVE_DEPTH)
> +		return -EINVAL;
> +	*ret_type = t;
> +	if (btf_is_ptr(t))
> +		return btf__pointer_size(btf);
> +	if (btf_is_int(t) || btf_is_any_enum(t) || btf_is_struct(t))
> +		return t->size;

[Severity: Medium]
Will this return -EINVAL for union types?

Unlike the kernel where btf_type_is_struct() covers both structs and unions,
libbpf's btf_is_struct() strictly checks for BTF_KIND_STRUCT. Because
btf_is_union(t) is missing from this condition, union types fall through and
return -EINVAL.

> +	return -EINVAL;
> +}
> +
> +bool btf_type_is_traceable_func(const struct btf *btf, const struct btf_type *t)
> +{

[ ... ]

> +	for (i = 0; i < nargs; i++) {
> +		/* No support for variable args. */
> +		if (i == nargs - 1 && args[i].type == 0)
> +			return false;
> +		ret = btf_get_type_size(btf, args[i].type, &t);
> +		/* No support of struct argument size greater than 16 bytes. */
> +		if (ret < 0 || ret > 16)

Because of the behavior above, will this regression incorrectly reject any
valid union arguments up to 16 bytes since btf_get_type_size() will return a
negative value? Should btf_get_type_size() check btf_is_union() to match the
kernel's actual tracing attachment rules?

> +			return false;
> +		/* No support for void argument. */
> +		if (ret == 0)
> +			return false;
> +	}
> +
> +	return true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603110554.29590-20-jolsa@kernel.org?part=1