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@xxxxxxxxxx>
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);
[...]
