Re: [PATCH v2 06/12] mm/execmem: introduce execmem_data_alloc()

[Song Liu]

On Fri, Jun 16, 2023 at 1:51 AM Mike Rapoport <rppt@xxxxxxxxxx> wrote:
>
> From: "Mike Rapoport (IBM)" <rppt@xxxxxxxxxx>
>
> Data related to code allocations, such as module data section, need to
> comply with architecture constraints for its placement and its
> allocation right now was done using execmem_text_alloc().
>
> Create a dedicated API for allocating data related to code allocations
> and allow architectures to define address ranges for data allocations.
>
> Since currently this is only relevant for powerpc variants that use the
> VMALLOC address space for module data allocations, automatically reuse
> address ranges defined for text unless address range for data is
> explicitly defined by an architecture.
>
> With separation of code and data allocations, data sections of the
> modules are now mapped as PAGE_KERNEL rather than PAGE_KERNEL_EXEC which
> was a default on many architectures.
>
> Signed-off-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
[...]
>  static void free_mod_mem(struct module *mod)
> diff --git a/mm/execmem.c b/mm/execmem.c
> index a67acd75ffef..f7bf496ad4c3 100644
> --- a/mm/execmem.c
> +++ b/mm/execmem.c
> @@ -63,6 +63,20 @@ void *execmem_text_alloc(size_t size)
>                              fallback_start, fallback_end, kasan);
>  }
>
> +void *execmem_data_alloc(size_t size)
> +{
> +       unsigned long start = execmem_params.modules.data.start;
> +       unsigned long end = execmem_params.modules.data.end;
> +       pgprot_t pgprot = execmem_params.modules.data.pgprot;
> +       unsigned int align = execmem_params.modules.data.alignment;
> +       unsigned long fallback_start = execmem_params.modules.data.fallback_start;
> +       unsigned long fallback_end = execmem_params.modules.data.fallback_end;
> +       bool kasan = execmem_params.modules.flags & EXECMEM_KASAN_SHADOW;
> +
> +       return execmem_alloc(size, start, end, align, pgprot,
> +                            fallback_start, fallback_end, kasan);
> +}
> +
>  void execmem_free(void *ptr)
>  {
>         /*
> @@ -101,6 +115,28 @@ static bool execmem_validate_params(struct execmem_params *p)
>         return true;
>  }
>
> +static void execmem_init_missing(struct execmem_params *p)

Shall we call this execmem_default_init_data?

> +{
> +       struct execmem_modules_range *m = &p->modules;
> +
> +       if (!pgprot_val(execmem_params.modules.data.pgprot))
> +               execmem_params.modules.data.pgprot = PAGE_KERNEL;

Do we really need to check each of these? IOW, can we do:

if (!pgprot_val(execmem_params.modules.data.pgprot)) {
       execmem_params.modules.data.pgprot = PAGE_KERNEL;
       execmem_params.modules.data.alignment = m->text.alignment;
       execmem_params.modules.data.start = m->text.start;
       execmem_params.modules.data.end = m->text.end;
       execmem_params.modules.data.fallback_start = m->text.fallback_start;
      execmem_params.modules.data.fallback_end = m->text.fallback_end;
}

Thanks,
Song

[...]