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

[Christophe Leroy]

Le 31/01/2023 à 13:04, Peter Zijlstra a écrit :
> On Tue, Jan 31, 2023 at 10:58:56AM +0000, Christophe Leroy wrote:
>>
>>
>> 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.
> 
> More sharing more good. But I don't think we can lift a 'within'
> function to the global namespace, that's just asking for pain.
> 
>>> 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);
> 
> Urgh, how about?
> 
> 	for_each_mod_mem_type(type) {
> 		if (!mod_mem_type_is_init(type) && within(addr, mod, type))
> 			return true;
> 	}
> 	return false;
> 
> Then you have have a bunch of mod_mem_type_id_foo() filter functions
> that are non-contiguous without having to endlessly repeat stuff
> manually.

But that's un-readable.

You have to have the list of possible types in front of you in order to 
understand what the function does. Which means that one day or another 
someone will change the order of types in the enum, and it will break.

Better have the types explicit.

Christophe