Re: [PATCH v6 bpf-next 8/9] libbpf,bpf: share BTF relocate-related code with kernel

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-06-13 at 10:50 +0100, Alan Maguire wrote:
> Share relocation implementation with the kernel.  As part of this,
> we also need the type/string iteration functions so add them to a
> btf_iter.c file that also gets shared with the kernel. Relocation
> code in kernel and userspace is identical save for the impementation
> of the reparenting of split BTF to the relocated base BTF and
> retrieval of BTF header from "struct btf"; these small functions
> need separate user-space and kernel implementations.
> 
> One other wrinkle on the kernel side is we have to map .BTF.ids in
> modules as they were generated with the type ids used at BTF encoding
> time. btf_relocate() optionally returns an array mapping from old BTF
> ids to relocated ids, so we use that to fix up these references where
> needed for kfuncs.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

Hi Alan,

I've looked through this patch and all seems to look good,
two minor notes below.

Thanks,
Eduard

[...]

> @@ -8133,21 +8207,15 @@ static int btf_populate_kfunc_set(struct btf *btf, enum btf_kfunc_hook hook,
>  		goto end;
>  	}
>  
> -	/* We don't need to allocate, concatenate, and sort module sets, because
> -	 * only one is allowed per hook. Hence, we can directly assign the
> -	 * pointer and return.
> -	 */
> -	if (!vmlinux_set) {
> -		tab->sets[hook] = add_set;
> -		goto do_add_filter;
> -	}
> -

Is it necessary to adjust btf_free_kfunc_set_tab()? It currently skips
freeing tab->sets[*] for modules. I've added two printk's and it looks
like sets allocated for module here are leaking after insmod/rmmod.

>  	/* In case of vmlinux sets, there may be more than one set being
>  	 * registered per hook. To create a unified set, we allocate a new set
>  	 * and concatenate all individual sets being registered. While each set
>  	 * is individually sorted, they may become unsorted when concatenated,
>  	 * hence re-sorting the final set again is required to make binary
>  	 * searching the set using btf_id_set8_contains function work.
> +	 *
> +	 * For module sets, we need to allocate as we may need to relocate
> +	 * BTF ids.
>  	 */
>  	set_cnt = set ? set->cnt : 0;
>  

[...]

> @@ -8451,6 +8522,13 @@ int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dtors, u32 add_c
>  	btf->dtor_kfunc_tab = tab;
>  
>  	memcpy(tab->dtors + tab->cnt, dtors, add_cnt * sizeof(tab->dtors[0]));
> +
> +	/* remap BTF ids based on BTF relocation (if any) */
> +	for (i = tab_cnt; i < tab_cnt + add_cnt; i++) {
> +		tab->dtors[i].btf_id = btf_relocate_id(btf, tab->dtors[i].btf_id);
> +		tab->dtors[i].kfunc_btf_id = btf_relocate_id(btf, tab->dtors[i].kfunc_btf_id);

The register_btf_id_dtor_kfuncs() is exported and thus could to be
called from the modules, that's why you update it, right?
Do we want to add such call to bpf_testmod? Currently, with kernel
config used for selftests, I see only identity mappings.

> +	}
> +
>  	tab->cnt += add_cnt;
>  
>  	sort(tab->dtors, tab->cnt, sizeof(tab->dtors[0]), btf_id_cmp_func, NULL);

[...]