Hi,
On 8/3/2024 8:11 AM, Amery Hung wrote:
> From: Dave Marchevsky <davemarchevsky@xxxxxx>
>
> Currently btf_parse_fields is used in two places to create struct
> btf_record's for structs: when looking at mapval type, and when looking
> at any struct in program BTF. The former looks for kptr fields while the
> latter does not. This patch modifies the btf_parse_fields call made when
> looking at prog BTF struct types to search for kptrs as well.
>
SNIP
> On a side note, when building program BTF, the refcount of program BTF
> is now initialized before btf_parse_struct_metas() to prevent a
> refcount_inc() on zero warning. This happens when BPF_KPTR is present
> in program BTF: btf_parse_struct_metas() -> btf_parse_fields()
> -> btf_parse_kptr() -> btf_get(). This should be okay as the program BTF
> is not available yet outside btf_parse().
If btf_parse_kptr() pins the passed btf, there will be memory leak for
the btf after closing the btf fd, because the invocation of btf_put()
for kptr record in btf->struct_meta_tab depends on the invocation of
btf_free_struct_meta_tab() in btf_free(), but the invocation of
btf_free() depends the final refcnt of the btf is released, so the btf
will not be freed forever. The reason why map value doesn't have such
problem is that the invocation of btf_put() for kptr record doesn't
depends on the release of map value btf and it is accomplished by
bpf_map_free_record().
Maybe we should move the common btf used by kptr and graph_root into
btf_record and let the callers of btf_parse_fields() and
btf_record_free() to decide the life cycle of btf in btf_record.
> Acked-by: Martin KaFai Lau <martin.lau@xxxxxxxxxx>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> Signed-off-by: Amery Hung <amery.hung@xxxxxxxxxxxxx>
> ---
> kernel/bpf/btf.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 95426d5b634e..7b8275e3e500 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -5585,7 +5585,8 @@ btf_parse_struct_metas(struct bpf_verifier_log *log, struct btf *btf)
> type = &tab->types[tab->cnt];
> type->btf_id = i;
> record = btf_parse_fields(btf, t, BPF_SPIN_LOCK | BPF_LIST_HEAD | BPF_LIST_NODE |
> - BPF_RB_ROOT | BPF_RB_NODE | BPF_REFCOUNT, t->size);
> + BPF_RB_ROOT | BPF_RB_NODE | BPF_REFCOUNT |
> + BPF_KPTR, t->size);
> /* The record cannot be unset, treat it as an error if so */
> if (IS_ERR_OR_NULL(record)) {
> ret = PTR_ERR_OR_ZERO(record) ?: -EFAULT;
> @@ -5737,6 +5738,8 @@ static struct btf *btf_parse(const union bpf_attr *attr, bpfptr_t uattr, u32 uat
> if (err)
> goto errout;
>
> + refcount_set(&btf->refcnt, 1);
> +
> struct_meta_tab = btf_parse_struct_metas(&env->log, btf);
> if (IS_ERR(struct_meta_tab)) {
> err = PTR_ERR(struct_meta_tab);
> @@ -5759,7 +5762,6 @@ static struct btf *btf_parse(const union bpf_attr *attr, bpfptr_t uattr, u32 uat
> goto errout_free;
>
> btf_verifier_env_free(env);
> - refcount_set(&btf->refcnt, 1);
> return btf;
>
> errout_meta:
