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

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

[Christophe Leroy]

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>
---
 include/linux/init.h |  4 ----
 init/main.c          | 21 +++++++--------------
 2 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/include/linux/init.h b/include/linux/init.h
index 01b52c9c7526..d2b47be38a07 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -168,12 +168,8 @@ extern initcall_entry_t __initcall_end[];
 
 extern struct file_system_type rootfs_fs_type;
 
-#if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
 extern bool rodata_enabled;
-#endif
-#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void);
-#endif
 
 extern void (*late_time_init)(void);
 
diff --git a/init/main.c b/init/main.c
index e24b0780fdff..807df08c501f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1396,10 +1396,9 @@ static int __init set_debug_rodata(char *str)
 early_param("rodata", set_debug_rodata);
 #endif
 
-#ifdef CONFIG_STRICT_KERNEL_RWX
 static void mark_readonly(void)
 {
-	if (rodata_enabled) {
+	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled) {
 		/*
 		 * load_module() results in W+X mappings, which are cleaned
 		 * up with call_rcu().  Let's make sure that queued work is
@@ -1409,20 +1408,14 @@ static void mark_readonly(void)
 		rcu_barrier();
 		mark_rodata_ro();
 		rodata_test();
-	} else
+	} else if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) {
 		pr_info("Kernel memory protection disabled.\n");
+	} else if (IS_ENABLED(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)) {
+		pr_warn("Kernel memory protection not selected by kernel config.\n");
+	} else {
+		pr_warn("This architecture does not have kernel memory protection.\n");
+	}
 }
-#elif defined(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)
-static inline void mark_readonly(void)
-{
-	pr_warn("Kernel memory protection not selected by kernel config.\n");
-}
-#else
-static inline void mark_readonly(void)
-{
-	pr_warn("This architecture does not have kernel memory protection.\n");
-}
-#endif
 
 void __weak free_initmem(void)
 {
-- 
2.41.0

[PATCH 2/3] modules: Remove #ifdef CONFIG_STRICT_MODULE_RWX around rodata_enabled

[Christophe Leroy]

Now that rodata_enabled is declared at all time, the #ifdef
CONFIG_STRICT_MODULE_RWX can be removed.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 kernel/module/strict_rwx.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index a2b656b4e3d2..eadff63b6e80 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -34,12 +34,8 @@ void module_enable_x(const struct module *mod)
 
 void module_enable_ro(const struct module *mod, bool after_init)
 {
-	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
-		return;
-#ifdef CONFIG_STRICT_MODULE_RWX
-	if (!rodata_enabled)
+	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX) || !rodata_enabled)
 		return;
-#endif
 
 	module_set_memory(mod, MOD_TEXT, set_memory_ro);
 	module_set_memory(mod, MOD_INIT_TEXT, set_memory_ro);
-- 
2.41.0

[PATCH 3/3] powerpc: Simplify strict_kernel_rwx_enabled()

[Christophe Leroy]

Now that rodata_enabled is always declared, remove #ifdef
and define a single version of strict_kernel_rwx_enabled().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/mmu.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index d8b7e246a32f..24241995f740 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -330,17 +330,10 @@ static __always_inline bool early_radix_enabled(void)
 	return early_mmu_has_feature(MMU_FTR_TYPE_RADIX);
 }
 
-#ifdef CONFIG_STRICT_KERNEL_RWX
 static inline bool strict_kernel_rwx_enabled(void)
 {
-	return rodata_enabled;
+	return IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled;
 }
-#else
-static inline bool strict_kernel_rwx_enabled(void)
-{
-	return false;
-}
-#endif
 
 static inline bool strict_module_rwx_enabled(void)
 {
-- 
2.41.0

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

[Michael Ellerman]

Cc +Kees

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 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>
> ---
>  include/linux/init.h |  4 ----
>  init/main.c          | 21 +++++++--------------
>  2 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 01b52c9c7526..d2b47be38a07 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -168,12 +168,8 @@ extern initcall_entry_t __initcall_end[];
>  
>  extern struct file_system_type rootfs_fs_type;
>  
> -#if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
>  extern bool rodata_enabled;
> -#endif
> -#ifdef CONFIG_STRICT_KERNEL_RWX
>  void mark_rodata_ro(void);
> -#endif
>  
>  extern void (*late_time_init)(void);
>  
> diff --git a/init/main.c b/init/main.c
> index e24b0780fdff..807df08c501f 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1396,10 +1396,9 @@ static int __init set_debug_rodata(char *str)
>  early_param("rodata", set_debug_rodata);
>  #endif
>  
> -#ifdef CONFIG_STRICT_KERNEL_RWX
>  static void mark_readonly(void)
>  {
> -	if (rodata_enabled) {
> +	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled) {
>  		/*
>  		 * load_module() results in W+X mappings, which are cleaned
>  		 * up with call_rcu().  Let's make sure that queued work is
> @@ -1409,20 +1408,14 @@ static void mark_readonly(void)
>  		rcu_barrier();
>  		mark_rodata_ro();
>  		rodata_test();
> -	} else
> +	} else if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) {
>  		pr_info("Kernel memory protection disabled.\n");
> +	} else if (IS_ENABLED(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)) {
> +		pr_warn("Kernel memory protection not selected by kernel config.\n");
> +	} else {
> +		pr_warn("This architecture does not have kernel memory protection.\n");
> +	}
>  }
> -#elif defined(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)
> -static inline void mark_readonly(void)
> -{
> -	pr_warn("Kernel memory protection not selected by kernel config.\n");
> -}
> -#else
> -static inline void mark_readonly(void)
> -{
> -	pr_warn("This architecture does not have kernel memory protection.\n");
> -}
> -#endif
>  
>  void __weak free_initmem(void)
>  {
> -- 
> 2.41.0

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

[Kees Cook]

On December 21, 2023 4:16:56 AM PST, Michael Ellerman <mpe@ellerman.id.au> wrote:
>Cc +Kees
>
>Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> 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>
>> ---
>>  include/linux/init.h |  4 ----
>>  init/main.c          | 21 +++++++--------------
>>  2 files changed, 7 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/linux/init.h b/include/linux/init.h
>> index 01b52c9c7526..d2b47be38a07 100644
>> --- a/include/linux/init.h
>> +++ b/include/linux/init.h
>> @@ -168,12 +168,8 @@ extern initcall_entry_t __initcall_end[];
>>  
>>  extern struct file_system_type rootfs_fs_type;
>>  
>> -#if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
>>  extern bool rodata_enabled;
>> -#endif
>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>>  void mark_rodata_ro(void);
>> -#endif
>>  
>>  extern void (*late_time_init)(void);
>>  
>> diff --git a/init/main.c b/init/main.c
>> index e24b0780fdff..807df08c501f 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -1396,10 +1396,9 @@ static int __init set_debug_rodata(char *str)
>>  early_param("rodata", set_debug_rodata);
>>  #endif
>>  
>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>>  static void mark_readonly(void)
>>  {
>> -	if (rodata_enabled) {
>> +	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled) {

I think this will break without rodata_enabled actual existing on other architectures. (Only declaration was made visible, not the definition, which is above here and still behind ifdefs?)

-Kees

>>  		/*
>>  		 * load_module() results in W+X mappings, which are cleaned
>>  		 * up with call_rcu().  Let's make sure that queued work is
>> @@ -1409,20 +1408,14 @@ static void mark_readonly(void)
>>  		rcu_barrier();
>>  		mark_rodata_ro();
>>  		rodata_test();
>> -	} else
>> +	} else if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) {
>>  		pr_info("Kernel memory protection disabled.\n");
>> +	} else if (IS_ENABLED(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)) {
>> +		pr_warn("Kernel memory protection not selected by kernel config.\n");
>> +	} else {
>> +		pr_warn("This architecture does not have kernel memory protection.\n");
>> +	}
>>  }
>> -#elif defined(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)
>> -static inline void mark_readonly(void)
>> -{
>> -	pr_warn("Kernel memory protection not selected by kernel config.\n");
>> -}
>> -#else
>> -static inline void mark_readonly(void)
>> -{
>> -	pr_warn("This architecture does not have kernel memory protection.\n");
>> -}
>> -#endif
>>  
>>  void __weak free_initmem(void)
>>  {
>> -- 
>> 2.41.0

-- 
Kees Cook

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

[Christophe Leroy]

Le 22/12/2023 à 06:35, Kees Cook a écrit :
> [Vous ne recevez pas souvent de courriers de kees@kernel.org. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> On December 21, 2023 4:16:56 AM PST, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Cc +Kees
>>
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> 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>
>>> ---
>>>   include/linux/init.h |  4 ----
>>>   init/main.c          | 21 +++++++--------------
>>>   2 files changed, 7 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/include/linux/init.h b/include/linux/init.h
>>> index 01b52c9c7526..d2b47be38a07 100644
>>> --- a/include/linux/init.h
>>> +++ b/include/linux/init.h
>>> @@ -168,12 +168,8 @@ extern initcall_entry_t __initcall_end[];
>>>
>>>   extern struct file_system_type rootfs_fs_type;
>>>
>>> -#if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
>>>   extern bool rodata_enabled;
>>> -#endif
>>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>>>   void mark_rodata_ro(void);
>>> -#endif
>>>
>>>   extern void (*late_time_init)(void);
>>>
>>> diff --git a/init/main.c b/init/main.c
>>> index e24b0780fdff..807df08c501f 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -1396,10 +1396,9 @@ static int __init set_debug_rodata(char *str)
>>>   early_param("rodata", set_debug_rodata);
>>>   #endif
>>>
>>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>>>   static void mark_readonly(void)
>>>   {
>>> -    if (rodata_enabled) {
>>> +    if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled) {
> 
> I think this will break without rodata_enabled actual existing on other architectures. (Only declaration was made visible, not the definition, which is above here and still behind ifdefs?)

The compiler constant-folds IS_ENABLED(CONFIG_STRICT_KERNEL_RWX).
When it is false, the second part is dropped.

Exemple:

bool test(void)
{
	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled)
		return true;
	else
		return false;
}

With CONFIG_STRICT_KERNEL_RWX set, it directly returns the content of 
rodata_enabled:

00000160 <test>:
  160:	3d 20 00 00 	lis     r9,0
			162: R_PPC_ADDR16_HA	rodata_enabled
  164:	88 69 00 00 	lbz     r3,0(r9)
			166: R_PPC_ADDR16_LO	rodata_enabled
  168:	4e 80 00 20 	blr

With CONFIG_STRICT_KERNEL_RWX unset, it returns 0 and doesn't reference 
rodata_enabled at all:

000000bc <test>:
   bc:	38 60 00 00 	li      r3,0
   c0:	4e 80 00 20 	blr

Many places in the kernel use this approach to minimise amount of #ifdefs.

Christophe

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

[Luis Chamberlain]

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

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

[Chen-Yu Tsai]

Hi,

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.

[   10.481015] Unable to handle kernel paging request at virtual address ffffffde85245d30
[   10.490369] KASAN: maybe wild-memory-access in range [0x000000f42922e980-0x000000f42922e987]
[   10.503744] Mem abort info:
[   10.509383]   ESR = 0x0000000096000047
[   10.514400]   EC = 0x25: DABT (current EL), IL = 32 bits
[   10.522366]   SET = 0, FnV = 0
[   10.526343]   EA = 0, S1PTW = 0
[   10.530695]   FSC = 0x07: level 3 translation fault
[   10.537081] Data abort info:
[   10.540839]   ISV = 0, ISS = 0x00000047, ISS2 = 0x00000000
[   10.546456]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
[   10.551726]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[   10.557612] swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000041f98000
[   10.565214] [ffffffde85245d30] pgd=100000023ffff003, p4d=100000023ffff003, pud=100000023ffff003, pmd=10000001121eb003, pte=0000000000000000
[   10.578887] Internal error: Oops: 0000000096000047 [#1] PREEMPT SMP
[   10.585815] Modules linked in:
[   10.590235] CPU: 6 PID: 195 Comm: (udev-worker) Tainted: G    B              6.8.0-rc2-next-20240130-02908-ge8ad01d60927-dirty #163 3f2318148ecc5fa70d1092c2b874f9b59bdb7d60
[   10.607021] Hardware name: Google Tentacruel board (DT)
[   10.613607] pstate: a0400009 (NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   10.621954] pc : module_bug_finalize+0x118/0x148
[   10.626823] lr : module_bug_finalize+0x118/0x148
[   10.631463] sp : ffffffc0820478d0
[   10.631466] x29: ffffffc0820478d0 x28: ffffffc082047ca0 x27: ffffffde8d7d31a0
[   10.631477] x26: ffffffde85223780 x25: 0000000000000000 x24: ffffffde8c413cc0
[   10.631486] x23: ffffffde8dfcec80 x22: ffffffde8dfce000 x21: ffffffde85223ba8
[   10.631495] x20: ffffffde85223780 x19: ffffffde85245d28 x18: 0000000000000000
[   10.631504] x17: ffffffde8aa15938 x16: ffffffde8aabdd90 x15: ffffffde8aab8124
[   10.631513] x14: ffffffde8acdd380 x13: 0000000041b58ab3 x12: ffffffbbd1bf9d91
[   10.631522] x11: 1ffffffbd1bf9d90 x10: ffffffbbd1bf9d90 x9 : dfffffc000000000
[   10.631531] x8 : 000000442e406270 x7 : ffffffde8dfcec87 x6 : 0000000000000001
[   10.631539] x5 : ffffffde8dfcec80 x4 : 0000000000000000 x3 : ffffffde8bbadf08
[   10.631548] x2 : 0000000000000001 x1 : ffffffde8eaff080 x0 : 0000000000000000
[   10.631556] Call trace:
[   10.631559]  module_bug_finalize+0x118/0x148
[   10.631565]  load_module+0x25ec/0x2a78
[   10.631572]  __do_sys_init_module+0x234/0x418
[   10.631578]  __arm64_sys_init_module+0x4c/0x68
[   10.631584]  invoke_syscall+0x68/0x198
[   10.631589]  el0_svc_common.constprop.0+0x11c/0x150
[   10.631594]  do_el0_svc+0x38/0x50
[   10.631598]  el0_svc+0x50/0xa0
[   10.631604]  el0t_64_sync_handler+0x120/0x130
[   10.631609]  el0t_64_sync+0x1a8/0x1b0
[   10.631619] Code: 97c5418e c89ffef5 91002260 97c53ca7 (f9000675)
[   10.631624] ---[ end trace 0000000000000000 ]---
[   10.642965] Kernel panic - not syncing: Oops: Fatal exception
[   10.642975] SMP: stopping secondary CPUs
[   10.648339] Kernel Offset: 0x1e0a800000 from 0xffffffc080000000
[   10.648343] PHYS_OFFSET: 0x40000000
[   10.648345] CPU features: 0x0,c0000061,7002814a,2100720b
[   10.648350] Memory Limit: none

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

[Christophe Leroy]

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 ]
> 
> Hi,
> 
> 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.

Thanks
Christophe


> 
> [   10.481015] Unable to handle kernel paging request at virtual address ffffffde85245d30
> [   10.490369] KASAN: maybe wild-memory-access in range [0x000000f42922e980-0x000000f42922e987]
> [   10.503744] Mem abort info:
> [   10.509383]   ESR = 0x0000000096000047
> [   10.514400]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   10.522366]   SET = 0, FnV = 0
> [   10.526343]   EA = 0, S1PTW = 0
> [   10.530695]   FSC = 0x07: level 3 translation fault
> [   10.537081] Data abort info:
> [   10.540839]   ISV = 0, ISS = 0x00000047, ISS2 = 0x00000000
> [   10.546456]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> [   10.551726]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [   10.557612] swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000041f98000
> [   10.565214] [ffffffde85245d30] pgd=100000023ffff003, p4d=100000023ffff003, pud=100000023ffff003, pmd=10000001121eb003, pte=0000000000000000
> [   10.578887] Internal error: Oops: 0000000096000047 [#1] PREEMPT SMP
> [   10.585815] Modules linked in:
> [   10.590235] CPU: 6 PID: 195 Comm: (udev-worker) Tainted: G    B              6.8.0-rc2-next-20240130-02908-ge8ad01d60927-dirty #163 3f2318148ecc5fa70d1092c2b874f9b59bdb7d60
> [   10.607021] Hardware name: Google Tentacruel board (DT)
> [   10.613607] pstate: a0400009 (NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   10.621954] pc : module_bug_finalize+0x118/0x148
> [   10.626823] lr : module_bug_finalize+0x118/0x148
> [   10.631463] sp : ffffffc0820478d0
> [   10.631466] x29: ffffffc0820478d0 x28: ffffffc082047ca0 x27: ffffffde8d7d31a0
> [   10.631477] x26: ffffffde85223780 x25: 0000000000000000 x24: ffffffde8c413cc0
> [   10.631486] x23: ffffffde8dfcec80 x22: ffffffde8dfce000 x21: ffffffde85223ba8
> [   10.631495] x20: ffffffde85223780 x19: ffffffde85245d28 x18: 0000000000000000
> [   10.631504] x17: ffffffde8aa15938 x16: ffffffde8aabdd90 x15: ffffffde8aab8124
> [   10.631513] x14: ffffffde8acdd380 x13: 0000000041b58ab3 x12: ffffffbbd1bf9d91
> [   10.631522] x11: 1ffffffbd1bf9d90 x10: ffffffbbd1bf9d90 x9 : dfffffc000000000
> [   10.631531] x8 : 000000442e406270 x7 : ffffffde8dfcec87 x6 : 0000000000000001
> [   10.631539] x5 : ffffffde8dfcec80 x4 : 0000000000000000 x3 : ffffffde8bbadf08
> [   10.631548] x2 : 0000000000000001 x1 : ffffffde8eaff080 x0 : 0000000000000000
> [   10.631556] Call trace:
> [   10.631559]  module_bug_finalize+0x118/0x148
> [   10.631565]  load_module+0x25ec/0x2a78
> [   10.631572]  __do_sys_init_module+0x234/0x418
> [   10.631578]  __arm64_sys_init_module+0x4c/0x68
> [   10.631584]  invoke_syscall+0x68/0x198
> [   10.631589]  el0_svc_common.constprop.0+0x11c/0x150
> [   10.631594]  do_el0_svc+0x38/0x50
> [   10.631598]  el0_svc+0x50/0xa0
> [   10.631604]  el0t_64_sync_handler+0x120/0x130
> [   10.631609]  el0t_64_sync+0x1a8/0x1b0
> [   10.631619] Code: 97c5418e c89ffef5 91002260 97c53ca7 (f9000675)
> [   10.631624] ---[ end trace 0000000000000000 ]---
> [   10.642965] Kernel panic - not syncing: Oops: Fatal exception
> [   10.642975] SMP: stopping secondary CPUs
> [   10.648339] Kernel Offset: 0x1e0a800000 from 0xffffffc080000000
> [   10.648343] PHYS_OFFSET: 0x40000000
> [   10.648345] CPU features: 0x0,c0000061,7002814a,2100720b
> [   10.648350] Memory Limit: none
> 

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

[Marek Szyprowski]

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.

Here is the stack trace I've got on Khadas VIM3 ARM64 board:

Unable to handle kernel paging request at virtual address ffff80007bfeeb30
Mem abort info:
   ESR = 0x0000000096000047
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x07: level 3 translation fault
Data abort info:
   ISV = 0, ISS = 0x00000047, ISS2 = 0x00000000
   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
swapper pgtable: 4k pages, 48-bit VAs, pgdp=000000000a35a000
[ffff80007bfeeb30] pgd=10000000f4806003, p4d=10000000f4806003, 
pud=1000000007ed1003, pmd=1000000007ed2003, pte=0000000000000000
Internal error: Oops: 0000000096000047 [#1] PREEMPT SMP
Modules linked in:
CPU: 4 PID: 182 Comm: (udev-worker) Not tainted 6.8.0-rc2-next-20240130 
#14391
Hardware name: Khadas VIM3 (DT)
pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : module_bug_finalize+0xb0/0xdc
lr : module_bug_finalize+0x70/0xdc
...
Call trace:
  module_bug_finalize+0xb0/0xdc
  load_module+0x182c/0x1c88
  init_module_from_file+0x84/0xc0
  idempotent_init_module+0x180/0x250
  __arm64_sys_finit_module+0x64/0xa0
  invoke_syscall+0x48/0x114
  el0_svc_common.constprop.0+0xc0/0xe0
  do_el0_svc+0x1c/0x28
  el0_svc+0x4c/0xe4
  el0t_64_sync_handler+0xc0/0xc4
  el0t_64_sync+0x190/0x194
Code: 9116e003 f942dc01 a93e8c41 c89ffc73 (f9000433)
---[ end trace 0000000000000000 ]---



Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Luis Chamberlain]

On Tue, Jan 30, 2024 at 06:48:11PM +0100, Marek Szyprowski wrote:
> 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've reset my tree to commit
3559ad395bf02 ("module: Change module_enable_{nx/x/ro}() to more
explicit names") for now then, so to remove those commits.

  Luis

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

[Christophe Leroy]

Le 30/01/2024 à 21:27, Luis Chamberlain a écrit :
> On Tue, Jan 30, 2024 at 06:48:11PM +0100, Marek Szyprowski wrote:
>> 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've reset my tree to commit
> 3559ad395bf02 ("module: Change module_enable_{nx/x/ro}() to more
> explicit names") for now then, so to remove those commits.
> 

The problem being identified in commit 677bfb9db8a3 ("module: Don't 
ignore errors from set_memory_XX()"), you can keep/re-apply the series 
[PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time.

Christophe

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

[Luis Chamberlain]

On Wed, Jan 31, 2024 at 06:53:13AM +0000, Christophe Leroy wrote:
> The problem being identified in commit 677bfb9db8a3 ("module: Don't 
> ignore errors from set_memory_XX()"), you can keep/re-apply the series 
> [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time.

Sure, queued that up into modules-testing before I push to modules-next.

  Luis

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

[Christophe Leroy]

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

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

[Marek Szyprowski]

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

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

[Christophe Leroy]

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
> 

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

[Marek Szyprowski]

Hi Christophe,

On 31.01.2024 21:07, Christophe Leroy wrote:
> 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 ]
>>
>> 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'.

Here are relevant parts of the boot log:

[    8.096850] ------------[ cut here ]------------
[    8.096939] module_set_memory(6, 0000000000000000, 0) name ipv6 
returned -22
[    8.102947] WARNING: CPU: 4 PID: 1 at kernel/module/strict_rwx.c:22 
module_set_memory+0x9c/0xb8
[    8.111561] Modules linked in:
[    8.114596] CPU: 4 PID: 1 Comm: systemd Not tainted 
6.8.0-rc2-next-20240130-dirty #14429
[    8.122654] Hardware name: Khadas VIM3 (DT)
[    8.126815] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[    8.133747] pc : module_set_memory+0x9c/0xb8
[    8.137994] lr : module_set_memory+0x9c/0xb8
[    8.142240] sp : ffff800083fcba80
[    8.145534] x29: ffff800083fcba80 x28: 0000000000000001 x27: 
ffff80007c024448
[    8.152640] x26: ffff800083fcbc10 x25: ffff80007c007958 x24: 
ffff80007c024450
[    8.159747] x23: ffff800083f2a090 x22: ffff80007c007940 x21: 
0000000000000006
[    8.166854] x20: 00000000ffffffea x19: ffff80007c007af0 x18: 
0000000000000030
[    8.173960] x17: 0000000000000000 x16: 0000000000005932 x15: 
ffffffffffffffff
[    8.181067] x14: ffff800082ea5658 x13: 00000000000003d5 x12: 
0000000000000147
[    8.188174] x11: 6920656d616e2029 x10: ffff800082efd658 x9 : 
00000000fffff000
[    8.195280] x8 : ffff800082ea5658 x7 : ffff800082efd658 x6 : 
0000000000000000
[    8.202387] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 
0000000000000000
[    8.209494] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff0000001f0000
[    8.216601] Call trace:
[    8.219027]  module_set_memory+0x9c/0xb8
[    8.222927]  module_enable_rodata_ro+0x64/0xa0
[    8.227347]  load_module+0x1838/0x1c88
[    8.231074]  init_module_from_file+0x84/0xc0
[    8.235320]  idempotent_init_module+0x180/0x250
[    8.239827]  __arm64_sys_finit_module+0x64/0xa0
[    8.244334]  invoke_syscall+0x48/0x114
[    8.248060]  el0_svc_common.constprop.0+0x40/0xe0
[    8.252740]  do_el0_svc+0x1c/0x28
[    8.256034]  el0_svc+0x4c/0xe4
[    8.259067]  el0t_64_sync_handler+0xc0/0xc4
[    8.263227]  el0t_64_sync+0x190/0x194
[    8.266868] irq event stamp: 1304234
[    8.270421] hardirqs last  enabled at (1304233): [<ffff80008012c95c>] 
console_unlock+0x120/0x12c
[    8.279174] hardirqs last disabled at (1304234): [<ffff800081239bc0>] 
el1_dbg+0x24/0x8c
[    8.287147] softirqs last  enabled at (1301556): [<ffff800080010a60>] 
__do_softirq+0x4a0/0x4e8
[    8.295727] softirqs last disabled at (1301545): [<ffff8000800169b0>] 
____do_softirq+0x10/0x1c
[    8.304307] ---[ end trace 0000000000000000 ]---

[    8.508381] ------------[ cut here ]------------
[    8.508432] module_set_memory(6, 0000000000000000, 0) name x_tables 
returned -22
[    8.514785] WARNING: CPU: 2 PID: 1 at kernel/module/strict_rwx.c:22 
module_set_memory+0x9c/0xb8
[    8.523410] Modules linked in:
[    8.526444] CPU: 2 PID: 1 Comm: systemd Tainted: G W          
6.8.0-rc2-next-20240130-dirty #14429
[    8.535976] Hardware name: Khadas VIM3 (DT)
[    8.540137] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[    8.547069] pc : module_set_memory+0x9c/0xb8
[    8.551316] lr : module_set_memory+0x9c/0xb8
[    8.555562] sp : ffff800083fcba80
[    8.558856] x29: ffff800083fcba80 x28: 0000000000000001 x27: 
ffff80007bfab1f0
[    8.565963] x26: ffff800083fcbc10 x25: ffff80007bfa5458 x24: 
ffff80007bfab1f8
[    8.573069] x23: ffff800083f2a090 x22: ffff80007bfa5440 x21: 
0000000000000006
[    8.580176] x20: 00000000ffffffea x19: ffff80007bfa55f0 x18: 
0000000000000030
[    8.587282] x17: 64656e7275746572 x16: 2073656c6261745f x15: 
7820656d616e2029
[    8.594389] x14: ffff800082ea5658 x13: 000000000000044a x12: 
000000000000016e
[    8.601496] x11: 6261745f7820656d x10: ffff800082efd658 x9 : 
00000000fffff000
[    8.608602] x8 : ffff800082ea5658 x7 : ffff800082efd658 x6 : 
0000000000000000
[    8.615709] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 
0000000000000000
[    8.622816] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff0000001f0000
[    8.629923] Call trace:
[    8.632349]  module_set_memory+0x9c/0xb8
[    8.636249]  module_enable_rodata_ro+0x64/0xa0
[    8.640669]  load_module+0x1838/0x1c88
[    8.644396]  init_module_from_file+0x84/0xc0
[    8.648642]  idempotent_init_module+0x180/0x250
[    8.653149]  __arm64_sys_finit_module+0x64/0xa0
[    8.657656]  invoke_syscall+0x48/0x114
[    8.661383]  el0_svc_common.constprop.0+0x40/0xe0
[    8.666062]  do_el0_svc+0x1c/0x28
[    8.669356]  el0_svc+0x4c/0xe4
[    8.672389]  el0t_64_sync_handler+0xc0/0xc4
[    8.676549]  el0t_64_sync+0x190/0x194
[    8.680190] irq event stamp: 1304890
[    8.683742] hardirqs last  enabled at (1304889): [<ffff80008012c95c>] 
console_unlock+0x120/0x12c
[    8.692496] hardirqs last disabled at (1304890): [<ffff800081239bc0>] 
el1_dbg+0x24/0x8c
[    8.700469] softirqs last  enabled at (1304870): [<ffff800080010a60>] 
__do_softirq+0x4a0/0x4e8
[    8.709049] softirqs last disabled at (1304865): [<ffff8000800169b0>] 
____do_softirq+0x10/0x1c
[    8.717629] ---[ end trace 0000000000000000 ]---

[   10.560872] ------------[ cut here ]------------
[   10.560924] module_set_memory(6, 0000000000000000, 0) name dm_mod 
returned -22
[   10.567128] WARNING: CPU: 4 PID: 127 at kernel/module/strict_rwx.c:22 
module_set_memory+0x9c/0xb8
[   10.575902] Modules linked in:
[   10.578937] CPU: 4 PID: 127 Comm: modprobe Tainted: G W          
6.8.0-rc2-next-20240130-dirty #14429
[   10.588728] Hardware name: Khadas VIM3 (DT)
[   10.592889] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[   10.599821] pc : module_set_memory+0x9c/0xb8
[   10.604068] lr : module_set_memory+0x9c/0xb8
[   10.608314] sp : ffff800084da3a80
[   10.611608] x29: ffff800084da3a80 x28: 0000000000000001 x27: 
ffff80007bfbf22c
[   10.618714] x26: ffff800084da3c10 x25: ffff80007bfb4c58 x24: 
ffff80007bfbf234
[   10.623602] systemd[1]: Starting modprobe@efi_pstore.service - Load 
Kernel Module efi_pstore...
[   10.625820] x23: ffff800083f2a090 x22: ffff80007bfb4c40 x21: 
0000000000000006
[   10.625848] x20: 00000000ffffffea x19: ffff80007bfb4df0 x18: 
0000000000000030
[   10.648751] x17: 2d2064656e727574 x16: 657220646f6d5f6d x15: 
6420656d616e2029
[   10.648768] x14: ffff800082ea5658 x13: 000000000000051f x12: 
00000000000001b5
[   10.648782] x11: 5f6d6420656d616e x10: ffff800082efd658 x9 : 
00000000fffff000
[   10.648795] x8 : ffff800082ea5658 x7 : ffff800082efd658 x6 : 
0000000000000000
[   10.677128] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 
0000000000000000
[   10.684235] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff00003d341b80
[   10.691342] Call trace:
[   10.693768]  module_set_memory+0x9c/0xb8
[   10.697668]  module_enable_rodata_ro+0x64/0xa0
[   10.702088]  load_module+0x1838/0x1c88
[   10.705815]  init_module_from_file+0x84/0xc0
[   10.710061]  idempotent_init_module+0x180/0x250
[   10.714568]  __arm64_sys_finit_module+0x64/0xa0
[   10.719075]  invoke_syscall+0x48/0x114
[   10.722801]  el0_svc_common.constprop.0+0x40/0xe0
[   10.727481]  do_el0_svc+0x1c/0x28
[   10.730774]  el0_svc+0x4c/0xe4
[   10.733808]  el0t_64_sync_handler+0xc0/0xc4
[   10.737968]  el0t_64_sync+0x190/0x194
[   10.741609] irq event stamp: 13696
[   10.744988] hardirqs last  enabled at (13695): [<ffff80008012c95c>] 
console_unlock+0x120/0x12c
[   10.753568] hardirqs last disabled at (13696): [<ffff800081239bc0>] 
el1_dbg+0x24/0x8c
[   10.761368] softirqs last  enabled at (13692): [<ffff800080010a60>] 
__do_softirq+0x4a0/0x4e8
[   10.769774] softirqs last disabled at (13683): [<ffff8000800169b0>] 
____do_softirq+0x10/0x1c
[   10.778181] ---[ end trace 0000000000000000 ]---

[   10.860938] ------------[ cut here ]------------
[   10.860989] module_set_memory(6, 0000000000000000, 0) name fuse 
returned -22
[   10.867007] WARNING: CPU: 3 PID: 130 at kernel/module/strict_rwx.c:22 
module_set_memory+0x9c/0xb8
[   10.875796] Modules linked in:
[   10.878829] CPU: 3 PID: 130 Comm: modprobe Tainted: G W          
6.8.0-rc2-next-20240130-dirty #14429
[   10.888621] Hardware name: Khadas VIM3 (DT)
[   10.892781] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[   10.899714] pc : module_set_memory+0x9c/0xb8
[   10.903960] lr : module_set_memory+0x9c/0xb8
[   10.908207] sp : ffff800084d8ba80
[   10.911500] x29: ffff800084d8ba80 x28: 0000000000000001 x27: 
ffff80007bff7148
[   10.918607] x26: ffff800084d8bc10 x25: ffff80007bfee758 x24: 
ffff80007bff7150
[   10.925714] x23: ffff800083f2a090 x22: ffff80007bfee740 x21: 
0000000000000006
[   10.932820] x20: 00000000ffffffea x19: ffff80007bfee8f0 x18: 
0000000000000030
[   10.939927] x17: 0000000000000000 x16: 0000000000000000 x15: 
ffffffffffffffff
[   10.947034] x14: ffff800082ea5658 x13: 000000000000059d x12: 
00000000000001df
[   10.954140] x11: 6620656d616e2029 x10: ffff800082efd658 x9 : 
00000000fffff000
[   10.961247] x8 : ffff800082ea5658 x7 : ffff800082efd658 x6 : 
0000000000000000
[   10.968354] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 
0000000000000000
[   10.975460] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff00000268d280
[   10.982568] Call trace:
[   10.984994]  module_set_memory+0x9c/0xb8
[   10.988894]  module_enable_rodata_ro+0x64/0xa0
[   10.993314]  load_module+0x1838/0x1c88
[   10.997041]  init_module_from_file+0x84/0xc0
[   11.001287]  idempotent_init_module+0x180/0x250
[   11.005794]  __arm64_sys_finit_module+0x64/0xa0
[   11.010301]  invoke_syscall+0x48/0x114
[   11.014027]  el0_svc_common.constprop.0+0x40/0xe0
[   11.018707]  do_el0_svc+0x1c/0x28
[   11.022000]  el0_svc+0x4c/0xe4
[   11.025034]  el0t_64_sync_handler+0xc0/0xc4
[   11.029194]  el0t_64_sync+0x190/0x194
[   11.032835] irq event stamp: 13612
[   11.036214] hardirqs last  enabled at (13611): [<ffff80008012c95c>] 
console_unlock+0x120/0x12c
[   11.044794] hardirqs last disabled at (13612): [<ffff800081239bc0>] 
el1_dbg+0x24/0x8c
[   11.052594] softirqs last  enabled at (13584): [<ffff800080010a60>] 
__do_softirq+0x4a0/0x4e8
[   11.061000] softirqs last disabled at (13333): [<ffff8000800169b0>] 
____do_softirq+0x10/0x1c
[   11.069407] ---[ end trace 0000000000000000 ]---


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

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland