* [PATCH 0/2] More PCID fixes @ 2017-09-08 5:06 Andy Lutomirski 2017-09-08 5:06 ` [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() Andy Lutomirski 2017-09-08 5:06 ` [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 Andy Lutomirski 0 siblings, 2 replies; 9+ messages in thread From: Andy Lutomirski @ 2017-09-08 5:06 UTC (permalink / raw) To: X86 ML Cc: Borislav Petkov, linux-kernel@vger.kernel.org, Linus Torvalds, Jiri Kosina, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() 2017-09-08 5:06 [PATCH 0/2] More PCID fixes Andy Lutomirski @ 2017-09-08 5:06 ` Andy Lutomirski 2017-09-08 5:09 ` Andy Lutomirski 2017-09-13 8:55 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski 2017-09-08 5:06 ` [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 Andy Lutomirski 1 sibling, 2 replies; 9+ messages in thread From: Andy Lutomirski @ 2017-09-08 5:06 UTC (permalink / raw) To: X86 ML Cc: Borislav Petkov, linux-kernel@vger.kernel.org, Linus Torvalds, Jiri Kosina, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() 2017-09-08 5:06 ` [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() Andy Lutomirski @ 2017-09-08 5:09 ` Andy Lutomirski 2017-09-13 8:55 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski 1 sibling, 0 replies; 9+ messages in thread From: Andy Lutomirski @ 2017-09-08 5:09 UTC (permalink / raw) To: Andy Lutomirski Cc: X86 ML, Borislav Petkov, linux-kernel@vger.kernel.org, Linus Torvalds, Jiri Kosina 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip:x86/urgent] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() 2017-09-08 5:06 ` [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() Andy Lutomirski 2017-09-08 5:09 ` Andy Lutomirski @ 2017-09-13 8:55 ` tip-bot for Andy Lutomirski 1 sibling, 0 replies; 9+ messages in thread From: tip-bot for Andy Lutomirski @ 2017-09-13 8:55 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, linux-kernel, bpetkov, torvalds, peterz, tglx, jikos, luto, hpa 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) != ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 2017-09-08 5:06 [PATCH 0/2] More PCID fixes Andy Lutomirski 2017-09-08 5:06 ` [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() Andy Lutomirski @ 2017-09-08 5:06 ` Andy Lutomirski 2017-09-08 7:59 ` Jiri Kosina 2017-09-13 8:55 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski 1 sibling, 2 replies; 9+ messages in thread From: Andy Lutomirski @ 2017-09-08 5:06 UTC (permalink / raw) To: X86 ML Cc: Borislav Petkov, linux-kernel@vger.kernel.org, Linus Torvalds, Jiri Kosina, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 2017-09-08 5:06 ` [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 Andy Lutomirski @ 2017-09-08 7:59 ` Jiri Kosina 2017-09-10 19:17 ` Andy Lutomirski 2017-09-13 8:55 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski 1 sibling, 1 reply; 9+ messages in thread From: Jiri Kosina @ 2017-09-08 7:59 UTC (permalink / raw) To: Andy Lutomirski Cc: X86 ML, Borislav Petkov, linux-kernel@vger.kernel.org, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 2017-09-08 7:59 ` Jiri Kosina @ 2017-09-10 19:17 ` Andy Lutomirski 2017-09-11 5:48 ` Ingo Molnar 0 siblings, 1 reply; 9+ messages in thread From: Andy Lutomirski @ 2017-09-10 19:17 UTC (permalink / raw) To: Jiri Kosina Cc: Andy Lutomirski, X86 ML, Borislav Petkov, linux-kernel@vger.kernel.org, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 2017-09-10 19:17 ` Andy Lutomirski @ 2017-09-11 5:48 ` Ingo Molnar 0 siblings, 0 replies; 9+ messages in thread From: Ingo Molnar @ 2017-09-11 5:48 UTC (permalink / raw) To: Andy Lutomirski Cc: Jiri Kosina, X86 ML, Borislav Petkov, linux-kernel@vger.kernel.org, Linus Torvalds * 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip:x86/urgent] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 2017-09-08 5:06 ` [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 Andy Lutomirski 2017-09-08 7:59 ` Jiri Kosina @ 2017-09-13 8:55 ` tip-bot for Andy Lutomirski 1 sibling, 0 replies; 9+ messages in thread From: tip-bot for Andy Lutomirski @ 2017-09-13 8:55 UTC (permalink / raw) To: linux-tip-commits Cc: tglx, torvalds, luto, jikos, hpa, bpetkov, linux-kernel, jkosina, mingo, peterz 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); ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-13 8:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-08 5:06 [PATCH 0/2] More PCID fixes Andy Lutomirski 2017-09-08 5:06 ` [PATCH 1/2] x86/mm: Get rid of VM_BUG_ON in switch_tlb_irqs_off() Andy Lutomirski 2017-09-08 5:09 ` Andy Lutomirski 2017-09-13 8:55 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski 2017-09-08 5:06 ` [PATCH 2/2] x86/hibernate/64: Mask off CR3's PCID bits in the saved CR3 Andy Lutomirski 2017-09-08 7:59 ` Jiri Kosina 2017-09-10 19:17 ` Andy Lutomirski 2017-09-11 5:48 ` Ingo Molnar 2017-09-13 8:55 ` [tip:x86/urgent] " tip-bot for Andy Lutomirski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.