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

[Alan Maguire <alan.maguire@oracle.com>]

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@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.
>

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