Re: [PATCH/RFC] module: replace module_layout with module_memory

[Christophe Leroy]

Le 09/01/2023 à 21:51, Song Liu a écrit :
> On Mon, Jan 9, 2023 at 10:24 AM Song Liu <song@xxxxxxxxxx> wrote:
>>
>> On Mon, Jan 9, 2023 at 10:03 AM Christophe Leroy
>> <christophe.leroy@xxxxxxxxxx> wrote:
>>>
>>>
>>>
>>> Le 06/01/2023 à 23:09, Song Liu a écrit :
>>>> module_layout manages different types of memory (text, data, rodata, etc.)
>>>> in one allocation, which is problematic for some reasons:
>>>>
>>>> 1. It is hard to enable CONFIG_STRICT_MODULE_RWX.
>>>> 2. It is hard to use huge pages in modules (and not break strict rwx).
>>>> 3. Many archs uses module_layout for arch-specific data, but it is not
>>>>      obvious how these data are used (are they RO, RX, or RW?)
>>>>
>>>> Improve the scenario by replacing 2 (or 3) module_layout per module with
>>>> up to 7 module_memory per module:
>>>>
>>>>           MOD_MEM_TYPE_TEXT,
>>>>           MOD_MEM_TYPE_DATA,
>>>>           MOD_MEM_TYPE_RODATA,
>>>>           MOD_MEM_TYPE_RO_AFTER_INIT,
>>>>           MOD_MEM_TYPE_INIT_TEXT,
>>>>           MOD_MEM_TYPE_INIT_DATA,
>>>>           MOD_MEM_TYPE_INIT_RODATA,
>>>>
>>>> and allocating them separately.
>>>>
>>>> Various archs use module_layout for different data. These data are put
>>>> into different module_memory based on their location in module_layout.
>>>> IOW, data that used to go with text is allocated with MOD_MEM_TYPE_TEXT;
>>>> data that used to go with data is allocated with MOD_MEM_TYPE_DATA, etc.
>>>
>>> I dislike how it looks with enums, things like
>>> mod->mod_mem[MOD_MEM_TYPE_INIT_TEXT] are odd and don't read nicely.
>>> Could we have something nicer like mod->mod_mem_init_text ?
>>> I know it will complicate your for_each_mod_mem_type() but it would look
>>> nicer.
>>
>> Hmm.. I am not sure whether we want 7 module_memory here. But if we
>> agree that it looks better like that, I am ok with it.
>>
>>>
>>> Also, can you explain how you switch from two trees to only one ?
>>> As far as I remember, the same question arised when I implemented
>>> CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC, and the conclusion was that
>>> we had to keep two independant trees, so I'm a bit puzzled that you have
>>> now merged everything into a single tree.
>>
>> AFAICT, we only need __module_address() to work? So one tree is enough.
>> Did I miss something?
> 
> Do you mean one tree will cause addr_[min|max] to be inaccurate?
> 

Yes at least. On powerpc you will have module text below kernel, 
somewhere between 0xb0000000 and 0xcfffffff, and you will have module 
data in vmalloc area, somewhere between 0xf0000000 and 0xffffffff.

If you have only one tree, any address between 0xc0000000 and 0xefffffff 
will trigger a tree search.

Christophe