On Mon, Aug 5, 2024 at 12:32 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 8/5/2024 12:31 PM, Amery Hung wrote:
> > 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.
>
> Sorry, I didn't express the rough idea clearly enough. I didn't mean to
> move btf of btf_field_kptr and btf_field_graph_root to btf_record,
> because there are other btf-s which are different with the btf which
> creates the struct_meta_tab. What I was trying to suggest is to save one
> btf in btf_record and hope it will simplify the pin and the unpin of btf
> in btf_record:
>
> 1) save the btf which owns the btf_record in btf_record.
> 2) during btf_parse_kptr() or similar, if the used btf is the same as
> the btf in btf_record, there is no need to pin the btf
> 3) when freeing the btf_record, if the btf saved in btf_field is the
> same as the btf in btf_record, there is no need to put it
>
> For step 2) and step 3), however I think it is also doable through other
> ways (e.g., pass the btf to btf_record_free or similar).
Thanks for clarifying. I see what you mean in 1), and saving the
owner's btf in btf_record seems to be cleaner than adding additional
arguments to btf_record_free() and other related functions.
Thanks,
Amery
> >
> > 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:
> > .
>
