powerpc/strict_rwx: quirks for STRICT_RWX patches
PATCH 1/2 powerpc/strict_rwx: quirks for STRICT_RWX patches
This patch disables STRICT_RWX for power9 DD1 machines where due to some limitations with the way we do tlb updates, we clear the TLB entry of the text that's doing the update to kernel text and that does lead to a crash.
--- arch/powerpc/mm/pgtable-radix.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
arch/powerpc/mm/pgtable-radix.c +12 -0
@@ -169,6 +169,18 @@ void radix__mark_rodata_ro(void) {unsigned long start, end; +/* +* mark_rodata_ro() will mark itself as !writable at some point +* due to workaround in radix__pte_update(), we'll end up with +* an invalid pte and the system will crash quite severly. +* The alternatives are quite cumbersome and leaving out +* the page containing the flush code is not very nice. +*/ +if (cpu_has_feature (CPU_FTR_POWER9_DD1 )) { +pr_warn ("Warning: Unable to mark rodata read only on P9 DD1\n" ); +return ; + } + start = (unsigned long )_stext; end = (unsigned long )__init_begin;
Signature · 1 line
2.9.5
PATCH 2/2 powerpc/strict_rwx: fixup region splitting
We were aggressive with splitting regions and missed the case when _stext and __init_begin completely overlap addr and addr+mapping.
This patch fixes that case and allows us to keep the largest possible mapping through the overlap. The patch also simplies the check into a function
--- arch/powerpc/mm/pgtable-radix.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
arch/powerpc/mm/pgtable-radix.c +28 -11
@@ -210,17 +210,36 @@ static inline void __meminit print_mapping(unsigned long start,pr_info ("Mapped 0x%016lx-0x%016lx with %s pages\n" , start, end, buf); } +static inline int __meminit +should_split_mapping_size (unsigned long addr,unsigned long mapping_size) +{ +#ifdef CONFIG_STRICT_KERNEL_RWX +/* +* If addr, addr+mapping overlap the text region and +* _stext and __init_begin end up lying between +* addr, addr+mapping split the mapping size down +* further +*/ +if ((addr <__pa_symbol (__init_begin)) && + (addr + mapping_size) >__pa_symbol (_stext)) { + +if (((addr + mapping_size) <=__pa_symbol (__init_begin)) && + (addr >=__pa_symbol (_stext))) +return 0 ; + +return 1 ; + } +#endif +return 0 ; +} + +static int __meminit create_physical_mapping (unsigned long start,unsigned long end) {unsigned long vaddr, addr, mapping_size =0 ;pgprot_t prot;unsigned long max_mapping_size; -#ifdef CONFIG_STRICT_KERNEL_RWX -int split_text_mapping =1 ; -#else -int split_text_mapping =0 ; -#endif start =_ALIGN_UP (start,PAGE_SIZE );for (addr = start; addr < end; addr += mapping_size) {
@@ -242,16 +261,14 @@ static int __meminit create_physical_mapping(unsigned long start,else mapping_size =PAGE_SIZE ; -if (split_text_mapping && (mapping_size ==PUD_SIZE ) && - (addr <=__pa_symbol (__init_begin)) && - (addr + mapping_size) >=__pa_symbol (_stext)) { +if ((mapping_size ==PUD_SIZE ) && +should_split_mapping_size (addr, mapping_size)) { max_mapping_size =PMD_SIZE ; goto retry; } -if (split_text_mapping && (mapping_size ==PMD_SIZE ) && - (addr <=__pa_symbol (__init_begin)) && - (addr + mapping_size) >=__pa_symbol (_stext)) +if ((mapping_size ==PMD_SIZE ) && +should_split_mapping_size (addr, mapping_size)) mapping_size =PAGE_SIZE ;if (mapping_size != previous_size) {
Signature · 1 line
2.9.5
Michael Ellerman
reviewerRe: [PATCH 2/2] powerpc/strict_rwx: fixup region splitting
Quoted 6 lines · Balbir Singh
We were aggressive with splitting regions and missed the case when
_stextand__init_begincompletely overlap addr and addr+mapping.This patch fixes that case and allows us to keep the largest possible mapping through the overlap. The patch also simplies the check into a function
Please do the fix in one patch, which we can backport if necessary, and then move it into a function in a separate patch.
cheers
Re: [PATCH 2/2] powerpc/strict_rwx: fixup region splitting
Quoted 5 lines · On Wed, 04 Oct 2017 22:14:17 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 3 lines · Balbir Singh
We were aggressive with splitting regions and missed the case when
_stextand__init_begincompletely overlap addr and addr+mapping.
Quoted 7 lines
Quoted 3 lines
This patch fixes that case and allows us to keep the largest possible mapping through the overlap. The patch also simplies the check into a function
Please do the fix in one patch, which we can backport if necessary, and then move it into a function in a separate patch.
I've tried for a while now, although this looks like its touching more code, this is more elegant. Anything else involves changing the two if conditions almost identically.
Balbir Singh.
Michael Ellerman
reviewerHi Balbir,
Mainly I think we need to add a check in mark_initmem_nx() too don't we? Or should we put it somewhere common to both?
But seeing as I'm replying here are some more comments.
Quoted 1 line
Subject: [PATCH 1/2] powerpc/strict_rwx: quirks for
STRICT_RWXpatches
This misses three key things, 1) it's a bug fix 2) it's for Power9 DD1, and 3) it only applies to radix.
So how about:
powerpc/64s: Fix crashes on Power9 DD1 with radix MMU and STRICT_RWX
Quoted 5 lines · Balbir Singh
This patch disables
STRICT_RWXfor power9 DD1 machines where due to some limitations with the way we do tlb updates, we clear the TLB entry of the text that's doing the update to kernel text and that does lead to a crash.
This mostly describes the patch, but I would prefer more detail, eg:
When using the radix MMU on Power9 DD1, to work around a hardware problem, radix__pte_update() is required to do a two stage update of the PTE. First we write a zero value into the PTE, then we flush the TLB, and then we write the new PTE value.
In the normal case that works OK, but it does not work if we're updating the PTE that maps the code we're executing, because the mapping is removed by the TLB flush and we can no longer execute from it. Unfortunately the STRICT_RWX code needs to do exactly that.
The exact symptoms when we hit this case vary, sometimes we print an oops and then get stuck after that, but I've also seen a machine just get stuck continually page faulting with no oops printed. The variance is presumably due to the exact layout of the text and the page size used for the mappings. In all cases we are unable to boot to a shell.
There are possible solutions such as creating a second mapping of the TLB flush code, executing from that, and then jumping back to the original. However we don't want to add that level of complexity for a DD1 work around.
So just detect that we're running on Power9 DD1 and refrain from changing the permissions, effectively disabling STRICT_RWX on Power9 DD1.
arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 173
Quoted 10 lines
arch/powerpc/mm/pgtable-radix.c +2
-0@@ -169,6 +169,18 @@ void radix__mark_rodata_ro(void) {unsigned long start, end; +/* +* mark_rodata_ro() will mark itself as !writable at some point
^
.arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 174
Quoted 1 line
arch/powerpc/mm/pgtable-radix.c +1
-0+* due to workaround in radix__pte_update(), we'll end up with
^ ^
D the DD1
arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 177
Quoted 4 lines
arch/powerpc/mm/pgtable-radix.c +3
-0+* an invalid pte and the system will crash quite severly. +* The alternatives are quite cumbersome and leaving out +* the page containing the flush code is not very nice.
Just drop the last sentence, it doesn't have enough detail to be useful and we chose not to implement those alternatives, so better not to confuse the reader by talking about them.
arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 182
Quoted 5 lines
arch/powerpc/mm/pgtable-radix.c +5
-0+*/ +if (cpu_has_feature (CPU_FTR_POWER9_DD1 )) { +pr_warn ("Warning: Unable to mark rodata read only on P9 DD1\n" ); +return ; + }
Don't we also need to do the check in radix__mark_initmem_nx() ?
cheers
Quoted 42 lines · On Wed, 04 Oct 2017 20:04:52 +1100 Michael Ellerman <[email protected]> wrote:
Hi Balbir,
Mainly I think we need to add a check in
mark_initmem_nx()too don't we? Or should we put it somewhere common to both?But seeing as I'm replying here are some more comments.
Quoted 1 line
Subject: [PATCH 1/2] powerpc/strict_rwx: quirks for
STRICT_RWXpatchesThis misses three key things, 1) it's a bug fix 2) it's for Power9 DD1, and 3) it only applies to radix.
So how about:
powerpc/64s: Fix crashes on Power9 DD1 with radix MMU and
STRICT_RWXQuoted 5 lines · Balbir Singh
This patch disables
STRICT_RWXfor power9 DD1 machines where due to some limitations with the way we do tlb updates, we clear the TLB entry of the text that's doing the update to kernel text and that does lead to a crash.This mostly describes the patch, but I would prefer more detail, eg:
When using the radix MMU on Power9 DD1, to work around a hardware problem,
radix__pte_update()is required to do a two stage update of the PTE. First we write a zero value into the PTE, then we flush the TLB, and then we write the new PTE value.In the normal case that works OK, but it does not work if we're updating the PTE that maps the code we're executing, because the mapping is removed by the TLB flush and we can no longer execute from it. Unfortunately the
STRICT_RWXcode needs to do exactly that.The exact symptoms when we hit this case vary, sometimes we print an oops and then get stuck after that, but I've also seen a machine just get stuck continually page faulting with no oops printed. The variance is presumably due to the exact layout of the text and the page size used for the mappings. In all cases we are unable to boot to a shell.
Quoted 9 lines
There are possible solutions such as creating a second mapping of the TLB flush code, executing from that, and then jumping back to the original. However we don't want to add that level of complexity for a DD1 work around.
So just detect that we're running on Power9 DD1 and refrain from changing the permissions, effectively disabling
STRICT_RWXon Power9 DD1.
I can copy this verbatim
Quoted 16 lines
arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 173
Quoted 11 lines
arch/powerpc/mm/pgtable-radix.c +2
-0@@ -169,6 +169,18 @@ void radix__mark_rodata_ro(void) {unsigned long start, end; +/* +* mark_rodata_ro() will mark itself as !writable at some point ^ .arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 174
Quoted 1 line
arch/powerpc/mm/pgtable-radix.c +1
-0+* due to workaround in radix__pte_update(), we'll end up with ^ ^
D the DD1
Quoted 9 lines
arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 177
Quoted 4 lines
arch/powerpc/mm/pgtable-radix.c +3
-0+* an invalid pte and the system will crash quite severly. +* The alternatives are quite cumbersome and leaving out +* the page containing the flush code is not very nice. Just drop the last sentence, it doesn't have enough detail to be useful and we chose not to implement those alternatives, so better not to confuse the reader by talking about them.
Can and will do
Quoted 7 lines
arch/powerpc/mm/pgtable-radix.c void radix__mark_rodata_ro(void) · line 182
Quoted 5 lines
arch/powerpc/mm/pgtable-radix.c +5
-0+*/ +if (cpu_has_feature (CPU_FTR_POWER9_DD1 )) { +pr_warn ("Warning: Unable to mark rodata read only on P9 DD1\n" ); +return ; + }Don't we also need to do the check in
radix__mark_initmem_nx()?
No.. not for NX, since we mark NX pages (.data and .init.text..init.end) from .text. We don't change permissions for anything in .text in that routine.
Quoted 2 lines
cheers