Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1727881 > unrolled thread

[PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume Andy Lutomirski <luto@kernel.org> - 2017-09-07 05:00 +0200
    [PATCH] mm/debug: Change BUG_ON() crashes to survivable WARN_ON()  warnings Ingo Molnar <mingo@kernel.org> - 2017-09-07 09:10 +0200
      Re: [PATCH] mm/debug: Change BUG_ON() crashes to survivable WARN_ON() warnings Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-07 23:00 +0200
    Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and  resume Jiri Kosina <jikos@kernel.org> - 2017-09-07 09:40 +0200
      Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume Ingo Molnar <mingo@kernel.org> - 2017-09-07 09:50 +0200
        Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and  resume Jiri Kosina <jikos@kernel.org> - 2017-09-07 22:00 +0200
          Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume Andy Lutomirski <luto@amacapital.net> - 2017-09-08 03:30 +0200
    Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume Ingo Molnar <mingo@kernel.org> - 2017-09-07 12:00 +0200
      Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume Borislav Petkov <bp@suse.de> - 2017-09-07 12:20 +0200
    Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume Borislav Petkov <bp@suse.de> - 2017-09-07 12:00 +0200

#1727881 — [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume

When Linux brings a CPU down and back up, it switches to init_mm and then
loads swapper_pg_dir into CR3.  With PCID enabled, this has the side effect
of masking off the ASID bits in CR3.

This can result in some confusion in the TLB handling code.  If we
bring a CPU down and back up with any ASID other than 0, we end up
with the wrong ASID active on the CPU after resume.  This could
cause our internal state to become corrupt, although major
corruption is unlikely because init_mm doesn't have any user pages.
More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion
in the next context switch.  The result of *that* is a failure to
resume from suspend with probability 1 - 1/6^(cpus-1).

Fix it by reinitializing cpu_tlbstate on resume and CPU bringup.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Jiri Kosina <jikos@kernel.org>
Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/tlbflush.h |  2 ++
 arch/x86/kernel/cpu/common.c    |  2 ++
 arch/x86/mm/tlb.c               | 44 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/power/cpu.c            |  1 +
 4 files changed, 49 insertions(+)

diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index d23e61dc0640..4893abf7f74f 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -198,6 +198,8 @@ static inline void cr4_set_bits_and_update_boot(unsigned long mask)
 	cr4_set_bits(mask);
 }
 
+extern void initialize_tlbstate_and_flush(void);
+
 static inline void __native_flush_tlb(void)
 {
 	/*
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index efba8e3da3e2..40cb4d0a5982 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1583,6 +1583,7 @@ void cpu_init(void)
 	mmgrab(&init_mm);
 	me->active_mm = &init_mm;
 	BUG_ON(me->mm);
+	initialize_tlbstate_and_flush();
 	enter_lazy_tlb(&init_mm, me);
 
 	load_sp0(t, &current->thread);
@@ -1637,6 +1638,7 @@ void cpu_init(void)
 	mmgrab(&init_mm);
 	curr->active_mm = &init_mm;
 	BUG_ON(curr->mm);
+	initialize_tlbstate_and_flush();
 	enter_lazy_tlb(&init_mm, curr);
 
 	load_sp0(t, thread);
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index ce104b962a17..dbbcfd59726a 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -214,6 +214,50 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 }
 
 /*
+ * Call this when reinitializing a CPU.  It fixes the following potential
+ * problems:
+ *
+ * - The ASID changed from what cpu_tlbstate thinks it is (most likely
+ *   because the CPU was taken down and came back up with CR3's PCID
+ *   bits clear.  CPU hotplug can do this.
+ *
+ * - The TLB contains junk in slots corresponding to inactive ASIDs.
+ *
+ * - The CPU went so far out to lunch that it may have missed a TLB
+ *   flush.
+ */
+void initialize_tlbstate_and_flush(void)
+{
+	int i;
+	struct mm_struct *mm = this_cpu_read(cpu_tlbstate.loaded_mm);
+	u64 tlb_gen = atomic64_read(&init_mm.context.tlb_gen);
+	unsigned long cr3 = __read_cr3();
+
+	/* Assert that CR3 already references the right mm. */
+	WARN_ON((cr3 & CR3_ADDR_MASK) != __pa(mm->pgd));
+
+	/*
+	 * Assert that CR4.PCIDE is set if needed.  (CR4.PCIDE initialization
+	 * doesn't work like other CR4 bits because it can only be set from
+	 * long mode.)
+	 */
+	WARN_ON(boot_cpu_has(X86_CR4_PCIDE) &&
+		!(cr4_read_shadow() & X86_CR4_PCIDE));
+
+	/* Force ASID 0 and force a TLB flush. */
+	write_cr3(cr3 & ~CR3_PCID_MASK);
+
+	/* Reinitialize tlbstate. */
+	this_cpu_write(cpu_tlbstate.loaded_mm_asid, 0);
+	this_cpu_write(cpu_tlbstate.next_asid, 1);
+	this_cpu_write(cpu_tlbstate.ctxs[0].ctx_id, mm->context.ctx_id);
+	this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen, tlb_gen);
+
+	for (i = 1; i < TLB_NR_DYN_ASIDS; i++)
+		this_cpu_write(cpu_tlbstate.ctxs[i].ctx_id, 0);
+}
+
+/*
  * flush_tlb_func_common()'s memory ordering requirement is that any
  * TLB fills that happen after we flush the TLB are ordered after we
  * read active_mm's tlb_gen.  We don't need any explicit barriers
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 78459a6d455a..4d68d59f457d 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -181,6 +181,7 @@ static void fix_processor_context(void)
 #endif
 	load_TR_desc();				/* This does ltr */
 	load_mm_ldt(current->active_mm);	/* This does lldt */
+	initialize_tlbstate_and_flush();
 
 	fpu__resume_cpu();
 
-- 
2.13.5

[toc] | [next] | [standalone]

#1727961 — [PATCH] mm/debug: Change BUG_ON() crashes to survivable WARN_ON() warnings

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

> More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion
> in the next context switch.  The result of *that* is a failure to
> resume from suspend with probability 1 - 1/6^(cpus-1).

Nice fix, thanks!

On a related note, this bug could have been more debuggable I think.
Could we _please_ change VM_BUG_ON() to WARN_ON() or such?

Here the stupid VM_BUG_ON() crashed Linus's laptop in a totally
undebuggable state ... while a WARN_ON() might have at least
gotten something out to his laptop's screen, right?

So I propose the patch below. Detailed arguments in the changelog.

Pretty please?

==============>
From 673b348ab4a5b2abd17d392cacbf9ab6de3d3042 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Thu, 7 Sep 2017 08:44:13 +0200
Subject: [PATCH] mm/debug: Change BUG_ON() crashes to survivable WARN_ON() warnings

So a VM_BUG_ON() that triggered with the following bug:

  72c0098d92ce: ("x86/mm: Reinitialize TLB state on hotplug and resume")

... crashed and made Linus's laptop totally undebuggable, because when
it triggered there was no screen up yet. It looked like a total lockup
on resume - although we produced a warning that could have helped
narrowing down the problem.

Thus instead of being able to report the warning, Linus had to bisect
the bug the hard way in the middle of the merge window - which is beyond
most users' capability and won't work with regular distro kernels anyway.

To make matters worse, a BUG_ON() done when Xorg is active is utterly
undebuggable anyway in most cases, because it won't be printed on the
framebuffer, and because the BUG_ON() prevents the system log to be
synced to disk.

The symptoms, typically, are similar to what Linus saw: a hard lockup
followed by a bootup that shows nothing in the logs ...

Utterly crazy behavior from the kernel, IMHO!

So instead of crashing the system with a BUG_ON(), use a WARN_ON()
instead. In the above situation the kernel would probably have survived
long enough to produce a kernel log.

I realize that in principle there might be bugs where it's better to stop,
i.e. crash the kernel intentionally.

But I argue that most of the kernel bugs are _not_ such bugs, and being
able to get a log out trumps that concern - because the people who run
new kernels early are not crazy enough to _depend_ on that kernel, and
the ability to get logs off is actually more important.

People wanting to crash the kernel here and now have the burden of proof
and we should not make it the default for any widely used assert to crash
the kernel ...

To not have to do a mass rename this patch simply reuses the existing
VM_BUG_ON() which becomes somewhat of a misnomer after this change.

I will send a rename patch as well after the merge window, separately.

Note that I also made mmdebug.h a bit more readable:

 - align the various constructs coherently and separate them
   visually a bit better

 - use consistent definitions. I mean, half the functions have
   externs, half don't - what the heck?

 - add a bit of description what this is about

Plus, for consistency's sake, VIRTUAL_BUG_ON() is changed as well,
but it's not a widespread primitive.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/mmdebug.h | 56 +++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 451a811f48f2..ad127a020c3f 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -1,6 +1,11 @@
 #ifndef LINUX_MM_DEBUG_H
 #define LINUX_MM_DEBUG_H 1
 
+/*
+ * Various VM related debug assert helper functions.
+ * On perfect kernels they should never trigger.
+ */
+
 #include <linux/bug.h>
 #include <linux/stringify.h>
 
@@ -8,59 +13,64 @@ struct page;
 struct vm_area_struct;
 struct mm_struct;
 
-extern void dump_page(struct page *page, const char *reason);
+extern void   dump_page(struct page *page, const char *reason);
 extern void __dump_page(struct page *page, const char *reason);
-void dump_vma(const struct vm_area_struct *vma);
-void dump_mm(const struct mm_struct *mm);
+extern void   dump_vma(const struct vm_area_struct *vma);
+extern void   dump_mm(const struct mm_struct *mm);
 
 #ifdef CONFIG_DEBUG_VM
-#define VM_BUG_ON(cond) BUG_ON(cond)
+
+#define VM_BUG_ON(cond) WARN_ON(cond)
+
 #define VM_BUG_ON_PAGE(cond, page)					\
 	do {								\
 		if (unlikely(cond)) {					\
 			dump_page(page, "VM_BUG_ON_PAGE(" __stringify(cond)")");\
-			BUG();						\
+			WARN_ON(1);					\
 		}							\
 	} while (0)
+
 #define VM_BUG_ON_VMA(cond, vma)					\
 	do {								\
 		if (unlikely(cond)) {					\
 			dump_vma(vma);					\
-			BUG();						\
+			WARN_ON(1);					\
 		}							\
 	} while (0)
+
 #define VM_BUG_ON_MM(cond, mm)						\
 	do {								\
 		if (unlikely(cond)) {					\
 			dump_mm(mm);					\
-			BUG();						\
+			WARN_ON(1);					\
 		}							\
 	} while (0)
-#define VM_WARN_ON(cond) WARN_ON(cond)
-#define VM_WARN_ON_ONCE(cond) WARN_ON_ONCE(cond)
-#define VM_WARN_ONCE(cond, format...) WARN_ONCE(cond, format)
-#define VM_WARN(cond, format...) WARN(cond, format)
+
+#define VM_WARN_ON(cond)		WARN_ON(cond)
+#define VM_WARN_ON_ONCE(cond)		WARN_ON_ONCE(cond)
+#define VM_WARN_ONCE(cond, format...)	WARN_ONCE(cond, format)
+#define VM_WARN(cond, format...)	WARN(cond, format)
 #else
-#define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)
-#define VM_BUG_ON_PAGE(cond, page) VM_BUG_ON(cond)
-#define VM_BUG_ON_VMA(cond, vma) VM_BUG_ON(cond)
-#define VM_BUG_ON_MM(cond, mm) VM_BUG_ON(cond)
-#define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
-#define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
-#define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
-#define VM_WARN(cond, format...) BUILD_BUG_ON_INVALID(cond)
+#define VM_BUG_ON(cond)			BUILD_BUG_ON_INVALID(cond)
+#define VM_BUG_ON_PAGE(cond, page)	VM_BUG_ON(cond)
+#define VM_BUG_ON_VMA(cond, vma)	VM_BUG_ON(cond)
+#define VM_BUG_ON_MM(cond, mm)		VM_BUG_ON(cond)
+#define VM_WARN_ON(cond)		BUILD_BUG_ON_INVALID(cond)
+#define VM_WARN_ON_ONCE(cond)		BUILD_BUG_ON_INVALID(cond)
+#define VM_WARN_ONCE(cond, format...)	BUILD_BUG_ON_INVALID(cond)
+#define VM_WARN(cond, format...)	BUILD_BUG_ON_INVALID(cond)
 #endif
 
 #ifdef CONFIG_DEBUG_VIRTUAL
-#define VIRTUAL_BUG_ON(cond) BUG_ON(cond)
+#define VIRTUAL_BUG_ON(cond)		WARN_ON(cond)
 #else
-#define VIRTUAL_BUG_ON(cond) do { } while (0)
+#define VIRTUAL_BUG_ON(cond)		do { } while (0)
 #endif
 
 #ifdef CONFIG_DEBUG_VM_PGFLAGS
-#define VM_BUG_ON_PGFLAGS(cond, page) VM_BUG_ON_PAGE(cond, page)
+#define VM_BUG_ON_PGFLAGS(cond, page)	VM_BUG_ON_PAGE(cond, page)
 #else
-#define VM_BUG_ON_PGFLAGS(cond, page) BUILD_BUG_ON_INVALID(cond)
+#define VM_BUG_ON_PGFLAGS(cond, page)	BUILD_BUG_ON_INVALID(cond)
 #endif
 
 #endif

[toc] | [prev] | [next] | [standalone]

#1728424 — Re: [PATCH] mm/debug: Change BUG_ON() crashes to survivable WARN_ON() warnings

On Thu, Sep 7, 2017 at 12:01 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> On a related note, this bug could have been more debuggable I think.
> Could we _please_ change VM_BUG_ON() to WARN_ON() or such?

I think it should be WARN_ON_ONCE(), or at least rate-limited some way.

Because once you have one of the VM bugs, they tend to repeat.

(We had a discussion long ago about making the "ONCE" behavior
actually be "once in a blue moon", and just mean that you warn at most
once every five minutes or something like that. Because the "once"
behavior has also resulted in people missing bugs, because the machine
has been up a long time, and maybe you got a warning at boot time, but
then five days later something fails silently again).

Also, should you do a "dump_vma()" if you then don't give a call stack
because you already did it earlier? So the rate limiting would need to
cover that part too, methinks.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1727979 — Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume

On Wed, 6 Sep 2017, Andy Lutomirski wrote:

> When Linux brings a CPU down and back up, it switches to init_mm and then
> loads swapper_pg_dir into CR3.  With PCID enabled, this has the side effect
> of masking off the ASID bits in CR3.
> 
> This can result in some confusion in the TLB handling code.  If we
> bring a CPU down and back up with any ASID other than 0, we end up
> with the wrong ASID active on the CPU after resume.  This could
> cause our internal state to become corrupt, although major
> corruption is unlikely because init_mm doesn't have any user pages.
> More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion
> in the next context switch.  The result of *that* is a failure to
> resume from suspend with probability 1 - 1/6^(cpus-1).
> 
> Fix it by reinitializing cpu_tlbstate on resume and CPU bringup.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Reported-by: Jiri Kosina <jikos@kernel.org>
> Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

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

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1727994

* Jiri Kosina <jikos@kernel.org> wrote:

> On Wed, 6 Sep 2017, Andy Lutomirski wrote:
> 
> > When Linux brings a CPU down and back up, it switches to init_mm and then
> > loads swapper_pg_dir into CR3.  With PCID enabled, this has the side effect
> > of masking off the ASID bits in CR3.
> > 
> > This can result in some confusion in the TLB handling code.  If we
> > bring a CPU down and back up with any ASID other than 0, we end up
> > with the wrong ASID active on the CPU after resume.  This could
> > cause our internal state to become corrupt, although major
> > corruption is unlikely because init_mm doesn't have any user pages.
> > More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion
> > in the next context switch.  The result of *that* is a failure to
> > resume from suspend with probability 1 - 1/6^(cpus-1).
> > 
> > Fix it by reinitializing cpu_tlbstate on resume and CPU bringup.
> > 
> > Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Reported-by: Jiri Kosina <jikos@kernel.org>
> > Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID")
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> 
> Tested-by: Jiri Kosina <jkosina@suse.cz>

The fix should be upstream already, as of 1c9fe4409ce3 and later.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1728404 — Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume

On Thu, 7 Sep 2017, Ingo Molnar wrote:

> > > When Linux brings a CPU down and back up, it switches to init_mm and then
> > > loads swapper_pg_dir into CR3.  With PCID enabled, this has the side effect
> > > of masking off the ASID bits in CR3.
> > > 
> > > This can result in some confusion in the TLB handling code.  If we
> > > bring a CPU down and back up with any ASID other than 0, we end up
> > > with the wrong ASID active on the CPU after resume.  This could
> > > cause our internal state to become corrupt, although major
> > > corruption is unlikely because init_mm doesn't have any user pages.
> > > More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion
> > > in the next context switch.  The result of *that* is a failure to
> > > resume from suspend with probability 1 - 1/6^(cpus-1).
> > > 
> > > Fix it by reinitializing cpu_tlbstate on resume and CPU bringup.
> > > 
> > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Reported-by: Jiri Kosina <jikos@kernel.org>
> > > Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID")
> > > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > 
> > Tested-by: Jiri Kosina <jkosina@suse.cz>
> 
> The fix should be upstream already, as of 1c9fe4409ce3 and later.

Hm, so I've just experienced two instances in a row of reboot just after 
reading hibernation image (i.e. exactly the same symptom as before) even 
with 3b9f8ed kernel (which contains the fix). Seems like the fix is either 
incomplete (just the probability of it happening is lower), or I'm seeing 
something differet with the same symptom.

I'll try to figure out whether it's the same VM_BUG_ON() triggering, but 
probably will be able to do so only tomorrow.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1728530

> On Sep 7, 2017, at 12:55 PM, Jiri Kosina <jikos@kernel.org> wrote:
> 
> On Thu, 7 Sep 2017, Ingo Molnar wrote:
> 
>>>> When Linux brings a CPU down and back up, it switches to init_mm and then
>>>> loads swapper_pg_dir into CR3.  With PCID enabled, this has the side effect
>>>> of masking off the ASID bits in CR3.
>>>> 
>>>> This can result in some confusion in the TLB handling code.  If we
>>>> bring a CPU down and back up with any ASID other than 0, we end up
>>>> with the wrong ASID active on the CPU after resume.  This could
>>>> cause our internal state to become corrupt, although major
>>>> corruption is unlikely because init_mm doesn't have any user pages.
>>>> More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion
>>>> in the next context switch.  The result of *that* is a failure to
>>>> resume from suspend with probability 1 - 1/6^(cpus-1).
>>>> 
>>>> Fix it by reinitializing cpu_tlbstate on resume and CPU bringup.
>>>> 
>>>> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
>>>> Reported-by: Jiri Kosina <jikos@kernel.org>
>>>> Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID")
>>>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>>> 
>>> Tested-by: Jiri Kosina <jkosina@suse.cz>
>> 
>> The fix should be upstream already, as of 1c9fe4409ce3 and later.
> 
> Hm, so I've just experienced two instances in a row of reboot just after 
> reading hibernation image (i.e. exactly the same symptom as before) even 
> with 3b9f8ed kernel (which contains the fix). Seems like the fix is either 
> incomplete (just the probability of it happening is lower), or I'm seeing 
> something differet with the same symptom.
> 
> I'll try to figure out whether it's the same VM_BUG_ON() triggering, but 
> probably will be able to do so only tomorrow.
> 

Nah, don't waste your time.  I think I see the bug, and it's a different bug.  It's an easy one-line fix, but I have to figure out how to test it.

> -- 
> Jiri Kosina
> SUSE Labs
> 

[toc] | [prev] | [next] | [standalone]

#1728105

* Borislav Petkov <bp@suse.de> wrote:

> > + */
> > +void initialize_tlbstate_and_flush(void)
> 
> I think we should prefix all those visible, TLB-handling functions with
> "tlb_". So you'd have tlb_init_state_and_flush().

Agreed absolutely, but note that this affects more functions as well - for example 
enter_lazy_tlb() should probably be tlb_lazy_enter() - or at least 
lazy_tlb_enter() or such?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1728120

On Thu, Sep 07, 2017 at 11:59:32AM +0200, Ingo Molnar wrote:
> Agreed absolutely, but note that this affects more functions as well - for example 
> enter_lazy_tlb() should probably be tlb_lazy_enter() - or at least 
> lazy_tlb_enter() or such?

Yeah, or tlb_enter_lazy() or even tlb_enter_lazy_mode() or so. I could
give it a try when things get a bit quieter and see how it actually
looks and how much "better" it becomes, staring at that code...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1728107

Just nitpicks:

On Wed, Sep 06, 2017 at 07:54:53PM -0700, Andy Lutomirski wrote:
> When Linux brings a CPU down and back up, it switches to init_mm and then
> loads swapper_pg_dir into CR3.  With PCID enabled, this has the side effect
> of masking off the ASID bits in CR3.
> 
> This can result in some confusion in the TLB handling code.  If we
> bring a CPU down and back up with any ASID other than 0, we end up
> with the wrong ASID active on the CPU after resume.  This could
> cause our internal state to become corrupt, although major
> corruption is unlikely because init_mm doesn't have any user pages.
> More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion
> in the next context switch.  The result of *that* is a failure to
> resume from suspend with probability 1 - 1/6^(cpus-1).
> 
> Fix it by reinitializing cpu_tlbstate on resume and CPU bringup.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Reported-by: Jiri Kosina <jikos@kernel.org>
> Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/asm/tlbflush.h |  2 ++
>  arch/x86/kernel/cpu/common.c    |  2 ++
>  arch/x86/mm/tlb.c               | 44 +++++++++++++++++++++++++++++++++++++++++
>  arch/x86/power/cpu.c            |  1 +
>  4 files changed, 49 insertions(+)
> 
> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
> index d23e61dc0640..4893abf7f74f 100644
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -198,6 +198,8 @@ static inline void cr4_set_bits_and_update_boot(unsigned long mask)
>  	cr4_set_bits(mask);
>  }
>  
> +extern void initialize_tlbstate_and_flush(void);

Let's put that declaration at the end.

>  static inline void __native_flush_tlb(void)
>  {
>  	/*
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index efba8e3da3e2..40cb4d0a5982 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1583,6 +1583,7 @@ void cpu_init(void)
>  	mmgrab(&init_mm);
>  	me->active_mm = &init_mm;
>  	BUG_ON(me->mm);
> +	initialize_tlbstate_and_flush();
>  	enter_lazy_tlb(&init_mm, me);
>  
>  	load_sp0(t, &current->thread);
> @@ -1637,6 +1638,7 @@ void cpu_init(void)
>  	mmgrab(&init_mm);
>  	curr->active_mm = &init_mm;
>  	BUG_ON(curr->mm);
> +	initialize_tlbstate_and_flush();
>  	enter_lazy_tlb(&init_mm, curr);
>  
>  	load_sp0(t, thread);
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index ce104b962a17..dbbcfd59726a 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -214,6 +214,50 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>  }
>  
>  /*
> + * Call this when reinitializing a CPU.  It fixes the following potential
> + * problems:
> + *
> + * - The ASID changed from what cpu_tlbstate thinks it is (most likely
> + *   because the CPU was taken down and came back up with CR3's PCID
> + *   bits clear.  CPU hotplug can do this.
> + *
> + * - The TLB contains junk in slots corresponding to inactive ASIDs.
> + *
> + * - The CPU went so far out to lunch that it may have missed a TLB
> + *   flush.
> + */
> +void initialize_tlbstate_and_flush(void)

I think we should prefix all those visible, TLB-handling functions with
"tlb_". So you'd have tlb_init_state_and_flush().

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web