module: clean-up for module_memory

Message ID 20230209175653.2275559-1-song@kernel.org
State New
Headers
Series module: clean-up for module_memory |

Commit Message

Song Liu Feb. 9, 2023, 5:56 p.m. UTC
  Three changes here:

1. Shorter variable names in arch/arc/kernel/unwind.c:unwind_add_table, to
   make it easier to read.
2. Rewrite free_mod_mem() so it is more obvious that MOD_DATA need to be
   freed last.
3. Clean up the use of CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC.

Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Song Liu <song@kernel.org>

---

This is the follow up patch on top of [1]. I would recommend fold this
into [1].

[1] https://lore.kernel.org/linux-modules/20230207002802.2514802-1-song@kernel.org/T/#u
---
 arch/arc/kernel/unwind.c | 15 ++++------
 kernel/module/main.c     | 61 +++++++++++-----------------------------
 2 files changed, 22 insertions(+), 54 deletions(-)
  

Comments

Christophe Leroy Feb. 9, 2023, 7:43 p.m. UTC | #1
Le 09/02/2023 à 18:56, Song Liu a écrit :
> Three changes here:
> 
> 1. Shorter variable names in arch/arc/kernel/unwind.c:unwind_add_table, to
>     make it easier to read.
> 2. Rewrite free_mod_mem() so it is more obvious that MOD_DATA need to be
>     freed last.
> 3. Clean up the use of CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC.
> 
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Song Liu <song@kernel.org>
> 
> ---
> 
> This is the follow up patch on top of [1]. I would recommend fold this
> into [1].

With this patch folded into [1],

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> 
> [1] https://lore.kernel.org/linux-modules/20230207002802.2514802-1-song@kernel.org/T/#u
> ---
>   arch/arc/kernel/unwind.c | 15 ++++------
>   kernel/module/main.c     | 61 +++++++++++-----------------------------
>   2 files changed, 22 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/arc/kernel/unwind.c b/arch/arc/kernel/unwind.c
> index 933451f4494f..9270d0a713c3 100644
> --- a/arch/arc/kernel/unwind.c
> +++ b/arch/arc/kernel/unwind.c
> @@ -369,8 +369,8 @@ void *unwind_add_table(struct module *module, const void *table_start,
>   		       unsigned long table_size)
>   {
>   	struct unwind_table *table;
> -	struct module_memory *mod_mem_core_text;
> -	struct module_memory *mod_mem_init_text;
> +	struct module_memory *core_text;
> +	struct module_memory *init_text;
>   
>   	if (table_size <= 0)
>   		return NULL;
> @@ -379,14 +379,11 @@ void *unwind_add_table(struct module *module, const void *table_start,
>   	if (!table)
>   		return NULL;
>   
> -	mod_mem_core_text = &module->mem[MOD_TEXT];
> -	mod_mem_init_text = &module->mem[MOD_INIT_TEXT];
> +	core_text = &module->mem[MOD_TEXT];
> +	init_text = &module->mem[MOD_INIT_TEXT];
>   
> -	init_unwind_table(table, module->name,
> -			  mod_mem_core_text->base, mod_mem_core_text->size,
> -			  mod_mem_init_text->base, mod_mem_init_text->size,
> -			  table_start, table_size,
> -			  NULL, 0);
> +	init_unwind_table(table, module->name, core_text->base, core_text->size,
> +			  init_text->base, init_text->size, table_start, table_size, NULL, 0);
>   
>   	init_unwind_hdr(table, unw_hdr_alloc);
>   
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c598f11e7016..2724bc1b9a90 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -927,26 +927,17 @@ static ssize_t store_uevent(struct module_attribute *mattr,
>   struct module_attribute module_uevent =
>   	__ATTR(uevent, 0200, NULL, store_uevent);
>   
> -#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> -
> -static ssize_t show_coresize(struct module_attribute *mattr,
> -			     struct module_kobject *mk, char *buffer)
> -{
> -	return sprintf(buffer, "%u\n", mk->mod->mem[MOD_TEXT].size);
> -}
> -
> -#else
> -
>   static ssize_t show_coresize(struct module_attribute *mattr,
>   			     struct module_kobject *mk, char *buffer)
>   {
> -	unsigned int size = 0;
> +	unsigned int size = mk->mod->mem[MOD_TEXT].size;
>   
> -	for_class_mod_mem_type(type, core)
> -		size += mk->mod->mem[type].size;
> +	if (!IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC)) {
> +		for_class_mod_mem_type(type, core_data)
> +			size += mk->mod->mem[type].size;
> +	}
>   	return sprintf(buffer, "%u\n", size);
>   }
> -#endif
>   
>   static struct module_attribute modinfo_coresize =
>   	__ATTR(coresize, 0444, show_coresize, NULL);
> @@ -1170,17 +1161,11 @@ void __weak module_arch_freeing_init(struct module *mod)
>   {
>   }
>   
> -#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>   static bool mod_mem_use_vmalloc(enum mod_mem_type type)
>   {
> -	return mod_mem_type_is_core_data(type);
> -}
> -#else
> -static bool mod_mem_use_vmalloc(enum mod_mem_type type)
> -{
> -	return false;
> +	return IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) &&
> +		mod_mem_type_is_core_data(type);
>   }
> -#endif
>   
>   static void *module_memory_alloc(unsigned int size, enum mod_mem_type type)
>   {
> @@ -1199,34 +1184,21 @@ static void module_memory_free(void *ptr, enum mod_mem_type type)
>   
>   static void free_mod_mem(struct module *mod)
>   {
> -	/* free the memory in the right order to avoid use-after-free */
> -	static enum mod_mem_type mod_mem_free_order[MOD_MEM_NUM_TYPES] = {
> -		/* first free init sections */
> -		MOD_INIT_TEXT,
> -		MOD_INIT_DATA,
> -		MOD_INIT_RODATA,
> -
> -		/* then core sections, except rw data */
> -		MOD_TEXT,
> -		MOD_RODATA,
> -		MOD_RO_AFTER_INIT,
> -
> -		/* rw data need to be freed last, as it hosts mod */
> -		MOD_DATA,
> -	};
> -	int i;
> -
> -	BUILD_BUG_ON(ARRAY_SIZE(mod_mem_free_order) != MOD_MEM_NUM_TYPES);
> -
> -	for (i = 0; i < MOD_MEM_NUM_TYPES; i++) {
> -		enum mod_mem_type type = mod_mem_free_order[i];
> +	for_each_mod_mem_type(type) {
>   		struct module_memory *mod_mem = &mod->mem[type];
>   
> +		if (type == MOD_DATA)
> +			continue;
> +
>   		/* Free lock-classes; relies on the preceding sync_rcu(). */
>   		lockdep_free_key_range(mod_mem->base, mod_mem->size);
>   		if (mod_mem->size)
>   			module_memory_free(mod_mem->base, type);
>   	}
> +
> +	/* MOD_DATA hosts mod, so free it at last */
> +	lockdep_free_key_range(mod->mem[MOD_DATA].base, mod->mem[MOD_DATA].size);
> +	module_memory_free(mod->mem[MOD_DATA].base, MOD_DATA);
>   }
>   
>   /* Free a module, remove from lists, etc. */
> @@ -2211,8 +2183,7 @@ static int move_module(struct module *mod, struct load_info *info)
>   		if (!(shdr->sh_flags & SHF_ALLOC))
>   			continue;
>   
> -		dest = mod->mem[type].base +
> -			(shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK);
> +		dest = mod->mem[type].base + (shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK);
>   
>   		if (shdr->sh_type != SHT_NOBITS)
>   			memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
  
Luis Chamberlain Feb. 21, 2023, 9:15 p.m. UTC | #2
On Thu, Feb 09, 2023 at 07:43:23PM +0000, Christophe Leroy wrote:
> 
> 
> Le 09/02/2023 à 18:56, Song Liu a écrit :
> > Three changes here:
> > 
> > 1. Shorter variable names in arch/arc/kernel/unwind.c:unwind_add_table, to
> >     make it easier to read.
> > 2. Rewrite free_mod_mem() so it is more obvious that MOD_DATA need to be
> >     freed last.
> > 3. Clean up the use of CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC.
> > 
> > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> > Signed-off-by: Song Liu <song@kernel.org>
> > 
> > ---
> > 
> > This is the follow up patch on top of [1]. I would recommend fold this
> > into [1].
> 
> With this patch folded into [1],
> 
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

I've squashed this into the last patch from Song and added your
Reviewed-by tag, and pushed to modules-next. That'll sit there
for a full cycle for testing.

  Luis
  

Patch

diff --git a/arch/arc/kernel/unwind.c b/arch/arc/kernel/unwind.c
index 933451f4494f..9270d0a713c3 100644
--- a/arch/arc/kernel/unwind.c
+++ b/arch/arc/kernel/unwind.c
@@ -369,8 +369,8 @@  void *unwind_add_table(struct module *module, const void *table_start,
 		       unsigned long table_size)
 {
 	struct unwind_table *table;
-	struct module_memory *mod_mem_core_text;
-	struct module_memory *mod_mem_init_text;
+	struct module_memory *core_text;
+	struct module_memory *init_text;
 
 	if (table_size <= 0)
 		return NULL;
@@ -379,14 +379,11 @@  void *unwind_add_table(struct module *module, const void *table_start,
 	if (!table)
 		return NULL;
 
-	mod_mem_core_text = &module->mem[MOD_TEXT];
-	mod_mem_init_text = &module->mem[MOD_INIT_TEXT];
+	core_text = &module->mem[MOD_TEXT];
+	init_text = &module->mem[MOD_INIT_TEXT];
 
-	init_unwind_table(table, module->name,
-			  mod_mem_core_text->base, mod_mem_core_text->size,
-			  mod_mem_init_text->base, mod_mem_init_text->size,
-			  table_start, table_size,
-			  NULL, 0);
+	init_unwind_table(table, module->name, core_text->base, core_text->size,
+			  init_text->base, init_text->size, table_start, table_size, NULL, 0);
 
 	init_unwind_hdr(table, unw_hdr_alloc);
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c598f11e7016..2724bc1b9a90 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -927,26 +927,17 @@  static ssize_t store_uevent(struct module_attribute *mattr,
 struct module_attribute module_uevent =
 	__ATTR(uevent, 0200, NULL, store_uevent);
 
-#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
-
-static ssize_t show_coresize(struct module_attribute *mattr,
-			     struct module_kobject *mk, char *buffer)
-{
-	return sprintf(buffer, "%u\n", mk->mod->mem[MOD_TEXT].size);
-}
-
-#else
-
 static ssize_t show_coresize(struct module_attribute *mattr,
 			     struct module_kobject *mk, char *buffer)
 {
-	unsigned int size = 0;
+	unsigned int size = mk->mod->mem[MOD_TEXT].size;
 
-	for_class_mod_mem_type(type, core)
-		size += mk->mod->mem[type].size;
+	if (!IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC)) {
+		for_class_mod_mem_type(type, core_data)
+			size += mk->mod->mem[type].size;
+	}
 	return sprintf(buffer, "%u\n", size);
 }
-#endif
 
 static struct module_attribute modinfo_coresize =
 	__ATTR(coresize, 0444, show_coresize, NULL);
@@ -1170,17 +1161,11 @@  void __weak module_arch_freeing_init(struct module *mod)
 {
 }
 
-#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
 static bool mod_mem_use_vmalloc(enum mod_mem_type type)
 {
-	return mod_mem_type_is_core_data(type);
-}
-#else
-static bool mod_mem_use_vmalloc(enum mod_mem_type type)
-{
-	return false;
+	return IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) &&
+		mod_mem_type_is_core_data(type);
 }
-#endif
 
 static void *module_memory_alloc(unsigned int size, enum mod_mem_type type)
 {
@@ -1199,34 +1184,21 @@  static void module_memory_free(void *ptr, enum mod_mem_type type)
 
 static void free_mod_mem(struct module *mod)
 {
-	/* free the memory in the right order to avoid use-after-free */
-	static enum mod_mem_type mod_mem_free_order[MOD_MEM_NUM_TYPES] = {
-		/* first free init sections */
-		MOD_INIT_TEXT,
-		MOD_INIT_DATA,
-		MOD_INIT_RODATA,
-
-		/* then core sections, except rw data */
-		MOD_TEXT,
-		MOD_RODATA,
-		MOD_RO_AFTER_INIT,
-
-		/* rw data need to be freed last, as it hosts mod */
-		MOD_DATA,
-	};
-	int i;
-
-	BUILD_BUG_ON(ARRAY_SIZE(mod_mem_free_order) != MOD_MEM_NUM_TYPES);
-
-	for (i = 0; i < MOD_MEM_NUM_TYPES; i++) {
-		enum mod_mem_type type = mod_mem_free_order[i];
+	for_each_mod_mem_type(type) {
 		struct module_memory *mod_mem = &mod->mem[type];
 
+		if (type == MOD_DATA)
+			continue;
+
 		/* Free lock-classes; relies on the preceding sync_rcu(). */
 		lockdep_free_key_range(mod_mem->base, mod_mem->size);
 		if (mod_mem->size)
 			module_memory_free(mod_mem->base, type);
 	}
+
+	/* MOD_DATA hosts mod, so free it at last */
+	lockdep_free_key_range(mod->mem[MOD_DATA].base, mod->mem[MOD_DATA].size);
+	module_memory_free(mod->mem[MOD_DATA].base, MOD_DATA);
 }
 
 /* Free a module, remove from lists, etc. */
@@ -2211,8 +2183,7 @@  static int move_module(struct module *mod, struct load_info *info)
 		if (!(shdr->sh_flags & SHF_ALLOC))
 			continue;
 
-		dest = mod->mem[type].base +
-			(shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK);
+		dest = mod->mem[type].base + (shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK);
 
 		if (shdr->sh_type != SHT_NOBITS)
 			memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);