On Sun, Aug 4, 2024 at 7:41 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> 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().
>
Thanks for pointing this out. It makes sense to me.
> 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.
Could you maybe explain if and why moving btf of btf_field_kptr and
btf_field_graph_root to btf_record is necessary? I think letting
callers of btf_parse_fields() and btf_record_free() decide whether or
not to change refcount should be enough. Besides, I personally would
like to keep individual btf in btf_field_kptr and
btf_field_graph_root, so that later we can have special fields
referencing different btf.
Thanks,
Amery
> > 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:
>
