linux-kernel - Re: [PATCH v10] module: replace module_layout with module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4CAD2C20-FD08-48A6-ACB7-C2FD42F66107@fb.com>
Date:   Thu, 9 Feb 2023 17:28:05 +0000
From:   Song Liu <songliubraving@...a.com>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
CC:     Song Liu <songliubraving@...a.com>, Song Liu <song@...nel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hch@....de" <hch@....de>, Kernel Team <kernel-team@...a.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v10] module: replace module_layout with module_memory



> On Feb 8, 2023, at 10:37 PM, Christophe Leroy <christophe.leroy@...roup.eu> wrote:
> 
> 
> 
> Le 09/02/2023 à 01:16, Song Liu a écrit :
>> 
>> 
>>> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@...roup.eu> 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
> };

I think using union here will be an overkill. Let's just keep it 
simple with #ifdef. 

Thanks,
Song