> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@xxxxxxxxxx> 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
[...]
