[v3,03/13] mm/execmem, arch: convert simple overrides of module_alloc to execmem

From: "Mike Rapoport (IBM)" <rppt@kernel.org>

Several architectures override module_alloc() only to define address
range for code allocations different than VMALLOC address space.

Provide a generic implementation in execmem that uses the parameters
for address space ranges, required alignment and page protections
provided by architectures.

The architectures must fill execmem_params structure and implement
execmem_arch_params() that returns a pointer to that structure. This
way the execmem initialization won't be called from every architecture,
but rather from a central place, namely initialization of the core
memory management.

The execmem provides execmem_text_alloc() API that wraps
__vmalloc_node_range() with the parameters defined by the architectures.
If an architecture does not implement execmem_arch_params(),
execmem_text_alloc() will fall back to module_alloc().

The name execmem_text_alloc() emphasizes that the allocated memory is
for executable code, the allocations of the associated data, like data
sections of a module will use execmem_data_alloc() interface that will
be added later.

Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
 arch/loongarch/kernel/module.c | 18 ++++++++--
 arch/mips/kernel/module.c      | 19 +++++++---
 arch/nios2/kernel/module.c     | 19 +++++++---
 arch/parisc/kernel/module.c    | 23 +++++++-----
 arch/riscv/kernel/module.c     | 20 ++++++++---
 arch/sparc/kernel/module.c     | 44 +++++++++++------------
 include/linux/execmem.h        | 44 +++++++++++++++++++++++
 mm/execmem.c                   | 66 ++++++++++++++++++++++++++++++++--
 mm/mm_init.c                   |  2 ++
 9 files changed, 203 insertions(+), 52 deletions(-)

On Mon, 2023-09-18 at 10:29 +0300, Mike Rapoport wrote:
> +
> +static void execmem_init_missing(struct execmem_params *p)
> +{
> +       struct execmem_range *default_range = &p-
> >ranges[EXECMEM_DEFAULT];
> +
> +       for (int i = EXECMEM_DEFAULT + 1; i < EXECMEM_TYPE_MAX; i++)
> {
> +               struct execmem_range *r = &p->ranges[i];
> +
> +               if (!r->start) {
> +                       r->pgprot = default_range->pgprot;
> +                       r->alignment = default_range->alignment;
> +                       r->start = default_range->start;
> +                       r->end = default_range->end;
> +               }
> +       }
> +}
> +

It seems a bit weird to copy all of this. Is it trying to be faster or
something?

Couldn't it just check r->start in execmem_text/data_alloc() path and
switch to EXECMEM_DEFAULT if needed then? The execmem_range_is_data()
part that comes later could be added to the logic there too. So this
seems like unnecessary complexity to me or I don't see the reason.

On Tue, 2023-10-03 at 17:29 -0700, Rick Edgecombe wrote:
> It seems a bit weird to copy all of this. Is it trying to be faster
> or
> something?
> 
> Couldn't it just check r->start in execmem_text/data_alloc() path and
> switch to EXECMEM_DEFAULT if needed then? The execmem_range_is_data()
> part that comes later could be added to the logic there too. So this
> seems like unnecessary complexity to me or I don't see the reason.

I guess this is a bad idea because if you have the full size array
sitting around anyway you might as well use it and reduce the
exec_mem_alloc() logic. Just looking at it from the x86 side (and
similar) though, where there is actually only one execmem_range and it
building this whole array with identical data and it seems weird.

On Wed, Oct 04, 2023 at 03:39:26PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2023-10-03 at 17:29 -0700, Rick Edgecombe wrote:
> > It seems a bit weird to copy all of this. Is it trying to be faster
> > or
> > something?
> > 
> > Couldn't it just check r->start in execmem_text/data_alloc() path and
> > switch to EXECMEM_DEFAULT if needed then? The execmem_range_is_data()
> > part that comes later could be added to the logic there too. So this
> > seems like unnecessary complexity to me or I don't see the reason.
> 
> I guess this is a bad idea because if you have the full size array
> sitting around anyway you might as well use it and reduce the
> exec_mem_alloc() logic.

That's was the idea, indeed. :)

> Just looking at it from the x86 side (and
> similar) though, where there is actually only one execmem_range and it
> building this whole array with identical data and it seems weird.

Right, most architectures have only one range, but to support all variants
that we have, execmem has to maintain the whole array.

On Thu, 2023-10-05 at 08:26 +0300, Mike Rapoport wrote:
> On Wed, Oct 04, 2023 at 03:39:26PM +0000, Edgecombe, Rick P wrote:
> > On Tue, 2023-10-03 at 17:29 -0700, Rick Edgecombe wrote:
> > > It seems a bit weird to copy all of this. Is it trying to be
> > > faster
> > > or
> > > something?
> > > 
> > > Couldn't it just check r->start in execmem_text/data_alloc() path
> > > and
> > > switch to EXECMEM_DEFAULT if needed then? The
> > > execmem_range_is_data()
> > > part that comes later could be added to the logic there too. So
> > > this
> > > seems like unnecessary complexity to me or I don't see the
> > > reason.
> > 
> > I guess this is a bad idea because if you have the full size array
> > sitting around anyway you might as well use it and reduce the
> > exec_mem_alloc() logic.
> 
> That's was the idea, indeed. :)
> 
> > Just looking at it from the x86 side (and
> > similar) though, where there is actually only one execmem_range and
> > it
> > building this whole array with identical data and it seems weird.
> 
> Right, most architectures have only one range, but to support all
> variants
> that we have, execmem has to maintain the whole array.

What about just having an index into a smaller set of ranges. The
module area and the extra JIT area. So ->ranges can be size 3
(statically allocated in the arch code) for three areas and then the
index array can be size EXECMEM_TYPE_MAX. The default 0 value of the
indexing array will point to the default area and any special areas can
be set in the index point to the desired range.

Looking at how it would do for x86 and arm64, it looks maybe a bit
better to me. A little bit less code and memory usage, and a bit easier
to trace the configuration through to the final state (IMO). What do
you think? Very rough, on top of this series, below.

As I was playing around with this, I was also wondering why it needs
two copies of struct execmem_params: one returned from the arch code
and one in exec mem. And why the temporary arch copy is ro_after_init,
but the final execmem.c copy is not ro_after_init?

 arch/arm64/mm/init.c    | 67 ++++++++++++++++++++++++++++++++++++++---
--------------------------
 arch/x86/mm/init.c      | 24 +++++++++++++-----------
 include/linux/execmem.h |  5 +++--
 mm/execmem.c            | 61 ++++++++++++++++-------------------------
--------------------
 4 files changed, 70 insertions(+), 87 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 9b7716b4d84c..7df119101f20 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -633,49 +633,58 @@ static int __init module_init_limits(void)
 	return 0;
 }
 
-static struct execmem_params execmem_params __ro_after_init = {
-	.ranges = {
-		[EXECMEM_DEFAULT] = {
-			.flags = EXECMEM_KASAN_SHADOW,
-			.alignment = MODULE_ALIGN,
-		},
-		[EXECMEM_KPROBES] = {
-			.start = VMALLOC_START,
-			.end = VMALLOC_END,
-			.alignment = 1,
-		},
-		[EXECMEM_BPF] = {
-			.start = VMALLOC_START,
-			.end = VMALLOC_END,
-			.alignment = 1,
-		},
+static struct execmem_range[2] ranges __ro_after_init = {
+	/* Module area */
+	[0] = {
+		.flags = EXECMEM_KASAN_SHADOW,
+		.alignment = MODULE_ALIGN,
+	},
+	/* Kprobes area */
+	[1] = {
+		.start = VMALLOC_START,
+		.end = VMALLOC_END,
+		.alignment = 1,
+	},
+	/* BPF area */
+	[2] = {
+		.start = VMALLOC_START,
+		.end = VMALLOC_END,
+		.alignment = 1,
 	},
 };
 
-struct execmem_params __init *execmem_arch_params(void)
+void __init execmem_arch_params(struct execmem_params *p)
 {
-	struct execmem_range *r =
&execmem_params.ranges[EXECMEM_DEFAULT];
+	struct execmem_range *default;
+	struct execmem_range *jit;
+
+	p->ranges = &ranges;
 
 	module_init_limits();
 
-	r->pgprot = PAGE_KERNEL;
-
+	/* Default area */
+	default = &ranges[0];
+	default->pgprot = PAGE_KERNEL;
 	if (module_direct_base) {
-		r->start = module_direct_base;
-		r->end = module_direct_base + SZ_128M;
+		default->start = module_direct_base;
+		default->end = module_direct_base + SZ_128M;
 
 		if (module_plt_base) {
-			r->fallback_start = module_plt_base;
-			r->fallback_end = module_plt_base + SZ_2G;
+			default->fallback_start = module_plt_base;
+			default->fallback_end = module_plt_base +
SZ_2G;
 		}
 	} else if (module_plt_base) {
-		r->start = module_plt_base;
-		r->end = module_plt_base + SZ_2G;
+		default->start = module_plt_base;
+		default->end = module_plt_base + SZ_2G;
 	}
 
-	execmem_params.ranges[EXECMEM_KPROBES].pgprot =
PAGE_KERNEL_ROX;
-	execmem_params.ranges[EXECMEM_BPF].pgprot = PAGE_KERNEL;
+	/* Jit area */
+	ranges[1].pgprot = PAGE_KERNEL_ROX;
+	p->defaults[EXECMEM_KPROBES] = 1;
+	
 
-	return &execmem_params;
+	/* BPF Area */
+	ranges[2].pgprot = PAGE_KERNEL;
+	p->defaults[EXECMEM_BPF] = 2;
 }
 #endif
diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 022af7ab50f9..7397472ffc39 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -1102,16 +1102,15 @@ unsigned long arch_max_swapfile_size(void)
 #endif
 
 #ifdef CONFIG_EXECMEM
-static struct execmem_params execmem_params __ro_after_init = {
-	.ranges = {
-		[EXECMEM_DEFAULT] = {
-			.flags = EXECMEM_KASAN_SHADOW,
-			.alignment = MODULE_ALIGN,
-		},
+static struct execmem_range ranges[1] __ro_after_init = {
+	/* Module area */
+	[0] = {
+		.flags = EXECMEM_KASAN_SHADOW,
+		.alignment = MODULE_ALIGN,
 	},
 };
 
-struct execmem_params __init *execmem_arch_params(void)
+void __init execmem_arch_params(struct execmem_params *p)
 {
 	unsigned long module_load_offset = 0;
 	unsigned long start;
@@ -1121,10 +1120,13 @@ struct execmem_params __init
*execmem_arch_params(void)
 			get_random_u32_inclusive(1, 1024) * PAGE_SIZE;
 
 	start = MODULES_VADDR + module_load_offset;
-	execmem_params.ranges[EXECMEM_DEFAULT].start = start;
-	execmem_params.ranges[EXECMEM_DEFAULT].end = MODULES_END;
-	execmem_params.ranges[EXECMEM_DEFAULT].pgprot = PAGE_KERNEL;
+	p->ranges = ranges;
 
-	return &execmem_params;
+	/* Module area */
+	p->ranges[0].start = start;
+	p->ranges[0].end = MODULES_END;
+	p->ranges[0].pgprot = PAGE_KERNEL;
+	p->ranges[0].flags = EXECMEM_KASAN_SHADOW;
+	p->ranges[0].alignment = MODULE_ALIGN;
 }
 #endif /* CONFIG_EXECMEM */
diff --git a/include/linux/execmem.h b/include/linux/execmem.h
index 09d45ac786e9..702435443d87 100644
--- a/include/linux/execmem.h
+++ b/include/linux/execmem.h
@@ -77,7 +77,8 @@ struct execmem_range {
  * each type of executable memory allocations
  */
 struct execmem_params {
-	struct execmem_range	ranges[EXECMEM_TYPE_MAX];
+	int areas[EXECMEM_TYPE_MAX];
+	struct execmem_range	*ranges;
 };
 
 /**
@@ -92,7 +93,7 @@ struct execmem_params {
  * Return: a structure defining architecture parameters and
restrictions
  * for allocations of executable memory
  */
-struct execmem_params *execmem_arch_params(void);
+void execmem_arch_params(struct execmem_params *p);
 
 /**
  * execmem_text_alloc - allocate executable memory
diff --git a/mm/execmem.c b/mm/execmem.c
index aeff85261360..dfdec8c2b074 100644
--- a/mm/execmem.c
+++ b/mm/execmem.c
@@ -6,15 +6,15 @@
 #include <linux/moduleloader.h>
 
 static struct execmem_params execmem_params;
+static struct execmem_range default_range;
 
-static void *execmem_alloc(size_t size, struct execmem_range *range)
+static void *execmem_alloc(size_t size, struct execmem_range *range,
pgprot_t pgprot)
 {
 	unsigned long start = range->start;
 	unsigned long end = range->end;
 	unsigned long fallback_start = range->fallback_start;
 	unsigned long fallback_end = range->fallback_end;
 	unsigned int align = range->alignment;
-	pgprot_t pgprot = range->pgprot;
 	bool kasan = range->flags & EXECMEM_KASAN_SHADOW;
 	unsigned long vm_flags  = VM_FLUSH_RESET_PERMS;
 	bool fallback  = !!fallback_start;
@@ -60,14 +60,18 @@ static inline bool execmem_range_is_data(enum
execmem_type type)
 
 void *execmem_text_alloc(enum execmem_type type, size_t size)
 {
-	return execmem_alloc(size, &execmem_params.ranges[type]);
+	struct execmem_range *range =
&execmem_params.ranges[execmem_params.areas[type]];
+
+	return execmem_alloc(size, range, range->pgprot);
 }
 
 void *execmem_data_alloc(enum execmem_type type, size_t size)
 {
+	struct execmem_range *range =
&execmem_params.ranges[execmem_params.areas[type]];
+
 	WARN_ON_ONCE(!execmem_range_is_data(type));
 
-	return execmem_alloc(size, &execmem_params.ranges[type]);
+	return execmem_alloc(size, range, PAGE_KERNEL);
 }
 
 void execmem_free(void *ptr)
@@ -80,9 +84,13 @@ void execmem_free(void *ptr)
 	vfree(ptr);
 }
 
-struct execmem_params * __weak execmem_arch_params(void)
+void __weak execmem_arch_params(struct execmem_params *p)
 {
-	return NULL;
+	p->ranges = default_range;
+	p->ranges[EXECMEM_DEFAULT].start = VMALLOC_START;
+	p->ranges[EXECMEM_DEFAULT].end = VMALLOC_END;
+	p->ranges[EXECMEM_DEFAULT].pgprot = PAGE_KERNEL_EXEC;
+	p->ranges[EXECMEM_DEFAULT].alignment = 1;
 }
 
 static bool execmem_validate_params(struct execmem_params *p)
@@ -97,46 +105,9 @@ static bool execmem_validate_params(struct
execmem_params *p)
 	return true;
 }
 
-static void execmem_init_missing(struct execmem_params *p)
-{
-	struct execmem_range *default_range = &p-
>ranges[EXECMEM_DEFAULT];
-
-	for (int i = EXECMEM_DEFAULT + 1; i < EXECMEM_TYPE_MAX; i++) {
-		struct execmem_range *r = &p->ranges[i];
-
-		if (!r->start) {
-			if (execmem_range_is_data(i))
-				r->pgprot = PAGE_KERNEL;
-			else
-				r->pgprot = default_range->pgprot;
-			r->alignment = default_range->alignment;
-			r->start = default_range->start;
-			r->end = default_range->end;
-			r->flags = default_range->flags;
-			r->fallback_start = default_range-
>fallback_start;
-			r->fallback_end = default_range->fallback_end;
-		}
-	}
-}
-
 void __init execmem_init(void)
 {
-	struct execmem_params *p = execmem_arch_params();
+	execmem_arch_params(&execmem_params);
 
-	if (!p) {
-		p = &execmem_params;
-		p->ranges[EXECMEM_DEFAULT].start = VMALLOC_START;
-		p->ranges[EXECMEM_DEFAULT].end = VMALLOC_END;
-		p->ranges[EXECMEM_DEFAULT].pgprot = PAGE_KERNEL_EXEC;
-		p->ranges[EXECMEM_DEFAULT].alignment = 1;
-
-		return;
-	}
-
-	if (!execmem_validate_params(p))
-		return;
-
-	execmem_init_missing(p);
-
-	execmem_params = *p;
+	execmem_validate_params(&execmem_params);
 }

On Mon, 2023-09-18 at 10:29 +0300, Mike Rapoport wrote:
> +/**
> + * struct execmem_range - definition of a memory range suitable for
> code and
> + *                       related data allocations
> + * @start:     address space start
> + * @end:       address space end (inclusive)
> + * @pgprot:    permissions for memory in this address space
> + * @alignment: alignment required for text allocations
> + */
> +struct execmem_range {
> +       unsigned long   start;
> +       unsigned long   end;
> +       pgprot_t        pgprot;
> +       unsigned int    alignment;
> +};

Not a strong opinion, but range doesn't seem an appropriate name. It
*has* a range, but also other allocation configuration. It gets
especially confusing when multiple "ranges" have the same range. Maybe
execmem_alloc_params?

Hi Rick,

Sorry for the delay, I was a bit preoccupied with $stuff.

On Thu, Oct 05, 2023 at 06:09:07PM +0000, Edgecombe, Rick P wrote:
> On Thu, 2023-10-05 at 08:26 +0300, Mike Rapoport wrote:
> > On Wed, Oct 04, 2023 at 03:39:26PM +0000, Edgecombe, Rick P wrote:
> > > On Tue, 2023-10-03 at 17:29 -0700, Rick Edgecombe wrote:
> > > > It seems a bit weird to copy all of this. Is it trying to be
> > > > faster
> > > > or
> > > > something?
> > > > 
> > > > Couldn't it just check r->start in execmem_text/data_alloc() path
> > > > and
> > > > switch to EXECMEM_DEFAULT if needed then? The
> > > > execmem_range_is_data()
> > > > part that comes later could be added to the logic there too. So
> > > > this
> > > > seems like unnecessary complexity to me or I don't see the
> > > > reason.
> > > 
> > > I guess this is a bad idea because if you have the full size array
> > > sitting around anyway you might as well use it and reduce the
> > > exec_mem_alloc() logic.
> > 
> > That's was the idea, indeed. :)
> > 
> > > Just looking at it from the x86 side (and
> > > similar) though, where there is actually only one execmem_range and
> > > it
> > > building this whole array with identical data and it seems weird.
> > 
> > Right, most architectures have only one range, but to support all
> > variants
> > that we have, execmem has to maintain the whole array.
> 
> What about just having an index into a smaller set of ranges. The
> module area and the extra JIT area. So ->ranges can be size 3
> (statically allocated in the arch code) for three areas and then the
> index array can be size EXECMEM_TYPE_MAX. The default 0 value of the
> indexing array will point to the default area and any special areas can
> be set in the index point to the desired range.
> 
> Looking at how it would do for x86 and arm64, it looks maybe a bit
> better to me. A little bit less code and memory usage, and a bit easier
> to trace the configuration through to the final state (IMO). What do
> you think? Very rough, on top of this series, below.

I like your suggestion to only have definitions of actual ranges in arch
code and index array to redirect allocation requests to the right range.
I'll make the next version along the lines of your patch.

> As I was playing around with this, I was also wondering why it needs
> two copies of struct execmem_params: one returned from the arch code
> and one in exec mem. 

No actual reason, one copy is enough, thanks for catching this.

> And why the temporary arch copy is ro_after_init,
> but the final execmem.c copy is not ro_after_init?

I just missed it, thanks for pointing out.