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>
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>
Subject: Re: [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time
Date: Wed, 31 Jan 2024 11:58:00 +0000	[thread overview]
Message-ID: <46627d92-976a-4126-b261-a4c6575e5a3e@csgroup.eu> (raw)
In-Reply-To: <30ddedc9-0829-4a99-9cb1-39190937981c@samsung.com>

Hi,

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 ]
> 
> Dear All,
> 
> 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 ?

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
