On Tue, 2024-05-14 at 17:14 +0100, Alan Maguire wrote:
> On 11/05/2024 02:46, Eduard Zingerman wrote:
> > On Fri, 2024-05-10 at 11:30 +0100, Alan Maguire wrote:
> >
> > [...]
> >
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 821063660d9f..82bd2a275a12 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -274,6 +274,7 @@ struct btf {
> > > u32 start_str_off; /* first string offset (0 for base BTF) */
> > > char name[MODULE_NAME_LEN];
> > > bool kernel_btf;
> > > + __u32 *base_map; /* map from distilled base BTF -> vmlinux BTF ids */
> > > };
> > >
> > > enum verifier_phase {
> > > @@ -1735,7 +1736,13 @@ static void btf_free(struct btf *btf)
> > > kvfree(btf->types);
> > > kvfree(btf->resolved_sizes);
> > > kvfree(btf->resolved_ids);
> > > - kvfree(btf->data);
> > > + /* only split BTF allocates data, but btf->data is non-NULL for
> > > + * vmlinux BTF too.
> > > + */
> > > + if (btf->base_btf)
> > > + kvfree(btf->data);
> >
> > Is this correct?
> > I see that btf->data is assigned in three functions:
> > - btf_parse(): allocated via kvmalloc(), does not set btf->base_btf;
> > - btf_parse_base(): not allocated passed from caller, either vmlinux
> > or module, does not set btf->base_btf;
> > - btf_parse_module(): allocated via kvmalloc(), does set btf->base_btf;
> >
> > So, the check above seems incorrect for btf_parse(), am I wrong?
> >
>
> You're right, we need to check btf->kernel_btf too to ensure we're
> dealing with vmlinux where the btf->data was assigned to __start_BTF.
Maybe add a flag saying if .data needs freeing?
Tbh, following the callgraph to check when conditions are true or
false is a bit time consuming for someone reading the code.
[...]
> > > +static int btf_rewrite_strs(__u32 *str_off, void *ctx)
> > > +{
> > > + struct btf_rewrite_strs *r = ctx;
> > > + const char *s;
> > > + int off;
> > > +
> > > + if (!*str_off)
> > > + return 0;
> > > + if (*str_off >= r->str_start) {
> > > + *str_off += r->str_diff;
> > > + } else {
> > > + s = btf_str_by_offset(r->old_base_btf, *str_off);
> > > + if (!s)
> > > + return -ENOENT;
> > > + if (r->str_map[*str_off]) {
> > > + off = r->str_map[*str_off];
> > > + } else {
> > > + off = btf_find_str(r->btf->base_btf, s);
> > > + if (off < 0)
> > > + return off;
> > > + r->str_map[*str_off] = off;
> > > + }
> >
> > If 'str_map' part would be abstracted as local function 'btf__add_str'
> > it should be possible to move btf_rewrite_strs() and btf_set_base_btf()
> > to btf_common.c, right?
> >
>
> We can minimize the non-common code alright, but because struct btf is
> locally declared in btf.c we need a few helper functions. I'd propose we
> add (to both btf.c files):
>
> struct btf_header *btf_header(const struct btf *btf)
> {
> return btf->hdr;
> }
>
> void btf_set_base_btf(struct btf *btf, struct btf *base_btf)
> {
> btf->base_btf = base_btf;
> btf->start_id = btf__type_cnt(base_btf);
> btf->start_str_off = base_btf->hdr->str_len;
> }
>
> ...and use common code for the rest. As you say, we'll also need a
> btf__add_str() for the kernel side. What do you think?
Sounds good, thank you.
[...]
