Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Sat, Sep 23, 2023 at 03:36:01PM -0700, Song Liu wrote:
> On Sat, Sep 23, 2023 at 8:39 AM Mike Rapoport 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 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
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -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.
Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Sat, Sep 23, 2023 at 8:39 AM Mike Rapoport 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 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -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.
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.
Thanks,
Song
Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Thu, Sep 21, 2023 at 03:10:26PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport wrote:
> >
> [...]
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +static void *execmem_alloc(size_t size)
> > +{
> > + return module_alloc(size);
> > +}
> > +
> > +void *execmem_text_alloc(enum execmem_type type, size_t size)
> > +{
> > + return execmem_alloc(size);
> > +}
>
> execmem_text_alloc (and later execmem_data_alloc) both take "type" as
> input. I guess we can just use execmem_alloc(type, size) for everything?
We could but I still prefer to keep this distinction.
> Thanks,
> Song
>
> > +
> > +void execmem_free(void *ptr)
> > +{
> > + /*
> > +* This memory may be RO, and freeing RO memory in an interrupt is
> > not
> > +* supported by vmalloc.
> > +*/
> > + WARN_ON(in_interrupt());
> > + vfree(ptr);
> > +}
> > --
> > 2.39.2
> >
--
Sincerely yours,
Mike.
Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Thu, Sep 21, 2023 at 03:14:54PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport wrote:
> >
> [...]
> > +
> > +/**
> > + * enum execmem_type - types of executable memory ranges
> > + *
> > + * There are several subsystems that allocate executable memory.
> > + * Architectures define different restrictions on placement,
> > + * permissions, alignment and other parameters for memory that can be used
> > + * by these subsystems.
> > + * Types in this enum identify subsystems that allocate executable memory
> > + * and let architectures define parameters for ranges suitable for
> > + * allocations by each subsystem.
> > + *
> > + * @EXECMEM_DEFAULT: default parameters that would be used for types that
> > + * are not explcitly defined.
> > + * @EXECMEM_MODULE_TEXT: parameters for module text sections
> > + * @EXECMEM_KPROBES: parameters for kprobes
> > + * @EXECMEM_FTRACE: parameters for ftrace
> > + * @EXECMEM_BPF: parameters for BPF
> > + * @EXECMEM_TYPE_MAX:
> > + */
> > +enum execmem_type {
> > + EXECMEM_DEFAULT,
>
> I found EXECMEM_DEFAULT more confusing than helpful.
I hesitated a lot about that, but in the end decided to have
EXECMEM_DEFAULT and alias EXECMEM_MODULE_TEXT to it because this is what we
essentially have now for the most architectures.
If you'll take a look at arch-specific patches, in many cases there is only
EXECMEM_DEFAULT that an architecture defines and that default is used by
all the subsystems.
> Song
>
> > + EXECMEM_MODULE_TEXT = EXECMEM_DEFAULT,
> > + EXECMEM_KPROBES,
> > + EXECMEM_FTRACE,
> > + EXECMEM_BPF,
> > + EXECMEM_TYPE_MAX,
> > +};
> > +
> [...]
--
Sincerely yours,
Mike.
Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Thu, Sep 21, 2023 at 03:34:18PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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.
> Thanks,
> Song
>
> > if (!start)
> > return -ENOMEM;
> > set_memory_rox((unsigned long)start, numpages);
> [...]
--
Sincerely yours,
Mike.
Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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?
Thanks,
Song
> if (!start)
> return -ENOMEM;
> set_memory_rox((unsigned long)start, numpages);
[...]
Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport wrote:
>
[...]
> +
> +/**
> + * enum execmem_type - types of executable memory ranges
> + *
> + * There are several subsystems that allocate executable memory.
> + * Architectures define different restrictions on placement,
> + * permissions, alignment and other parameters for memory that can be used
> + * by these subsystems.
> + * Types in this enum identify subsystems that allocate executable memory
> + * and let architectures define parameters for ranges suitable for
> + * allocations by each subsystem.
> + *
> + * @EXECMEM_DEFAULT: default parameters that would be used for types that
> + * are not explcitly defined.
> + * @EXECMEM_MODULE_TEXT: parameters for module text sections
> + * @EXECMEM_KPROBES: parameters for kprobes
> + * @EXECMEM_FTRACE: parameters for ftrace
> + * @EXECMEM_BPF: parameters for BPF
> + * @EXECMEM_TYPE_MAX:
> + */
> +enum execmem_type {
> + EXECMEM_DEFAULT,
I found EXECMEM_DEFAULT more confusing than helpful.
Song
> + EXECMEM_MODULE_TEXT = EXECMEM_DEFAULT,
> + EXECMEM_KPROBES,
> + EXECMEM_FTRACE,
> + EXECMEM_BPF,
> + EXECMEM_TYPE_MAX,
> +};
> +
[...]
Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()
On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport wrote:
>
[...]
> +
> +#include
> +#include
> +#include
> +#include
> +
> +static void *execmem_alloc(size_t size)
> +{
> + return module_alloc(size);
> +}
> +
> +void *execmem_text_alloc(enum execmem_type type, size_t size)
> +{
> + return execmem_alloc(size);
> +}
execmem_text_alloc (and later execmem_data_alloc) both take "type" as
input. I guess we can just use execmem_alloc(type, size) for everything?
Thanks,
Song
> +
> +void execmem_free(void *ptr)
> +{
> + /*
> +* This memory may be RO, and freeing RO memory in an interrupt is not
> +* supported by vmalloc.
> +*/
> + WARN_ON(in_interrupt());
> + vfree(ptr);
> +}
> --
> 2.39.2
>
