Re: [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time

From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Marek Szyprowski <m.szyprowski@samsung.com>,
	Chen-Yu Tsai <wenst@chromium.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>,
	Nicholas Piggin <npiggin@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time
Date: Wed, 31 Jan 2024 20:07:40 +0000	[thread overview]
Message-ID: <27eb37bb-f4eb-4cfb-ba3f-2317856f7b4b@csgroup.eu> (raw)
In-Reply-To: <2febff4d-26ba-4809-a124-7add25ab3d35@samsung.com>

Le 31/01/2024 à 16:17, Marek Szyprowski a écrit :
> [Vous ne recevez pas souvent de courriers de m.szyprowski@samsung.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hi Christophe,
> 
> On 31.01.2024 12:58, Christophe Leroy wrote:
>> Le 30/01/2024 à 18:48, Marek Szyprowski a écrit :
>>> [Vous ne recevez pas souvent de courriers de m.szyprowski@samsung.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> On 30.01.2024 12:03, Christophe Leroy wrote:
>>>> Le 30/01/2024 à 10:16, Chen-Yu Tsai a écrit :
>>>>> [Vous ne recevez pas souvent de courriers de wenst@chromium.org. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>> On Mon, Jan 29, 2024 at 12:09:50PM -0800, Luis Chamberlain wrote:
>>>>>> On Thu, Dec 21, 2023 at 10:02:46AM +0100, Christophe Leroy wrote:
>>>>>>> Declaring rodata_enabled and mark_rodata_ro() at all time
>>>>>>> helps removing related #ifdefery in C files.
>>>>>>>
>>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>> Very nice cleanup, thanks!, applied and pushed
>>>>>>
>>>>>>        Luis
>>>>> On next-20240130, which has your modules-next branch, and thus this
>>>>> series and the other "module: Use set_memory_rox()" series applied,
>>>>> my kernel crashes in some very weird way. Reverting your branch
>>>>> makes the crash go away.
>>>>>
>>>>> I thought I'd report it right away. Maybe you folks would know what's
>>>>> happening here? This is on arm64.
>>>> That's strange, it seems to bug in module_bug_finalize() which is
>>>> _before_ calls to module_enable_ro() and such.
>>>>
>>>> Can you try to revert the 6 patches one by one to see which one
>>>> introduces the problem ?
>>>>
>>>> In reality, only patch 677bfb9db8a3 really change things. Other ones are
>>>> more on less only cleanup.
>>> I've also run into this issue with today's (20240130) linux-next on my
>>> test farm. The issue is not fully reproducible, so it was a bit hard to
>>> bisect it automatically. I've spent some time on manual testing and it
>>> looks that reverting the following 2 commits on top of linux-next fixes
>>> the problem:
>>>
>>> 65929884f868 ("modules: Remove #ifdef CONFIG_STRICT_MODULE_RWX around
>>> rodata_enabled")
>>> 677bfb9db8a3 ("module: Don't ignore errors from set_memory_XX()")
>>>
>>> This in fact means that commit 677bfb9db8a3 is responsible for this
>>> regression, as 65929884f868 has to be reverted only because the latter
>>> depends on it. Let me know what I can do to help debugging this issue.
>>>
>> Thanks for the bisect. I suspect you hit one of the errors and something
>> goes wrong in the error path.
>>
>> To confirm this assumption, could you try with the following change on
>> top of everything ?
> 
> 
> Yes, this is the problem. I've added printing a mod->name to the log.
> Here is a log from kernel build from next-20240130 (sometimes it even
> boots to shell):
> 
> # dmesg | grep module_set_memory
> [    8.061525] module_set_memory(6, 0000000000000000, 0) name ipv6
> returned -22
> [    8.067543] WARNING: CPU: 3 PID: 1 at kernel/module/strict_rwx.c:22
> module_set_memory+0x9c/0xb8

Would be good if you could show the backtrace too so that we know who is 
the caller. I guess what you show here is what you get on the screen ? 
The backtrace should be available throught 'dmesg'.

I guess we will now seek help from ARM64 people to understand why 
module_set_memory_something() fails with -EINVAL when loading modules.

> [    8.097821] pc : module_set_memory+0x9c/0xb8
> [    8.102068] lr : module_set_memory+0x9c/0xb8
> [    8.183101]  module_set_memory+0x9c/0xb8
> [    8.472862] module_set_memory(6, 0000000000000000, 0) name x_tables
> returned -22
> [    8.479215] WARNING: CPU: 2 PID: 1 at kernel/module/strict_rwx.c:22
> module_set_memory+0x9c/0xb8
> [    8.510978] pc : module_set_memory+0x9c/0xb8
> [    8.515225] lr : module_set_memory+0x9c/0xb8
> [    8.596259]  module_set_memory+0x9c/0xb8
> [   10.529879] module_set_memory(6, 0000000000000000, 0) name dm_mod
> returned -22
> [   10.536087] WARNING: CPU: 3 PID: 127 at kernel/module/strict_rwx.c:22
> module_set_memory+0x9c/0xb8
> [   10.568254] pc : module_set_memory+0x9c/0xb8
> [   10.572501] lr : module_set_memory+0x9c/0xb8
> [   10.653535]  module_set_memory+0x9c/0xb8
> [   10.853177] module_set_memory(6, 0000000000000000, 0) name fuse
> returned -22
> [   10.859196] WARNING: CPU: 5 PID: 130 at kernel/module/strict_rwx.c:22
> module_set_memory+0x9c/0xb8
> [   10.891382] pc : module_set_memory+0x9c/0xb8
> [   10.895629] lr : module_set_memory+0x9c/0xb8
> [   10.976663]  module_set_memory+0x9c/0xb8
> 
> 
> 
>> diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
>> index a14df9655dbe..fdf8484154dd 100644
>> --- a/kernel/module/strict_rwx.c
>> +++ b/kernel/module/strict_rwx.c
>> @@ -15,9 +15,12 @@ static int module_set_memory(const struct module
>> *mod, enum mod_mem_type type,
>>                              int (*set_memory)(unsigned long start, int num_pages))
>>     {
>>        const struct module_memory *mod_mem = &mod->mem[type];
>> +     int err;
>>
>>        set_vm_flush_reset_perms(mod_mem->base);
>> -     return set_memory((unsigned long)mod_mem->base, mod_mem->size >>
>> PAGE_SHIFT);
>> +     err = set_memory((unsigned long)mod_mem->base, mod_mem->size >>
>> PAGE_SHIFT);
>> +     WARN(err, "module_set_memory(%d, %px, %x) returned %d\n", type,
>> mod_mem->base, mod_mem->size, err);
>> +     return err;
>>     }
>>
>>     /*
>>
>>
>> Thanks for your help
>> Christophe
> 
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel