Re: [PATCH v3 bpf-next 10/11] libbpf,bpf: share BTF relocate-related code with kernel

[Eduard Zingerman <eddyz87@gmail.com>]

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.

[...]