[PATCH 0/2] More PCID fixes

[PATCH 0/2] More PCID fixes

[Andy Lutomirski]

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(-)

-- 
2.13.5

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

[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.

Make it a warning instead.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/tlb.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index dbbcfd59726a..f4e471dd1526 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -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) !=
-- 
2.13.5

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

[Andy Lutomirski]

On Thu, Sep 7, 2017 at 10:06 PM, Andy Lutomirski <luto@kernel.org> wrote:
> 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?

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

chance of corruption.

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

[tip-bot for Andy Lutomirski]

Commit-ID:  a376e7f99be7c1e15b2d986e49b2bec834904381
Gitweb:     http://git.kernel.org/tip/a376e7f99be7c1e15b2d986e49b2bec834904381
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 7 Sep 2017 22:06:57 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
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 <luto@kernel.org>
Cc: Borislav Petkov <bpetkov@suse.de>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/3b4e06bbb382ca54a93218407c93925ff5871546.1504847163.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/mm/tlb.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 37689a7..1ab3821 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -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) !=

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

[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 <jikos@kernel.org>
Fixes: 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/x86/power/hibernate_64.c b/arch/x86/power/hibernate_64.c
index f2598d81cd55..f910c514438f 100644
--- a/arch/x86/power/hibernate_64.c
+++ b/arch/x86/power/hibernate_64.c
@@ -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);
-- 
2.13.5

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

[Jiri Kosina]

On Thu, 7 Sep 2017, Andy Lutomirski wrote:

> 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.

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).

> Reported-by: Jiri Kosina <jikos@kernel.org>
> Fixes: 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

	Tested-by: Jiri Kosina <jkosina@suse.cz>

Thanks!

-- 
Jiri Kosina
SUSE Labs

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

[Andy Lutomirski]

On Fri, Sep 8, 2017 at 12:59 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Thu, 7 Sep 2017, Andy Lutomirski wrote:
>
>> 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.
>
> 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).
>
>> Reported-by: Jiri Kosina <jikos@kernel.org>
>> Fixes: 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>
>         Tested-by: Jiri Kosina <jkosina@suse.cz>
>

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.

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

[Ingo Molnar]

* Andy Lutomirski <luto@kernel.org> wrote:

> On Fri, Sep 8, 2017 at 12:59 AM, Jiri Kosina <jikos@kernel.org> wrote:
> > On Thu, 7 Sep 2017, Andy Lutomirski wrote:
> >
> >> 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.
> >
> > 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).
> >
> >> Reported-by: Jiri Kosina <jikos@kernel.org>
> >> Fixes: 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >
> >         Tested-by: Jiri Kosina <jkosina@suse.cz>
> >
> 
> 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:x86/urgent] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3

[tip-bot for Andy Lutomirski]

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

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

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 <jikos@kernel.org>
Tested-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bpetkov@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 660da7c9228f ("x86/mm: Enable CR4.PCIDE on supported systems")
Link: http://lkml.kernel.org/r/18ca57090651a6341e97083883f9e814c4f14684.1504847163.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/power/hibernate_64.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/x86/power/hibernate_64.c b/arch/x86/power/hibernate_64.c
index f2598d8..f910c51 100644
--- a/arch/x86/power/hibernate_64.c
+++ b/arch/x86/power/hibernate_64.c
@@ -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);