More PCID fixes

PATCH In review Andy Lutomirski 4 participants 9 messages Sep 8, 2017 – Sep 13, 2017

Subsystems (3)

x86 x86 MM Hibernation

lore

Share & tools

lore

Andy Lutomirski

author maintainer

2017-09-08 05:07 UTC #0 lore raw

cover More PCID fixes

This gets rid of the problematic VM_BUG_ON and fixes hibernation.

Andy Lutomirski (2):
x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

 arch/x86/mm/tlb.c             | 22 +++++++++++++++++++++-
 arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++-
 2 files changed, 41 insertions(+), 2 deletions(-)

Signature · 1 line

2.13.5

Andy Lutomirski

author maintainer

2017-09-08 05:07 UTC #1 lore raw

PATCH 1/2 x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()

If we hit the VM_BUG_ON(), we're detecting a genuinely bad situation, but we're very unlikely to get a useful call trace.

Make it a warning instead.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/mm/tlb.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

arch/x86/mm/tlb.c +21 -1

index dbbcfd59726a..f4e471dd1526 100644
@@ -121,8 +121,28 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 	 * hypothetical buggy code that directly switches to swapper_pg_dir
 	 * without going through leave_mm() / switch_mm_irqs_off() or that
 	 * does something like write_cr3(read_cr3_pa()).
+	 *
+	 * Only do this check if CONFIG_DEBUG_VM=y because __read_cr3()
+	 * isn't free.
 	 */
-	VM_BUG_ON(__read_cr3() != (__sme_pa(real_prev->pgd) | prev_asid));
+#ifdef CONFIG_DEBUG_VM
+	if (WARN_ON_ONCE(__read_cr3() !=
+			 (__sme_pa(real_prev->pgd) | prev_asid))) {
+		/*
+		 * If we were to BUG here, we'd be very likely to kill
+		 * the system so hard that we don't see the call trace.
+		 * Try to recover instead by ignoring the error and doing
+		 * a global flush to minimize the change of corruption.
+		 *
+		 * (This is far from being a fully correct recovery.
+		 *  Architecturally, the CPU could prefetch something
+		 *  back into an incorrect ASID slot and leave it there
+		 *  to cause trouble down the road.  It's better than
+		 *  nothing, though.)
+		 */
+		__flush_tlb_all();
+	}
+#endif
 
 	if (real_prev == next) {
 		VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=

Signature · 1 line

2.13.5

Andy Lutomirski

author maintainer

2017-09-08 05:09 UTC #2 lore raw

Re: [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()

Quoted 2 lines · Andy Lutomirski

If we hit the VM_BUG_ON(), we're detecting a genuinely bad situation, but we're very unlikely to get a useful call trace.

Sigh, typo below. Ingo, if you apply this version, can you fix it?

Quoted 1 line

+ * a global flush to minimize the change of corruption.

chance of corruption.

tip-bot for Andy Lutomirski

merge bot

2017-09-13 08:59 UTC #3 lore raw

[tip:x86/urgent] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()

In reply to Andy Lutomirski

x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()

Commit-ID:  a376e7f99be7c1e15b2d986e49b2bec834904381
Gitweb:     http://git.kernel.org/tip/a376e7f99be7c1e15b2d986e49b2bec834904381
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Thu, 7 Sep 2017 22:06:57 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Sep 2017 09:50:52 +0200

x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off()

If we hit the VM_BUG_ON(), we're detecting a genuinely bad situation, but we're very unlikely to get a useful call trace.

Make it a warning instead.

Signed-off-by Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Jiri Kosina <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/3b4e06bbb382ca54a93218407c93925ff5871546.1504847163.git.luto@kernel.org open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/mm/tlb.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

arch/x86/mm/tlb.c +21 -1

index 37689a7..1ab3821 100644
@@ -121,8 +121,28 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 	 * hypothetical buggy code that directly switches to swapper_pg_dir
 	 * without going through leave_mm() / switch_mm_irqs_off() or that
 	 * does something like write_cr3(read_cr3_pa()).
+	 *
+	 * Only do this check if CONFIG_DEBUG_VM=y because __read_cr3()
+	 * isn't free.
 	 */
-	VM_BUG_ON(__read_cr3() != (__sme_pa(real_prev->pgd) | prev_asid));
+#ifdef CONFIG_DEBUG_VM
+	if (WARN_ON_ONCE(__read_cr3() !=
+			 (__sme_pa(real_prev->pgd) | prev_asid))) {
+		/*
+		 * If we were to BUG here, we'd be very likely to kill
+		 * the system so hard that we don't see the call trace.
+		 * Try to recover instead by ignoring the error and doing
+		 * a global flush to minimize the chance of corruption.
+		 *
+		 * (This is far from being a fully correct recovery.
+		 *  Architecturally, the CPU could prefetch something
+		 *  back into an incorrect ASID slot and leave it there
+		 *  to cause trouble down the road.  It's better than
+		 *  nothing, though.)
+		 */
+		__flush_tlb_all();
+	}
+#endif
 
 	if (real_prev == next) {
 		VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=

Andy Lutomirski

author maintainer

2017-09-08 05:07 UTC #4 lore raw

PATCH 2/2 x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

In reply to Andy Lutomirski

Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!

FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.

Reported-by Jiri Kosina <[email protected]>

Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

arch/x86/power/hibernate_64.c +20 -1

index f2598d81cd55..f910c514438f 100644
@@ -295,7 +295,26 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
 		return -EOVERFLOW;
 	rdr->jump_address = (unsigned long)restore_registers;
 	rdr->jump_address_phys = __pa_symbol(restore_registers);
-	rdr->cr3 = restore_cr3;
+
+	/*
+	 * The restore code fixes up CR3 and CR4 in the following sequence:
+	 *
+	 * [in hibernation asm]
+	 * 1. CR3 <= temporary page tables
+	 * 2. CR4 <= mmu_cr4_features (from the kernel that restores us)
+	 * 3. CR3 <= rdr->cr3
+	 * 4. CR4 <= mmu_cr4_features (from us, i.e. the image kernel)
+	 * [in restore_processor_state()]
+	 * 5. CR4 <= saved CR4
+	 * 6. CR3 <= saved CR3
+	 *
+	 * Our mmu_cr4_features has CR4.PCIDE=0, and toggling
+	 * CR4.PCIDE while CR3's PCID bits are nonzero is illegal, so
+	 * rdr->cr3 needs to point to valid page tables but must not
+	 * have any of the PCID bits set.
+	 */
+	rdr->cr3 = restore_cr3 & ~CR3_PCID_MASK;
+
 	rdr->magic = RESTORE_MAGIC;
 
 	hibernation_e820_save(rdr->e820_digest);

Signature · 1 line

2.13.5

Jiri Kosina

Tested-by on v1 2/2

2017-09-08 07:59 UTC #5 lore raw

Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

Quoted 11 lines · Andy Lutomirski

Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!

Quoted 2 lines

FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.

I've just performed 15 hibernation cycles with current Linus' tree (5969d1bb3082) with these two patches applied on top of it, and I haven't encountered any issue (and the warning in switch_mm_irqs_off() didn't trigger either).

Quoted 3 lines

Reported-by Jiri Kosina <[email protected]>
Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
Signed-off-by Andy Lutomirski <[email protected]>

Tested-by Jiri Kosina <[email protected]>

Thanks!

Signature · 2 lines

Jiri Kosina
SUSE Labs

Andy Lutomirski

author maintainer

2017-09-10 19:17 UTC #6 lore raw

Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

Quoted 21 lines · Jiri Kosina

Quoted 11 lines · Andy Lutomirski
Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!
Quoted 2 lines
FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.
I've just performed 15 hibernation cycles with current Linus' tree (5969d1bb3082) with these two patches applied on top of it, and I haven't encountered any issue (and the warning in switch_mm_irqs_off() didn't trigger either).

Quoted 6 lines

Quoted 3 lines
Reported-by Jiri Kosina <[email protected]>
Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
Signed-off-by Andy Lutomirski <[email protected]>
Tested-by Jiri Kosina <[email protected]>

Ingo, please do *not* apply this patch yet. The code is fine, but the comment is about to become wrong. I just found a nasty initialization order issue, and I need to rework a bunch of the way we deal with PCIDE.

Ingo Molnar

2017-09-11 05:48 UTC #7 lore raw

Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

Quoted 26 lines · Andy Lutomirski

Quoted 25 lines · Jiri Kosina
Quoted 11 lines · Andy Lutomirski
Jiri reported a resume-from-hibernation failure triggered by PCID. The root cause appears to be rather odd. The hibernation asm restores a CR3 value that comes from the image header. If the image kernel has PCID on, it's entirely reasonable for this CR3 value to have one of the low 12 bits set. The restore code restores it with CR4.PCIDE=0, which means that those low 12 bits are accepted by the CPU but are either ignored or interpreted as a caching mode. This is odd, but still works. We blow up later when the image kernel restores CR4, though, since changing CR4.PCIDE with CR3[11:0] != 0 is illegal. Boom!
Quoted 2 lines
FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.
I've just performed 15 hibernation cycles with current Linus' tree (5969d1bb3082) with these two patches applied on top of it, and I haven't encountered any issue (and the warning in switch_mm_irqs_off() didn't trigger either).
Quoted 3 lines
Reported-by Jiri Kosina <[email protected]>
Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
Signed-off-by Andy Lutomirski <[email protected]>

Quoted 7 lines

Quoted 2 lines
Tested-by Jiri Kosina <[email protected]>
Ingo, please do *not* apply this patch yet. The code is fine, but the comment is about to become wrong. I just found a nasty initialization order issue, and I need to rework a bunch of the way we deal with PCIDE.

Ok, I'll delay everything PCID delayed - once you've gathered it all together please send a full series against Linus's latest collecting all the fixes/cleanups.

If you find unexpected complications then there will be a point in time where it might be better to just disable PCID for this release and re-try in v4.15. As the number and complexity of fixes increases so does the risk that we'll introduce some last-minute regression.

Thanks,

Ingo

tip-bot for Andy Lutomirski

merge bot

2017-09-13 08:59 UTC #8 lore raw

[tip:x86/urgent] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

In reply to Andy Lutomirski

x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

Commit-ID:  f34902c5c6c08024371202a680ce69f2d488776d
Gitweb:     http://git.kernel.org/tip/f34902c5c6c08024371202a680ce69f2d488776d
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Thu, 7 Sep 2017 22:06:58 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Sep 2017 09:52:37 +0200

x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

FWIW, it's entirely unclear to me what's supposed to happen if a PAE kernel restores a non-PAE image or vice versa. Ditto for LA57.

Reported-by Jiri Kosina <[email protected]>

Tested-by Jiri Kosina <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Fixes 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")

Link http://lkml.kernel.org/r/18ca57090651a6341e97083883f9e814c4f14684.1504847163.git.luto@kernel.org open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

arch/x86/power/hibernate_64.c +20 -1

index f2598d8..f910c51 100644
@@ -295,7 +295,26 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
 		return -EOVERFLOW;
 	rdr->jump_address = (unsigned long)restore_registers;
 	rdr->jump_address_phys = __pa_symbol(restore_registers);
-	rdr->cr3 = restore_cr3;
+
+	/*
+	 * The restore code fixes up CR3 and CR4 in the following sequence:
+	 *
+	 * [in hibernation asm]
+	 * 1. CR3 <= temporary page tables
+	 * 2. CR4 <= mmu_cr4_features (from the kernel that restores us)
+	 * 3. CR3 <= rdr->cr3
+	 * 4. CR4 <= mmu_cr4_features (from us, i.e. the image kernel)
+	 * [in restore_processor_state()]
+	 * 5. CR4 <= saved CR4
+	 * 6. CR3 <= saved CR3
+	 *
+	 * Our mmu_cr4_features has CR4.PCIDE=0, and toggling
+	 * CR4.PCIDE while CR3's PCID bits are nonzero is illegal, so
+	 * rdr->cr3 needs to point to valid page tables but must not
+	 * have any of the PCID bits set.
+	 */
+	rdr->cr3 = restore_cr3 & ~CR3_PCID_MASK;
+
 	rdr->magic = RESTORE_MAGIC;
 
 	hibernation_e820_save(rdr->e820_digest);