[PATCH 0/7] powerpc: build / linker improvements

[PATCH 0/7] powerpc: build / linker improvements

[Nicholas Piggin]

This series is mainly about moving more things out of writable and
executable memory, and slightly moving the linker script in the
direction of the binutils ld internal linker script as we do.

Thanks,
Nick

Nicholas Piggin (7):
  powerpc/build: put sys_call_table in .data.rel.ro if RELOCATABLE
  powerpc: move __end_rodata to cover arch read-only sections
  powerpc/32/build: move got1/got2 sections out of text
  powerpc/build: move got, toc, plt, branch_lt sections to read-only
  powerpc/build: move .data.rel.ro, .sdata2 to read-only
  powerpc/64/build: only include .opd with ELFv1
  powerpc/64/build: merge .got and .toc input sections

 arch/powerpc/kernel/systbl.S             |  4 ++
 arch/powerpc/kernel/vmlinux.lds.S        | 85 +++++++++++++++---------
 arch/powerpc/mm/book3s32/mmu.c           |  2 +-
 arch/powerpc/mm/book3s64/hash_pgtable.c  |  2 +-
 arch/powerpc/mm/book3s64/radix_pgtable.c |  2 +-
 arch/powerpc/mm/pgtable_32.c             |  5 +-
 6 files changed, 62 insertions(+), 38 deletions(-)

-- 
2.37.2

[PATCH 1/7] powerpc/build: put sys_call_table in .data.rel.ro if RELOCATABLE

[Nicholas Piggin]

Const function pointers live in .data.rel.ro rather than .rodata because
they must be relocated. This change prevents powerpc/32 from generating
R_PPC_UADDR32 relocations (which are not handled). The sys_call_table is
moved to writeable memory, but a later change will move it back.

After this patch, 44x_defconfig + CONFIG_RELOCATABLE boots to busybox.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/systbl.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/systbl.S b/arch/powerpc/kernel/systbl.S
index cb3358886203..0bec33e86f50 100644
--- a/arch/powerpc/kernel/systbl.S
+++ b/arch/powerpc/kernel/systbl.S
@@ -12,7 +12,11 @@
 
 #include <asm/ppc_asm.h>
 
+#ifdef CONFIG_RELOCATABLE
+.section .data.rel.ro,"aw"
+#else
 .section .rodata,"a"
+#endif
 
 #ifdef CONFIG_PPC64
 	.p2align	3
-- 
2.37.2

[PATCH 2/7] powerpc: move __end_rodata to cover arch read-only sections

[Nicholas Piggin]

powerpc has a number of read-only sections and tables that are put
after RO_DATA(). Move the __end_rodata symbol to cover these as well.

Setting memory to read-only at boot is done using __init_begin,
change that that to use __end_rodata.

This also affects boot dmesg, is_kernel_rodata(), and some other checks.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/vmlinux.lds.S        | 3 +++
 arch/powerpc/mm/book3s32/mmu.c           | 2 +-
 arch/powerpc/mm/book3s64/hash_pgtable.c  | 2 +-
 arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +-
 arch/powerpc/mm/pgtable_32.c             | 5 ++---
 5 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index fe22d940412f..90ac5ff73df2 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -210,6 +210,9 @@ SECTIONS
 	}
 #endif
 
+	. = ALIGN(PAGE_SIZE);
+	__end_rodata = .;
+
 /*
  * Init sections discarded at runtime
  */
diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
index a96b73006dfb..e13b883e4e5b 100644
--- a/arch/powerpc/mm/book3s32/mmu.c
+++ b/arch/powerpc/mm/book3s32/mmu.c
@@ -240,7 +240,7 @@ void mmu_mark_rodata_ro(void)
 	for (i = 0; i < nb; i++) {
 		struct ppc_bat *bat = BATS[i];
 
-		if (bat_addrs[i].start < (unsigned long)__init_begin)
+		if (bat_addrs[i].start < (unsigned long)__end_rodata)
 			bat[1].batl = (bat[1].batl & ~BPP_RW) | BPP_RX;
 	}
 
diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c
index ae008b9df0e6..28332001bd87 100644
--- a/arch/powerpc/mm/book3s64/hash_pgtable.c
+++ b/arch/powerpc/mm/book3s64/hash_pgtable.c
@@ -541,7 +541,7 @@ void hash__mark_rodata_ro(void)
 	unsigned long start, end, pp;
 
 	start = (unsigned long)_stext;
-	end = (unsigned long)__init_begin;
+	end = (unsigned long)__end_rodata;
 
 	pp = htab_convert_pte_flags(pgprot_val(PAGE_KERNEL_ROX), HPTE_USE_KERNEL_KEY);
 
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 698274109c91..2305f34bcc33 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -228,7 +228,7 @@ void radix__mark_rodata_ro(void)
 	unsigned long start, end;
 
 	start = (unsigned long)_stext;
-	end = (unsigned long)__init_begin;
+	end = (unsigned long)__end_rodata;
 
 	radix__change_memory_range(start, end, _PAGE_WRITE);
 }
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index 3ac73f9fb5d5..112af8c5447a 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -158,10 +158,9 @@ void mark_rodata_ro(void)
 	}
 
 	/*
-	 * mark .text and .rodata as read only. Use __init_begin rather than
-	 * __end_rodata to cover NOTES and EXCEPTION_TABLE.
+	 * mark .text and .rodata as read only.
 	 */
-	numpages = PFN_UP((unsigned long)__init_begin) -
+	numpages = PFN_UP((unsigned long)__end_rodata) -
 		   PFN_DOWN((unsigned long)_stext);
 
 	set_memory_ro((unsigned long)_stext, numpages);
-- 
2.37.2

[PATCH 3/7] powerpc/32/build: move got1/got2 sections out of text

[Nicholas Piggin]

Following the example from the binutils default linker script, move
.got1 and .got2 out of .text, to just after RO_DATA where they are in
read-only NX memory.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/vmlinux.lds.S | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 90ac5ff73df2..341ac79f49a9 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -122,14 +122,6 @@ SECTIONS
 		*(.sfpr);
 		MEM_KEEP(init.text)
 		MEM_KEEP(exit.text)
-
-#ifdef CONFIG_PPC32
-		*(.got1)
-		__got2_start = .;
-		*(.got2)
-		__got2_end = .;
-#endif /* CONFIG_PPC32 */
-
 	} :text
 
 	. = ALIGN(PAGE_SIZE);
@@ -139,7 +131,16 @@ SECTIONS
 	/* Read-only data */
 	RO_DATA(PAGE_SIZE)
 
-#ifdef CONFIG_PPC64
+#ifdef CONFIG_PPC32
+	.got1 : AT(ADDR(.got1) - LOAD_OFFSET) {
+		*(.got1)
+	}
+	.got2 : AT(ADDR(.got2) - LOAD_OFFSET) {
+		__got2_start = .;
+		*(.got2)
+		__got2_end = .;
+	}
+#else /* CONFIG_PPC32 */
 	SOFT_MASK_TABLE(8)
 	RESTART_TABLE(8)
 
@@ -190,7 +191,7 @@ SECTIONS
 		*(__rfi_flush_fixup)
 		__stop___rfi_flush_fixup = .;
 	}
-#endif /* CONFIG_PPC64 */
+#endif /* CONFIG_PPC32 */
 
 #ifdef CONFIG_PPC_BARRIER_NOSPEC
 	. = ALIGN(8);
-- 
2.37.2

[PATCH 4/7] powerpc/build: move got, toc, plt, branch_lt sections to read-only

[Nicholas Piggin]

This moves linker related tables from .data to read-only area.
Relocations are performed at early boot time before memory is protected,
after which there should be no modifications required.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/vmlinux.lds.S | 42 ++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 341ac79f49a9..716fff86c3fd 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -131,6 +131,10 @@ SECTIONS
 	/* Read-only data */
 	RO_DATA(PAGE_SIZE)
 
+	.branch_lt : AT(ADDR(.branch_lt) - LOAD_OFFSET) {
+		*(.branch_lt)
+	}
+
 #ifdef CONFIG_PPC32
 	.got1 : AT(ADDR(.got1) - LOAD_OFFSET) {
 		*(.got1)
@@ -140,7 +144,30 @@ SECTIONS
 		*(.got2)
 		__got2_end = .;
 	}
+	.got : AT(ADDR(.got) - LOAD_OFFSET) SPECIAL {
+		*(.got)
+		*(.got.plt)
+	}
+	.plt : AT(ADDR(.plt) - LOAD_OFFSET) SPECIAL {
+		/* XXX: is .plt (and .got.plt) required? */
+		*(.plt)
+	}
+
 #else /* CONFIG_PPC32 */
+	.toc1 : AT(ADDR(.toc1) - LOAD_OFFSET) {
+		*(.toc1)
+	}
+
+	.got : AT(ADDR(.got) - LOAD_OFFSET) ALIGN(256) {
+		*(.got)
+#ifndef CONFIG_RELOCATABLE
+		__prom_init_toc_start = .;
+		arch/powerpc/kernel/prom_init.o*(.toc)
+		__prom_init_toc_end = .;
+#endif
+		*(.toc)
+	}
+
 	SOFT_MASK_TABLE(8)
 	RESTART_TABLE(8)
 
@@ -327,26 +354,11 @@ SECTIONS
 		*(.data.rel*)
 		*(SDATA_MAIN)
 		*(.sdata2)
-		*(.got.plt) *(.got)
-		*(.plt)
-		*(.branch_lt)
 	}
 #else
 	.data : AT(ADDR(.data) - LOAD_OFFSET) {
 		DATA_DATA
 		*(.data.rel*)
-		*(.toc1)
-		*(.branch_lt)
-	}
-
-	.got : AT(ADDR(.got) - LOAD_OFFSET) ALIGN(256) {
-		*(.got)
-#ifndef CONFIG_RELOCATABLE
-		__prom_init_toc_start = .;
-		arch/powerpc/kernel/prom_init.o*(.toc)
-		__prom_init_toc_end = .;
-#endif
-		*(.toc)
 	}
 #endif
 
-- 
2.37.2

[PATCH 5/7] powerpc/build: move .data.rel.ro, .sdata2 to read-only

[Nicholas Piggin]

.sdata2 is a readonly small data section for ppc32, and .data.rel.ro
is data that needs relocating but is read-only after that so these
can both be moved to the read only memory region.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/vmlinux.lds.S | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 716fff86c3fd..44050863032e 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -131,6 +131,16 @@ SECTIONS
 	/* Read-only data */
 	RO_DATA(PAGE_SIZE)
 
+#ifdef CONFIG_PPC32
+	.sdata2 : AT(ADDR(.sdata2) - LOAD_OFFSET) {
+		*(.sdata2)
+	}
+#endif
+
+	.data.rel.ro : AT(ADDR(.data.rel.ro) - LOAD_OFFSET) {
+		*(.data.rel.ro*)
+	}
+
 	.branch_lt : AT(ADDR(.branch_lt) - LOAD_OFFSET) {
 		*(.branch_lt)
 	}
@@ -348,19 +358,13 @@ SECTIONS
 	. = ALIGN(PAGE_SIZE);
 	_sdata = .;
 
-#ifdef CONFIG_PPC32
 	.data : AT(ADDR(.data) - LOAD_OFFSET) {
 		DATA_DATA
 		*(.data.rel*)
+#ifdef CONFIG_PPC32
 		*(SDATA_MAIN)
-		*(.sdata2)
-	}
-#else
-	.data : AT(ADDR(.data) - LOAD_OFFSET) {
-		DATA_DATA
-		*(.data.rel*)
-	}
 #endif
+	}
 
 	/* The initial task and kernel stack */
 	INIT_TASK_DATA_SECTION(THREAD_ALIGN)
-- 
2.37.2

[PATCH 6/7] powerpc/64/build: only include .opd with ELFv1

[Nicholas Piggin]

ELFv2 does not use function descriptors so .opd is not required.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/vmlinux.lds.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 44050863032e..404944263db8 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -181,11 +181,13 @@ SECTIONS
 	SOFT_MASK_TABLE(8)
 	RESTART_TABLE(8)
 
+#ifdef CONFIG_PPC64_ELF_ABI_V1
 	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
 		__start_opd = .;
 		KEEP(*(.opd))
 		__end_opd = .;
 	}
+#endif
 
 	. = ALIGN(8);
 	__stf_entry_barrier_fixup : AT(ADDR(__stf_entry_barrier_fixup) - LOAD_OFFSET) {
-- 
2.37.2

[PATCH 7/7] powerpc/64/build: merge .got and .toc input sections

[Nicholas Piggin]

Follow the binutils ld internal linker script and merge .got and .toc
input sections in the .got output section.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/vmlinux.lds.S | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 404944263db8..f7271bf78150 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -169,13 +169,12 @@ SECTIONS
 	}
 
 	.got : AT(ADDR(.got) - LOAD_OFFSET) ALIGN(256) {
-		*(.got)
+		*(.got .toc)
 #ifndef CONFIG_RELOCATABLE
 		__prom_init_toc_start = .;
 		arch/powerpc/kernel/prom_init.o*(.toc)
 		__prom_init_toc_end = .;
 #endif
-		*(.toc)
 	}
 
 	SOFT_MASK_TABLE(8)
-- 
2.37.2

Re: [PATCH 2/7] powerpc: move __end_rodata to cover arch read-only sections

[Christophe Leroy]

Le 14/09/2022 à 17:47, Nicholas Piggin a écrit :
> powerpc has a number of read-only sections and tables that are put
> after RO_DATA(). Move the __end_rodata symbol to cover these as well.
> 
> Setting memory to read-only at boot is done using __init_begin,
> change that that to use __end_rodata.

I see the idea after looking in more details in the generated code, but 
I think this commit description needs to be more explanatory.

In mm/pgtable_32.c there was a comment explaining why __init_begin is 
used. I think you need to explain why we don't want to use it anymore 
and why we can now use __end_rodata.

> 
> This also affects boot dmesg, is_kernel_rodata(), and some other checks.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/kernel/vmlinux.lds.S        | 3 +++
>   arch/powerpc/mm/book3s32/mmu.c           | 2 +-
>   arch/powerpc/mm/book3s64/hash_pgtable.c  | 2 +-
>   arch/powerpc/mm/book3s64/radix_pgtable.c | 2 +-
>   arch/powerpc/mm/pgtable_32.c             | 5 ++---
>   5 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
> index fe22d940412f..90ac5ff73df2 100644
> --- a/arch/powerpc/kernel/vmlinux.lds.S
> +++ b/arch/powerpc/kernel/vmlinux.lds.S
> @@ -210,6 +210,9 @@ SECTIONS
>   	}
>   #endif
>   
> +	. = ALIGN(PAGE_SIZE);
> +	__end_rodata = .;
> +

I think this will likely break block mapping on PPC32.

It needs to be aligned to STRICT_ALIGN_SIZE, like __init_begin is.


>   /*
>    * Init sections discarded at runtime
>    */
> diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
> index a96b73006dfb..e13b883e4e5b 100644
> --- a/arch/powerpc/mm/book3s32/mmu.c
> +++ b/arch/powerpc/mm/book3s32/mmu.c
> @@ -240,7 +240,7 @@ void mmu_mark_rodata_ro(void)
>   	for (i = 0; i < nb; i++) {
>   		struct ppc_bat *bat = BATS[i];
>   
> -		if (bat_addrs[i].start < (unsigned long)__init_begin)
> +		if (bat_addrs[i].start < (unsigned long)__end_rodata)
>   			bat[1].batl = (bat[1].batl & ~BPP_RW) | BPP_RX;
>   	}
>   
> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c
> index ae008b9df0e6..28332001bd87 100644
> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c
> @@ -541,7 +541,7 @@ void hash__mark_rodata_ro(void)
>   	unsigned long start, end, pp;
>   
>   	start = (unsigned long)_stext;
> -	end = (unsigned long)__init_begin;
> +	end = (unsigned long)__end_rodata;
>   
>   	pp = htab_convert_pte_flags(pgprot_val(PAGE_KERNEL_ROX), HPTE_USE_KERNEL_KEY);
>   
> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> index 698274109c91..2305f34bcc33 100644
> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -228,7 +228,7 @@ void radix__mark_rodata_ro(void)
>   	unsigned long start, end;
>   
>   	start = (unsigned long)_stext;
> -	end = (unsigned long)__init_begin;
> +	end = (unsigned long)__end_rodata;
>   
>   	radix__change_memory_range(start, end, _PAGE_WRITE);
>   }
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> index 3ac73f9fb5d5..112af8c5447a 100644
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -158,10 +158,9 @@ void mark_rodata_ro(void)
>   	}
>   
>   	/*
> -	 * mark .text and .rodata as read only. Use __init_begin rather than
> -	 * __end_rodata to cover NOTES and EXCEPTION_TABLE.
> +	 * mark .text and .rodata as read only.
>   	 */
> -	numpages = PFN_UP((unsigned long)__init_begin) -
> +	numpages = PFN_UP((unsigned long)__end_rodata) -
>   		   PFN_DOWN((unsigned long)_stext);
>   
>   	set_memory_ro((unsigned long)_stext, numpages);

Re: [PATCH 1/7] powerpc/build: put sys_call_table in .data.rel.ro if RELOCATABLE

[Christophe Leroy]

Le 14/09/2022 à 17:47, Nicholas Piggin a écrit :
> Const function pointers live in .data.rel.ro rather than .rodata because
> they must be relocated. This change prevents powerpc/32 from generating
> R_PPC_UADDR32 relocations (which are not handled). The sys_call_table is
> moved to writeable memory, but a later change will move it back.

Aren't you missing commit c7acee3d2f12 ("powerpc: align syscall table 
for ppc32") ?

I can't see any R_PPC_UADDR32 relocations generated by ppc4xx_defconfig 
+ CONFIG_RELOCATABLE unless I revert that commit.



> 
> After this patch, 44x_defconfig + CONFIG_RELOCATABLE boots to busybox.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/kernel/systbl.S | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/systbl.S b/arch/powerpc/kernel/systbl.S
> index cb3358886203..0bec33e86f50 100644
> --- a/arch/powerpc/kernel/systbl.S
> +++ b/arch/powerpc/kernel/systbl.S
> @@ -12,7 +12,11 @@
>   
>   #include <asm/ppc_asm.h>
>   
> +#ifdef CONFIG_RELOCATABLE
> +.section .data.rel.ro,"aw"
> +#else
>   .section .rodata,"a"
> +#endif
>   
>   #ifdef CONFIG_PPC64
>   	.p2align	3

Re: [PATCH 2/7] powerpc: move __end_rodata to cover arch read-only sections

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> powerpc has a number of read-only sections and tables that are put
> after RO_DATA(). Move the __end_rodata symbol to cover these as well.
>
> Setting memory to read-only at boot is done using __init_begin,
> change that that to use __end_rodata.

Did you just do that because it seems logical?

Because it does seem logical, but it leaves a RWX region in the gap
between __end_rodata and __init_begin, which is bad.

This is the current behaviour, on radix:

---[ Start of kernel VM ]---
0xc000000000000000-0xc000000001ffffff  0x0000000000000000        32M         r      X   pte  valid  present        dirty  accessed
0xc000000002000000-0xc00000007fffffff  0x0000000002000000      2016M         r  w       pte  valid  present        dirty  accessed

And with your change:

---[ Start of kernel VM ]---
0xc000000000000000-0xc0000000013fffff  0x0000000000000000        20M         r      X   pte  valid  present        dirty  accessed
0xc000000001400000-0xc000000001ffffff  0x0000000001400000        12M         r  w   X   pte  valid  present        dirty  accessed
0xc000000002000000-0xc00000007fffffff  0x0000000002000000      2016M         r  w       pte  valid  present        dirty  accessed


On radix the 16M alignment is larger than we need, but we need to chose
a value at build time that works for radix and hash.

We could make the code smarter on radix, to mark those pages in between
__end_rodata and __init_begin as RW_ and use them for data. But that
would be a more involved change.

I think if we just drop the changes to the C files this patch is OK to
go in.

cheers

Re: [PATCH 1/7] powerpc/build: put sys_call_table in .data.rel.ro if RELOCATABLE

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 14/09/2022 à 17:47, Nicholas Piggin a écrit :
>> Const function pointers live in .data.rel.ro rather than .rodata because
>> they must be relocated. This change prevents powerpc/32 from generating
>> R_PPC_UADDR32 relocations (which are not handled). The sys_call_table is
>> moved to writeable memory, but a later change will move it back.
>
> Aren't you missing commit c7acee3d2f12 ("powerpc: align syscall table 
> for ppc32") ?

That's in fixes. I'll sort it out when I apply this, or when I merge
fixes into next.

> I can't see any R_PPC_UADDR32 relocations generated by ppc4xx_defconfig 
> + CONFIG_RELOCATABLE unless I revert that commit.

Presumably this change accidentally aligns the syscall table.

>> After this patch, 44x_defconfig + CONFIG_RELOCATABLE boots to busybox.
 
So that's probably just because of the alignment too.

I think this patch should go after .data.rel.ro is made read only.

cheers

Re: [PATCH 2/7] powerpc: move __end_rodata to cover arch read-only sections

[Nicholas Piggin]

On Thu Sep 15, 2022 at 10:47 PM AEST, Michael Ellerman wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> > powerpc has a number of read-only sections and tables that are put
> > after RO_DATA(). Move the __end_rodata symbol to cover these as well.
> >
> > Setting memory to read-only at boot is done using __init_begin,
> > change that that to use __end_rodata.
>
> Did you just do that because it seems logical?

I actually was looking at moving init so runtime code and data is
closer.

> Because it does seem logical, but it leaves a RWX region in the gap
> between __end_rodata and __init_begin, which is bad.
>
> This is the current behaviour, on radix:
>
> ---[ Start of kernel VM ]---
> 0xc000000000000000-0xc000000001ffffff  0x0000000000000000        32M         r      X   pte  valid  present        dirty  accessed
> 0xc000000002000000-0xc00000007fffffff  0x0000000002000000      2016M         r  w       pte  valid  present        dirty  accessed
>
> And with your change:
>
> ---[ Start of kernel VM ]---
> 0xc000000000000000-0xc0000000013fffff  0x0000000000000000        20M         r      X   pte  valid  present        dirty  accessed
> 0xc000000001400000-0xc000000001ffffff  0x0000000001400000        12M         r  w   X   pte  valid  present        dirty  accessed
> 0xc000000002000000-0xc00000007fffffff  0x0000000002000000      2016M         r  w       pte  valid  present        dirty  accessed
>
>
> On radix the 16M alignment is larger than we need, but we need to chose
> a value at build time that works for radix and hash.
>
> We could make the code smarter on radix, to mark those pages in between
> __end_rodata and __init_begin as RW_ and use them for data. But that
> would be a more involved change.

Ah, yes Christophe pointed out it's broken too. We could just align
__end_rodata to STRICT_ALIGN_SIZE for this patch?

Thanks,
Nick

Re: [PATCH 1/7] powerpc/build: put sys_call_table in .data.rel.ro if RELOCATABLE

[Nicholas Piggin]

On Thu Sep 15, 2022 at 10:51 PM AEST, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> > Le 14/09/2022 à 17:47, Nicholas Piggin a écrit :
> >> Const function pointers live in .data.rel.ro rather than .rodata because
> >> they must be relocated. This change prevents powerpc/32 from generating
> >> R_PPC_UADDR32 relocations (which are not handled). The sys_call_table is
> >> moved to writeable memory, but a later change will move it back.
> >
> > Aren't you missing commit c7acee3d2f12 ("powerpc: align syscall table 
> > for ppc32") ?
>
> That's in fixes. I'll sort it out when I apply this, or when I merge
> fixes into next.

Yeah that explains the relocations I was seeing, I should have dug
further into that, so they're really unrelated to this patch.

> > I can't see any R_PPC_UADDR32 relocations generated by ppc4xx_defconfig 
> > + CONFIG_RELOCATABLE unless I revert that commit.
>
> Presumably this change accidentally aligns the syscall table.
>
> >> After this patch, 44x_defconfig + CONFIG_RELOCATABLE boots to busybox.
>  
> So that's probably just because of the alignment too.
>
> I think this patch should go after .data.rel.ro is made read only.

Yeah that should be fine.

Thanks,
Nick

Re: [PATCH 2/7] powerpc: move __end_rodata to cover arch read-only sections

[Michael Ellerman]

"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Thu Sep 15, 2022 at 10:47 PM AEST, Michael Ellerman wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> > powerpc has a number of read-only sections and tables that are put
>> > after RO_DATA(). Move the __end_rodata symbol to cover these as well.
>> >
>> > Setting memory to read-only at boot is done using __init_begin,
>> > change that that to use __end_rodata.
>>
>> Did you just do that because it seems logical?
>
> I actually was looking at moving init so runtime code and data is
> closer.
>
>> Because it does seem logical, but it leaves a RWX region in the gap
>> between __end_rodata and __init_begin, which is bad.
>>
>> This is the current behaviour, on radix:
>>
>> ---[ Start of kernel VM ]---
>> 0xc000000000000000-0xc000000001ffffff  0x0000000000000000        32M         r      X   pte  valid  present        dirty  accessed
>> 0xc000000002000000-0xc00000007fffffff  0x0000000002000000      2016M         r  w       pte  valid  present        dirty  accessed
>>
>> And with your change:
>>
>> ---[ Start of kernel VM ]---
>> 0xc000000000000000-0xc0000000013fffff  0x0000000000000000        20M         r      X   pte  valid  present        dirty  accessed
>> 0xc000000001400000-0xc000000001ffffff  0x0000000001400000        12M         r  w   X   pte  valid  present        dirty  accessed
>> 0xc000000002000000-0xc00000007fffffff  0x0000000002000000      2016M         r  w       pte  valid  present        dirty  accessed
>>
>>
>> On radix the 16M alignment is larger than we need, but we need to chose
>> a value at build time that works for radix and hash.
>>
>> We could make the code smarter on radix, to mark those pages in between
>> __end_rodata and __init_begin as RW_ and use them for data. But that
>> would be a more involved change.
>
> Ah, yes Christophe pointed out it's broken too. We could just align
> __end_rodata to STRICT_ALIGN_SIZE for this patch?

Yeah that should work.

I'd be happier if we had something more explicit to document that
boundary, I'll send a patch.

cheers