Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()

[Mike Rapoport <rppt@kernel.org>]

On Sat, Sep 23, 2023 at 03:36:01PM -0700, Song Liu wrote:
> On Sat, Sep 23, 2023 at 8:39 AM Mike Rapoport <rppt@kernel.org> wrote:
> >
> > On Thu, Sep 21, 2023 at 03:34:18PM -0700, Song Liu wrote:
> > > On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport <rppt@kernel.org> wrote:
> > > >
> > >
> > > [...]
> > >
> > > > diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> > > > index 42215f9404af..db5561d0c233 100644
> > > > --- a/arch/s390/kernel/module.c
> > > > +++ b/arch/s390/kernel/module.c
> > > > @@ -21,6 +21,7 @@
> > > >  #include <linux/moduleloader.h>
> > > >  #include <linux/bug.h>
> > > >  #include <linux/memory.h>
> > > > +#include <linux/execmem.h>
> > > >  #include <asm/alternative.h>
> > > >  #include <asm/nospec-branch.h>
> > > >  #include <asm/facility.h>
> > > > @@ -76,7 +77,7 @@ void *module_alloc(unsigned long size)
> > > >  #ifdef CONFIG_FUNCTION_TRACER
> > > >  void module_arch_cleanup(struct module *mod)
> > > >  {
> > > > -       module_memfree(mod->arch.trampolines_start);
> > > > +       execmem_free(mod->arch.trampolines_start);
> > > >  }
> > > >  #endif
> > > >
> > > > @@ -510,7 +511,7 @@ static int module_alloc_ftrace_hotpatch_trampolines(struct module *me,
> > > >
> > > >         size = FTRACE_HOTPATCH_TRAMPOLINES_SIZE(s->sh_size);
> > > >         numpages = DIV_ROUND_UP(size, PAGE_SIZE);
> > > > -       start = module_alloc(numpages * PAGE_SIZE);
> > > > +       start = execmem_text_alloc(EXECMEM_FTRACE, numpages * PAGE_SIZE);
> > >
> > > This should be EXECMEM_MODULE_TEXT?
> >
> > This is an ftrace trampoline, so I think it should be FTRACE type of
> > allocation.
> 
> Yeah, I was aware of the ftrace trampoline. My point was, ftrace trampoline
> doesn't seem to have any special requirements. Therefore, it is probably not
> necessary to have a separate type just for it.

Since ftrace trampolines are currently used only on s390 and x86 which
enforce the same range for all executable allocations there are no special
requirements indeed. But I think that explicitly marking these allocations
as FTRACE makes it clearer what are they used for and I don't see downsides
to having a type for FTRACE.
 
> AFAICT, kprobe, ftrace, and BPF (JIT and trampoline) can share the same
> execmem_type. We may need some work for some archs, but nothing is
> fundamentally different among these.

Using the same type for all generated code implies that all types of the
generated code must live in the same range and I don't think we want to
impose this limitation on architectures.

For example, RISC-V deliberately added a range for BPF code to allow
relative addressing, see commit 7f3631e88ee6 ("riscv, bpf: Provide RISC-V
specific JIT image alloc/free").
 
> Thanks,
> Song

-- 
Sincerely yours,
Mike.