Re: [PATCH v10] module: replace module_layout with module_memory

[Christophe Leroy]

Le 09/02/2023 à 01:16, Song Liu a écrit :
> 
> 
>> 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?

It is up to you. If you think it is not worth the effort, it's fine for me.

Allthough it could probably be easily fixed by doing (untested) :

struct mod_tree_root {
#ifdef CONFIG_MODULES_TREE_LOOKUP
	struct latch_tree_root root;
#endif
#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
	unsigned long addr_min;
	unsigned long addr_max;
	unsigned long data_addr_min;
	unsigned long data_addr_max;
#else
	union {
		unsigned long addr_min;
		unsigned long data_addr_min;
	};
	union {
		unsigned long addr_max;
		unsigned long data_addr_max;
	}:
#endif
};


> 
> 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
> 
> [...]