On 14/06/2024 23:49, Eduard Zingerman wrote:
> 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.
>
great catch! I think we need
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index da70914264fa..ef793731d40f 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1676,14 +1676,8 @@ static void btf_free_kfunc_set_tab(struct btf *btf)
if (!tab)
return;
- /* For module BTF, we directly assign the sets being registered, so
- * there is nothing to free except kfunc_set_tab.
- */
- if (btf_is_module(btf))
- goto free_tab;
for (hook = 0; hook < ARRAY_SIZE(tab->sets); hook++)
kfree(tab->sets[hook]);
-free_tab:
kfree(tab);
btf->kfunc_set_tab = NULL;
}
>> /* 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.
>
Yep, we don't currently have coverage for dtors in bpf_testmod. I'll
look at adding that. Thanks!
Alan
