| Message ID | 20230209175653.2275559-1-song@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | module: clean-up for module_memory | expand |
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);
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
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);
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(-)