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 <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Christophe Leroy <[email protected]>
Signed-off-by: Song Liu <[email protected]>
---
This is the follow up patch on top of [1]. I would recommend fold this
into [1].
[1] https://lore.kernel.org/linux-modules/[email protected]/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);
--
2.30.2
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 <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Signed-off-by: Song Liu <[email protected]>
>
> ---
>
> 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 <[email protected]>
>
> [1] https://lore.kernel.org/linux-modules/[email protected]/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);
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 <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Cc: Christophe Leroy <[email protected]>
> > Signed-off-by: Song Liu <[email protected]>
> >
> > ---
> >
> > 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 <[email protected]>
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