Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v10] module: replace module_layout with module_memory | Date | Thu, 9 Feb 2023 00:16:46 +0000 |
| |
> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
[...]
>> { >> unsigned long min = (unsigned long)base; >> unsigned long max = min + size; >> >> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC > > A #ifdef shouldn't be required. You can use IS_ENABLED() instead: > > > >> + if (mod_mem_type_is_core_data(type)) { > > if (IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) && > mod_mem_type_is_core_data(type)) > >> + if (min < tree->data_addr_min) >> + tree->data_addr_min = min; >> + if (max > tree->data_addr_max) >> + tree->data_addr_max = max; >> + return; >> + } >> +#endif
To use IS_ENABLED() here, we also need to keep data_addr_[min|max] around. Do we really want them?
Btw, we will think about all these details again with the type aware module_alloc().
[...]
>> >> + >> +static void free_mod_mem(struct module *mod) >> +{ >> + /* free the memory in the right order to avoid use-after-free */ > > Instead of 'right order', explain what the right order is. > As far as I understand it is only to free MOD_DATA last. Everything else > doesn't matter.
I rewrote the function as:
static void free_mod_mem(struct module *mod) { for_each_mod_mem_type(type) { struct module_memory *mod_mem = &mod->mem[type];
/* free MOD_DATA at the end, as it hosts mod */ 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); }
/* free MOD_DATA at the end, as it hosts mod */ lockdep_free_key_range(mod->mem[MOD_DATA].base, mod->mem[MOD_DATA].size); module_memory_free(mod->mem[MOD_DATA].base, MOD_DATA); }
Does this look good?
Thanks, Song
[...]
| |