Re: [PATCH v5 bpf-next 1/5] module: introduce module_alloc_huge

[Song Liu <songliubraving@fb.com>]

> On Jul 1, 2022, at 4:20 PM, Luis Chamberlain <mcgrof@kernel.org> wrote:
> 
> On Fri, Jun 24, 2022 at 02:57:08PM -0700, Song Liu wrote:
>> Introduce module_alloc_huge, which allocates huge page backed memory in
>> module memory space. The primary user of this memory is bpf_prog_pack
>> (multiple BPF programs sharing a huge page).
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
> 
> I see mm not Cc'd. I'd like review from them.

I will CC mm in the next version (or resend). Thanks for the reminder. 

> 
>> ---
>> arch/x86/kernel/module.c | 21 +++++++++++++++++++++
>> include/linux/moduleloader.h | 5 +++++
>> kernel/module/main.c | 8 ++++++++
>> 3 files changed, 34 insertions(+)
>> 
>> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
>> index b98ffcf4d250..63f6a16c70dc 100644
>> --- a/arch/x86/kernel/module.c
>> +++ b/arch/x86/kernel/module.c
>> @@ -86,6 +86,27 @@ void *module_alloc(unsigned long size)
>> 	return p;
>> }
>> 
>> +void *module_alloc_huge(unsigned long size)
>> +{
>> +	gfp_t gfp_mask = GFP_KERNEL;
>> +	void *p;
>> +
>> +	if (PAGE_ALIGN(size) > MODULES_LEN)
>> +		return NULL;
>> +
>> +	p = __vmalloc_node_range(size, MODULE_ALIGN,
>> +				 MODULES_VADDR + get_module_load_offset(),
>> +				 MODULES_END, gfp_mask, PAGE_KERNEL,
>> +				 VM_DEFER_KMEMLEAK | VM_ALLOW_HUGE_VMAP,
>> +				 NUMA_NO_NODE, __builtin_return_address(0));
>> +	if (p && (kasan_alloc_module_shadow(p, size, gfp_mask) < 0)) {
>> +		vfree(p);
>> +		return NULL;
>> +	}
>> +
>> +	return p;
>> +}
> 
> 1) When things like kernel/bpf/core.c start using a module alloc it
> is time to consider genearlizing this.

I am not quite sure what the generalization would look like. IMHO, the
ideal case would have:
  a) A kernel_text_rw_allocator, similar to current module_alloc.
  b) A kernel_text_ro_allocator, similar to current bpf_prog_pack_alloc.
     This is built on top of kernel_text_rw_allocator. Different 
     allocations could share a page, thus it requires text_poke like 
     support from the arch. 
  c) If the arch supports text_poke, kprobes, ftrace trampolines, and
     bpf trampolines should use kernel_text_ro_allocator.
  d) Major archs should support CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC,
     and they should use kernel_text_ro_allocator for module text. 

Does this sound reasonable to you?

I tried to enable CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC for x86_64, 
but that doesn't really work. Do we have plan to make this combination
work?

> 
> 2) How we free is important, and each arch does something funky for
> this. This is not addressed here.

How should we address this? IIUC, x86_64 just calls vfree. 

> 
> And yes I welcome generalizing generic module_alloc() too as suggested
> before. The concern on my part is the sloppiness this enables.

One question I have is, does module_alloc (or kernel_text_*_allocator 
above) belong to module code, or mm code (maybe vmalloc)?

I am planning to let BPF trampoline use bpf_prog_pack on x86_64, which 
is another baby step of c) above. 

Thanks,
Song