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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 31/01/2023 à 10:09, Peter Zijlstra a écrit :

>> @@ -573,23 +574,33 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
>>   bool is_module_percpu_address(unsigned long addr);
>>   bool is_module_text_address(unsigned long addr);
>>   
>> +static inline bool within_module_mem_type(unsigned long addr,
>> +					  const struct module *mod,
>> +					  enum mod_mem_type type)
>> +{
>> +	unsigned long base, size;
>> +
>> +	base = (unsigned long)mod->mem[type].base;
>> +	size = mod->mem[type].size;
>> +
>> +	return base <= addr && addr < base + size;
> 
> Possible (as inspired by all the above is_{init,core}() etc..
> 
> 	return addr - base < size;
> 

In kernel/module/main.c we have a function called within(). Maybe that 
function could be lifted in module.h and used.

> 
> static inline bool within_module_mem_types(unsigned long addr,
> 					   const struct module *mod,
> 					   enum mod_mem_type first,
> 					   enum mod_mem_type last)
> {
> 	for (enum mod_mem_type type = first; type <= last; type++) {
> 		if (within_module_mem_type(addr, mod, type))
> 			return true;
> 	}
> 	return false;
> }

Well, ok but what garanties it will always be contiguous types ?
And you can't anymore see at first look what types it is.

I prefer it to be explicit with within_module_mem_type(TYPE1) || 
within_module_mem_type(TYPE2) || within_module_mem_type(TYPE3). By the 
way we could make the function name shorter, even within() may be a 
better name as it is used only inside module code.

Something like

	return within(addr, mod, MOD_TEXT) || within(addr, mod, MOD_DATA) ||
	       within(addr, mod, MOD_RODATA) || within(addr, mod, 
MOD_RO_AFTER_INIT);


> 
>> +
>>   static inline bool within_module_core(unsigned long addr,
>>   				      const struct module *mod)
>>   {
>> -#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>> -	if ((unsigned long)mod->data_layout.base <= addr &&
>> -	    addr < (unsigned long)mod->data_layout.base + mod->data_layout.size)
>> -		return true;
>> -#endif
>> -	return (unsigned long)mod->core_layout.base <= addr &&
>> -	       addr < (unsigned long)mod->core_layout.base + mod->core_layout.size;
>> +	return within_module_mem_type(addr, mod, MOD_TEXT) ||
>> +		within_module_mem_type(addr, mod, MOD_DATA) ||
>> +		within_module_mem_type(addr, mod, MOD_RODATA) ||
>> +		within_module_mem_type(addr, mod, MOD_RO_AFTER_INIT);
>>   }
> 
> within_module_mem_types(addr, mod, MOD_TEXT, MOD_RO_AFTER_INIT);
> 
>>   static inline bool within_module_init(unsigned long addr,
>>   				      const struct module *mod)
>>   {
>> -	return (unsigned long)mod->init_layout.base <= addr &&
>> -	       addr < (unsigned long)mod->init_layout.base + mod->init_layout.size;
>> +	return within_module_mem_type(addr, mod, MOD_INIT_TEXT) ||
>> +		within_module_mem_type(addr, mod, MOD_INIT_DATA) ||
>> +		within_module_mem_type(addr, mod, MOD_INIT_RODATA);
>>   }
> 
> within_module_mem_types(addr, mod, MOD_INIT_TEXT, MOD_INIT_RODATA);

Christophe