Received-SPF: pass (google.com: domain of linux-kernel+bounces-46698-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <2febff4d-26ba-4809-a124-7add25ab3d35@samsung.com>
Date: Wed, 31 Jan 2024 16:17:20 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro()
 at all time
Content-Language: en-US
To: Christophe Leroy <christophe.leroy@csgroup.eu>, 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>
From: Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <46627d92-976a-4126-b261-a4c6575e5a3e@csgroup.eu>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <7b5df1782e94a755b4a18733af44d17d8dd8b37b.1703149011.git.christophe.leroy@csgroup.eu>
	<ZbgGDlgrLhB8tcGI@bombadil.infradead.org>
	<20240130091626.GA3684878@google.com>
	<9e298fa7-a953-462a-96a4-56a1b4316a17@csgroup.eu>
	<CGME20240130174812eucas1p166f62549457fd188fed6ed72b6b4b9cd@eucas1p1.samsung.com>
	<30ddedc9-0829-4a99-9cb1-39190937981c@samsung.com>
	<46627d92-976a-4126-b261-a4c6575e5a3e@csgroup.eu>

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
[    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