arm64: suspend: avoid potential TLB conflict

PATCH New Mark Rutland 2 participants 6 messages Aug 8, 2016 – Aug 11, 2016 arm64

Share & tools

Mark Rutland

author reviewer

2016-08-08 10:10 UTC #0 lore raw

PATCH 1/1 arm64: suspend: avoid potential TLB conflict

In create_safe_exec_page we install a set of global mappings in TTBR0, then subsequently invalidate TLBs. While TTBR0 points at the zero page, and the TLBs should be free of stale global entries, we may have stale ASID-tagged entries (e.g. from the EFI runtime services mappings) for the same VAs. Per the ARM ARM these ASID-tagged entries may conflict with newly-allocated global entries, and we must follow a Break-Before-Make approach to avoid issues resulting from this.

This patch reworks create_safe_exec_page to invalidate TLBs while the zero page is still in place, ensuring that there are no potential conflicts when the new TTBR0 value is installed. As a single CPU is online while this code executes, we do not need to perform broadcast TLB maintenance, and can call local_flush_tlb_all(), which also subsumes some barriers. The remaining assembly is converted to use write_sysreg() and isb().

Other than this, we safely manipulate TTBRs in the hibernate dance. The code we install as part of the new TTBR0 mapping (the hibernated kernel's swsusp_arch_suspend_exit) installs a zero page into TTBR1, invalidates TLBs, then installs its preferred value. Upon being restored to the middle of swsusp_arch_suspend, the new image will call __cpu_suspend_exit, which will call cpu_uninstall_idmap, installing the zero page in TTBR0 and invalidating all TLB entries.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc James Morse <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Will Deacon <[email protected]>

Fixes 82869ac57b5d3b55 ("arm64: kernel: Add support for hibernate/suspend-to-disk")

---
 arch/arm64/kernel/hibernate.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

arch/arm64/kernel/hibernate.c +11 -6

index 21ab5df..c59d3e4 100644
@@ -35,6 +35,7 @@
 #include <asm/sections.h>
 #include <asm/smp.h>
 #include <asm/suspend.h>
+#include <asm/sysreg.h>
 #include <asm/virt.h>
 
 /*

@@ -217,12 +218,16 @@ static int create_safe_exec_page(void *src_start, size_t length,
 	set_pte(pte, __pte(virt_to_phys((void *)dst) |
 			 pgprot_val(PAGE_KERNEL_EXEC)));
 
-	/* Load our new page tables */
-	asm volatile("msr	ttbr0_el1, %0;"
-		     "isb;"
-		     "tlbi	vmalle1is;"
-		     "dsb	ish;"
-		     "isb" : : "r"(virt_to_phys(pgd)));
+	/*
+	 * Load our new page tables. TTBR0 currently points to the zero page,
+	 * and the TLBs should be free of global entries, but may contain stale
+	 * ASID-tagged entries (e.g. from the EFI runtime services). A strict
+	 * BBM approach requires that we destroy these before installing
+	 * overlapping global mappings.
+	 */
+	local_flush_tlb_all();
+	write_sysreg(virt_to_phys(pgd), ttbr0_el1);
+	isb();
 
 	*phys_dst_addr = virt_to_phys((void *)dst);

Signature · 1 line

1.9.1

James Morse

2016-08-09 16:25 UTC #1 lore raw

PATCH 1/1 arm64: suspend: avoid potential TLB conflict

Hi Mark,

~s/suspend/hibernate/ in the subject?

Quoted 38 lines · Mark Rutland

In create_safe_exec_page we install a set of global mappings in TTBR0, then subsequently invalidate TLBs. While TTBR0 points at the zero page, and the TLBs should be free of stale global entries, we may have stale ASID-tagged entries (e.g. from the EFI runtime services mappings) for the same VAs. Per the ARM ARM these ASID-tagged entries may conflict with newly-allocated global entries, and we must follow a Break-Before-Make approach to avoid issues resulting from this.
This patch reworks create_safe_exec_page to invalidate TLBs while the zero page is still in place, ensuring that there are no potential conflicts when the new TTBR0 value is installed. As a single CPU is online while this code executes, we do not need to perform broadcast TLB maintenance, and can call local_flush_tlb_all(), which also subsumes some barriers. The remaining assembly is converted to use write_sysreg() and isb().
Other than this, we safely manipulate TTBRs in the hibernate dance. The code we install as part of the new TTBR0 mapping (the hibernated kernel's swsusp_arch_suspend_exit) installs a zero page into TTBR1, invalidates TLBs, then installs its preferred value. Upon being restored to the middle of swsusp_arch_suspend, the new image will call __cpu_suspend_exit, which will call cpu_uninstall_idmap, installing the zero page in TTBR0 and invalidating all TLB entries.
arch/arm64/kernel/hibernate.c +1 -0
index 21ab5df..c59d3e4 100644
@@ -35,6 +35,7 @@
 #include <asm/sections.h>
 #include <asm/smp.h>
 #include <asm/suspend.h>
+#include <asm/sysreg.h>
 #include <asm/virt.h>
 
 /*
@@ -217,12 +218,16 @@ static int create_safe_exec_page(void *src_start, size_t length,
 	set_pte(pte, __pte(virt_to_phys((void *)dst) |

arch/arm64/kernel/hibernate.c static int create_safe_exec_page(void *src_start, size_t length, · line 222

Quoted 10 lines

arch/arm64/kernel/hibernate.c +2 -6

 			 pgprot_val(PAGE_KERNEL_EXEC)));
 
-	/* Load our new page tables */
-	asm volatile("msr	ttbr0_el1, %0;"
-		     "isb;"
-		     "tlbi	vmalle1is;"
-		     "dsb	ish;"
-		     "isb" : : "r"(virt_to_phys(pgd)));
+	/*
+	 * Load our new page tables. TTBR0 currently points to the zero page,

fe12c00d21bb ("PM / hibernate: Introduce test_resume mode for hibernation") came in with the merge window, this does a suspend followed by a resume with the user page tables still loaded in ttbr0_el1.

So now we need to call cpu_set_reserved_ttbr0() in here to make this true/safe.

arch/arm64/kernel/hibernate.c static int create_safe_exec_page(void *src_start, size_t length, · line 232

Quoted 10 lines

arch/arm64/kernel/hibernate.c +8 -0

+	 * and the TLBs should be free of global entries, but may contain stale
+	 * ASID-tagged entries (e.g. from the EFI runtime services). A strict
+	 * BBM approach requires that we destroy these before installing
+	 * overlapping global mappings.
+	 */
+	local_flush_tlb_all();
+	write_sysreg(virt_to_phys(pgd), ttbr0_el1);
+	isb();
 
 	*phys_dst_addr = virt_to_phys((void *)dst);

and it even looks better!

If you think they're useful:

Tested-by James Morse <[email protected]>

Acked-by James Morse <[email protected]>

Thanks!

James

Mark Rutland

author reviewer

2016-08-09 16:49 UTC #2 lore raw

PATCH 1/1 arm64: suspend: avoid potential TLB conflict

Quoted 3 lines · James Morse

Hi Mark,
~s/suspend/hibernate/ in the subject?

Ah, yes. I'll fix that up.

Quoted 18 lines

arch/arm64/kernel/hibernate.c static int create_safe_exec_page(void *src_start, size_t length, · line 222
Quoted 13 lines
/*
arch/arm64/kernel/hibernate.c +2 -6
@@ -217,12 +218,16 @@ static int create_safe_exec_page(void *src_start, size_t length,
 	set_pte(pte, __pte(virt_to_phys((void *)dst) |
 			 pgprot_val(PAGE_KERNEL_EXEC)));
 
-	/* Load our new page tables */
-	asm volatile("msr	ttbr0_el1, %0;"
-		     "isb;"
-		     "tlbi	vmalle1is;"
-		     "dsb	ish;"
-		     "isb" : : "r"(virt_to_phys(pgd)));
+	/*
+	 * Load our new page tables. TTBR0 currently points to the zero page,
fe12c00d21bb ("PM / hibernate: Introduce test_resume mode for hibernation") came in with the merge window, this does a suspend followed by a resume with the user page tables still loaded in ttbr0_el1.

Quoted 1 line

So now we need to call cpu_set_reserved_ttbr0() in here to make this true/safe.

Thanks for the heads-up!

Quoted 11 lines

From a quick look, that sounds like the right thing to do.

Quoted 9 lines

arch/arm64/kernel/hibernate.c +8 -0

+	 * and the TLBs should be free of global entries, but may contain stale
+	 * ASID-tagged entries (e.g. from the EFI runtime services). A strict
+	 * BBM approach requires that we destroy these before installing
+	 * overlapping global mappings.
+	 */
+	local_flush_tlb_all();
+	write_sysreg(virt_to_phys(pgd), ttbr0_el1);
+	isb();

Quoted 7 lines

arch/arm64/kernel/hibernate.c static int create_safe_exec_page(void *src_start, size_t length, · line 232
Quoted 1 line
arch/arm64/kernel/hibernate.c +0 -0
 	*phys_dst_addr = virt_to_phys((void *)dst);
and it even looks better!
If you think they're useful:
Tested-by James Morse <[email protected]>
Acked-by James Morse <[email protected]>

Cheers!

I assume those apply with the addition of cpu_set_reserved_ttbr0(), which I'll fold into v2 along with them.

Thanks,
Mark.

Mark Rutland

author reviewer

2016-08-09 17:51 UTC #3 lore raw

PATCH 1/1 arm64: suspend: avoid potential TLB conflict

In reply to James Morse

Quoted 4 lines · James Morse

Quoted 4 lines

arch/arm64/kernel/hibernate.c +0 -0

@@ -217,12 +218,16 @@ static int create_safe_exec_page(void *src_start, size_t length,
 	set_pte(pte, __pte(virt_to_phys((void *)dst) |
 			 pgprot_val(PAGE_KERNEL_EXEC)));

Quoted 12 lines

arch/arm64/kernel/hibernate.c static int create_safe_exec_page(void *src_start, size_t length, · line 222
Quoted 8 lines
arch/arm64/kernel/hibernate.c +2 -6
-	/* Load our new page tables */
-	asm volatile("msr	ttbr0_el1, %0;"
-		     "isb;"
-		     "tlbi	vmalle1is;"
-		     "dsb	ish;"
-		     "isb" : : "r"(virt_to_phys(pgd)));
+	/*
+	 * Load our new page tables. TTBR0 currently points to the zero page,
fe12c00d21bb ("PM / hibernate: Introduce test_resume mode for hibernation") came in with the merge window, this does a suspend followed by a resume with the user page tables still loaded in ttbr0_el1.

Hmmm... given that, it looks like if we bail out in swsusp_arch_resume() after the call to create_safe_exec_page(), we may return to userspace with a corrupted TTBR0.

We probably need to defer the call to create_safe_exec_page() after the other potential failure sites so as to avoid that.

Looking around it's not clear to me how/where the get_safe_page() allocations are cleaned up when a failure occurs.

Thanks,
Mark.

James Morse

2016-08-10 09:39 UTC #4 lore raw

PATCH 1/1 arm64: suspend: avoid potential TLB conflict

Hi Mark,

Quoted 18 lines · Mark Rutland

Quoted 4 lines · James Morse
Quoted 4 lines
arch/arm64/kernel/hibernate.c +0 -0
@@ -217,12 +218,16 @@ static int create_safe_exec_page(void *src_start, size_t length,
 	set_pte(pte, __pte(virt_to_phys((void *)dst) |
 			 pgprot_val(PAGE_KERNEL_EXEC)));
 
Quoted 12 lines
arch/arm64/kernel/hibernate.c static int create_safe_exec_page(void *src_start, size_t length, · line 222
Quoted 8 lines
arch/arm64/kernel/hibernate.c +2 -6
-	/* Load our new page tables */
-	asm volatile("msr	ttbr0_el1, %0;"
-		     "isb;"
-		     "tlbi	vmalle1is;"
-		     "dsb	ish;"
-		     "isb" : : "r"(virt_to_phys(pgd)));
+	/*
+	 * Load our new page tables. TTBR0 currently points to the zero page,
fe12c00d21bb ("PM / hibernate: Introduce test_resume mode for hibernation") came in with the merge window, this does a suspend followed by a resume with the user page tables still loaded in ttbr0_el1.

Quoted 3 lines

Hmmm... given that, it looks like if we bail out in swsusp_arch_resume() after the call to create_safe_exec_page(), we may return to userspace with a corrupted TTBR0.

Ah, didn't spot that.

Quoted 5 lines

We probably need to defer the call to create_safe_exec_page() after the other potential failure sites so as to avoid that.
Looking around it's not clear to me how/where the get_safe_page() allocations are cleaned up when a failure occurs.

Its dealt with by the core code: they get added to to one of kernel/power/snapshot.c's plethora of bitmaps, and freed via free_basic_memory_bitmaps() -> memory_bm_free() -> free_list_of_pages() -> free_image_page().

It looks like pages allocated by get_safe_page() are on the 'forbidden_pages_map'.

Thanks,

James

Mark Rutland

author reviewer

2016-08-11 11:01 UTC #5 lore raw

PATCH 1/1 arm64: suspend: avoid potential TLB conflict

Quoted 20 lines · James Morse

Hi Mark,
Quoted 8 lines · Mark Rutland
Quoted 3 lines · James Morse
fe12c00d21bb ("PM / hibernate: Introduce test_resume mode for hibernation") came in with the merge window, this does a suspend followed by a resume with the user page tables still loaded in ttbr0_el1.
Hmmm... given that, it looks like if we bail out in swsusp_arch_resume() after the call to create_safe_exec_page(), we may return to userspace with a corrupted TTBR0.
Ah, didn't spot that.
Quoted 5 lines
We probably need to defer the call to create_safe_exec_page() after the other potential failure sites so as to avoid that.
Looking around it's not clear to me how/where the get_safe_page() allocations are cleaned up when a failure occurs.

Quoted 6 lines

Its dealt with by the core code: they get added to to one of kernel/power/snapshot.c's plethora of bitmaps, and freed via free_basic_memory_bitmaps() -> memory_bm_free() -> free_list_of_pages() -> free_image_page().
It looks like pages allocated by get_safe_page() are on the 'forbidden_pages_map'.

Thanks for the pointer!

Given that, it looks like a simple reshuffling of swsusp_arch_resume is all that's necessary. I'll spin that along with v2 of this patch.

Thanks,
Mark.