Re: [PATCH v2 bpf-next 02/18] libbpf: Add support for BTF kinds LOC[_PARAM|_PROTO|SEC]

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] btf_equal_loc_param() fails to compare the flags field of BTF_KIND_LOC_PARAM, leading to incorrect deduplication.
- [Medium] Public libbpf API functions (e.g., btf__add_locsec, btf__add_loc_param, btf__add_loc_proto) return negative error codes directly without using the required libbpf_err() wrapper.
- [Low] The m_offs array in struct btf_field_desc is mistakenly inflated to size 22 instead of 2, unnecessarily wasting stack memory.
--

commit ce9959576588b24d58d6a9f71651d919ddebad03
Author: Alan Maguire <alan.maguire@oracle.com>
Subject: libbpf: Add support for BTF kinds LOC[_PARAM|_PROTO|SEC]

This patch adds support for new BTF kinds to libbpf, including deduplication
logic, data accessors, and construction APIs. It also extends the BTF field
iterator to handle the new section types.

> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index c783359977b46..9449f6f50f18f 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
[ ... ]
> +int btf__add_loc_param(struct btf *btf, __u32 size, __u32 flags)
> +{
[ ... ]
> +	p = btf_loc_param(t);
> +	p->flags = flags;
> +
> +	return btf_commit_type(btf, sz);

[Severity: Medium]
Since btf__add_loc_param() is a public libbpf API, should its error return
paths be wrapped with libbpf_err() to ensure errno is set properly for
userspace callers? Here, btf_commit_type() can return a negative error code
directly.

> +}
[ ... ]
> +int btf__add_loc_proto(struct btf *btf)
> +{
[ ... ]
> +	t->info = btf_type_info(BTF_KIND_LOC_PROTO, 0, 0);
> +	t->size = 0;
> +
> +	return btf_commit_type(btf, sizeof(struct btf_type));

[Severity: Medium]
Does this unwrapped negative return code also apply to btf__add_loc_proto()?

> +}
[ ... ]
> +int btf__add_locsec(struct btf *btf, const char *name)
> +{
[ ... ]
> +	if (!str_is_empty(name)) {
> +		name_off = btf__add_str(btf, name);
> +		if (name_off < 0)
> +			return name_off;

[Severity: Medium]
Could this leak a negative error code without setting errno? The name_off
result can be negative on failure, and btf__add_locsec is a public API.

> +	}
> +	t->name_off = name_off;
> +	t->info = btf_type_info(BTF_KIND_LOCSEC, 0, 0);
> +	t->size = 0;
> +
> +	return btf_commit_type(btf, sizeof(struct btf_type));

[Severity: Medium]
Similarly, should the return value of btf_commit_type() be wrapped here?

> +}
[ ... ]
> +static bool btf_equal_loc_param(struct btf_type *t1, struct btf_type *t2)
> +{
> +	struct btf_loc_param *p1 = btf_loc_param(t1);
> +	struct btf_loc_param *p2 = btf_loc_param(t2);
> +	__u32 *v1 = (__u32 *)(p1 + 1);
> +	__u32 *v2 = (__u32 *)(p2 + 1);
> +	int i, vlen = btf_vlen(t1);
> +
> +	if (!btf_equal_common(t1, t2))
> +		return false;
> +	for (i = 0; i < vlen; i++, v1++, v2++) {
> +		if (*v1 != *v2)
> +			return false;
> +	}
> +	return true;

[Severity: High]
Does this loop need to compare the flags field of the two btf_loc_param
structures? The current logic checks the trailing parameters but appears
to skip checking if p1->flags equals p2->flags, which might cause
LOC_PARAM types with different metadata to be incorrectly deduplicated.

> +}
[ ... ]
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index cb4d962338441..ebac8db1ccfda 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -580,7 +580,7 @@ struct btf_field_desc {
>  	/* member struct size, or zero, if no members */
>  	int m_sz;
>  	/* repeated per-member offsets */
> -	int m_off_cnt, m_offs[1];
> +	int m_off_cnt, m_offs[22];

[Severity: Low]
Is m_offs sized to 22 here because BTF_KIND_LOCSEC equals 22? 
Since the iter_init function only specifies 2 offsets for LOCSEC (func and
loc_proto), could this array be sized to the maximum offset count needed 
(e.g., 2) to avoid unnecessary stack memory consumption when 
struct btf_field_desc is instantiated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901165757.801449-1-alan.maguire@oracle.com?part=2