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

Groups > linux.kernel > #1422047 > unrolled thread

[PATCH] Linux VM workaround for Knights Landing A/D leak

Back to article view | Back to linux.kernel

Contents

  [PATCH] Linux VM workaround for Knights Landing A/D leak Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> - 2016-06-14 18:00 +0200
    Re: [PATCH] Linux VM workaround for Knights Landing A/D leak kbuild test robot <lkp@intel.com> - 2016-06-14 18:40 +0200
    Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-14 18:50 +0200
      RE: [PATCH] Linux VM workaround for Knights Landing A/D leak "Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com> - 2016-06-14 19:00 +0200
      [PATCH v2] Linux VM workaround for Knights Landing A/D leak Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> - 2016-06-14 19:10 +0200
        Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-14 19:30 +0200
          Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-06-14 20:40 +0200
            Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-14 21:00 +0200
              Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Borislav Petkov <bp@alien8.de> - 2016-06-14 21:20 +0200
                Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak "H. Peter Anvin" <hpa@zytor.com> - 2016-06-14 22:30 +0200
                  Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Borislav Petkov <bp@alien8.de> - 2016-06-14 22:50 +0200
                    Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak "H. Peter Anvin" <hpa@zytor.com> - 2016-06-14 23:00 +0200
                      Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak "H. Peter Anvin" <hpa@zytor.com> - 2016-06-14 23:10 +0200
                      Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Borislav Petkov <bp@alien8.de> - 2016-06-14 23:10 +0200
                        Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak "H. Peter Anvin" <hpa@zytor.com> - 2016-06-14 23:20 +0200
        Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Borislav Petkov <bp@alien8.de> - 2016-06-14 20:20 +0200
          RE: [PATCH v2] Linux VM workaround for Knights Landing A/D leak "Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com> - 2016-06-15 15:20 +0200
        Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-14 20:40 +0200
          RE: [PATCH v2] Linux VM workaround for Knights Landing A/D leak "Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com> - 2016-06-15 15:20 +0200
            Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-15 22:10 +0200
              Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-15 22:20 +0200
                Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-15 22:30 +0200
        [PATCH v3] Linux VM workaround for Knights Landing A/D leak Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> - 2016-06-16 17:20 +0200
          Re: [PATCH v3] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-16 18:50 +0200
          Re: [PATCH v3] Linux VM workaround for Knights Landing A/D leak Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-16 22:30 +0200
      Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-14 19:20 +0200
        Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-14 22:20 +0200
          Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-14 23:40 +0200
            Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Andy Lutomirski <luto@amacapital.net> - 2016-06-15 04:30 +0200
              Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-15 04:40 +0200
                Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Andy Lutomirski <luto@amacapital.net> - 2016-06-15 04:40 +0200
                  Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-15 04:50 +0200
                    Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Andy Lutomirski <luto@amacapital.net> - 2016-06-15 05:10 +0200
            Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Nadav Amit <nadav.amit@gmail.com> - 2016-06-15 05:30 +0200
    Re: [PATCH] Linux VM workaround for Knights Landing A/D leak kbuild test robot <lkp@intel.com> - 2016-06-14 19:00 +0200
    Re: [PATCH] Linux VM workaround for Knights Landing A/D leak Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-14 19:30 +0200
      RE: [PATCH] Linux VM workaround for Knights Landing A/D leak "Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com> - 2016-06-15 15:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1423391 — Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak

On 06/15/2016 01:04 PM, Nadav Amit wrote:
> Be careful here. According to the SDM when invalidating a huge-page,
> each 4KB page needs to be invalidated separately. In practice, when
> Linux invalidates 2MB/1GB pages it performs a full TLB flush. The
> full flush may not be required on knights landing, and specifically
> for the workaround, but you should check.  

Where do you get that?  The SDM says: "they (TLB invalidation operations
invalidate all TLB entries corresponding to the translation specified by
the paging structures."

Here's the full paragraph from the SDM

... some processors may choose to cache multiple smaller-page TLB
entries for a translation specified by the paging structures to use a
page larger than 4 KBytes. There is no way for software to be aware
that multiple translations for smaller pages have been used for a large
page. The INVLPG instruction and page faults provide the same assurances
that they provide when a single TLB entry is used: they invalidate all
TLB entries corresponding to the translation specified by the paging
structures.

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

#1423416 — Re: [PATCH v2] Linux VM workaround for Knights Landing A/D leak

Dave Hansen <dave.hansen@linux.intel.com> wrote:

> On 06/15/2016 01:04 PM, Nadav Amit wrote:
>> Be careful here. According to the SDM when invalidating a huge-page,
>> each 4KB page needs to be invalidated separately. In practice, when
>> Linux invalidates 2MB/1GB pages it performs a full TLB flush. The
>> full flush may not be required on knights landing, and specifically
>> for the workaround, but you should check.  
> 
> Where do you get that?  The SDM says: "they (TLB invalidation operations
> invalidate all TLB entries corresponding to the translation specified by
> the paging structures.”

You are absolutely correct. Last time I write something based on my
recollection of the SDM without re-reading again. Sorry.

Nadav

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

#1424173 — [PATCH v3] Linux VM workaround for Knights Landing A/D leak

From: Andi Kleen <ak@linux.intel.com>

Knights Landing has an issue that a thread setting A or D bits
may not do so atomically against checking the present bit.
A thread which is going to page fault may still set those
bits, even though the present bit was already atomically cleared.

This implies that when the kernel clears present atomically,
some time later the supposed to be zero entry could be corrupted
with stray A or D bits.

Since the PTE could be already used for storing a swap index,
or a NUMA migration index, this cannot be tolerated. Most
of the time the kernel detects the problem, but in some
rare cases it may not.

This patch enforces that the page unmap path in vmscan/direct reclaim
always flushes other CPUs after clearing each page, and also
clears the PTE again after the flush. A new memory barrier may be
required, but this code is at least consistent with all
the existing uses in the kernel. If we decide that we need a new
barrier, it can added at the same time as the rest of the tree.

For reclaim this brings the performance back to before Mel's
flushing changes, but for unmap it disables batching.

This makes sure any leaked A/D bits are immediately cleared before the entry
is used for something else.

Any parallel faults that check for entry is zero may loop,
but they should eventually recover after the entry is written.

Also other users may spin in the page table lock until we
"fixed" the PTE. This is ensured by always taking the page table lock
even for the swap cache case. Previously this was only done
on architectures with non atomic PTE accesses (such as 32bit PTE),
but now it is also done when this bug workaround is active.

I audited apply_pte_range and other users of arch_enter_lazy...
and they seem to all not clear the present bit.

Right now the extra flush is done in the low level
architecture code, while the higher level code still
does batched TLB flush. This means there is always one extra
unnecessary TLB flush now. As a followon optimization
this could be avoided by telling the callers that
the flush already happenend.

The official erratum will be posted hopefully by the end of July'16.

v3 (Lukasz Anaczkowski):
    () Improved documentation
    () Removed unnecessary declaration and call to fix_pte_leak from hugetlb.h
    () Moved fix_pte_leak() definition from tlb.c to intel.c
    () Replaced boot_cpu_has_bug() with static_cpu_has_bug()
    () pr_info_once instead of pr_info
    () Fix applies only to 64-bit kernels as Knights Landing does not
       support 32-bit kernels

v2 (Lukasz Anaczkowski):
    () added call to smp_mb__after_atomic() to synchornize with
       switch_mm, based on Nadav's comment
    () fixed compilation breakage

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com>
---
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/pgtable.h     | 10 ++++++++++
 arch/x86/include/asm/pgtable_64.h  | 16 +++++++++++++++
 arch/x86/kernel/cpu/intel.c        | 41 ++++++++++++++++++++++++++++++++++++++
 include/linux/mm.h                 |  4 ++++
 mm/memory.c                        |  3 ++-
 6 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 4a41348..2c48011 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -303,6 +303,7 @@
 #define X86_BUG_SYSRET_SS_ATTRS	X86_BUG(8) /* SYSRET doesn't fix up SS attrs */
 #define X86_BUG_NULL_SEG	X86_BUG(9) /* Nulling a selector preserves the base */
 #define X86_BUG_SWAPGS_FENCE	X86_BUG(10) /* SWAPGS without input dep on GS */
+#define X86_BUG_PTE_LEAK        X86_BUG(11) /* PTE may leak A/D bits after clear */
 
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 1a27396..14abcb3 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -794,11 +794,21 @@ extern int ptep_test_and_clear_young(struct vm_area_struct *vma,
 extern int ptep_clear_flush_young(struct vm_area_struct *vma,
 				  unsigned long address, pte_t *ptep);
 
+#if defined(CONFIG_X86_64) && defined(CONFIG_CPU_SUP_INTEL)
+extern void fix_pte_leak(struct mm_struct *mm, unsigned long addr,
+			 pte_t *ptep);
+#else
+static inline void fix_pte_leak(struct mm_struct *mm, unsigned long addr,
+				pte_t *ptep) {}
+#endif
+
 #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
 static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
 				       pte_t *ptep)
 {
 	pte_t pte = native_ptep_get_and_clear(ptep);
+	if (static_cpu_has_bug(X86_BUG_PTE_LEAK))
+		fix_pte_leak(mm, addr, ptep);
 	pte_update(mm, addr, ptep);
 	return pte;
 }
diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
index 2ee7811..be7d63c 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -178,6 +178,22 @@ extern void cleanup_highmap(void);
 extern void init_extra_mapping_uc(unsigned long phys, unsigned long size);
 extern void init_extra_mapping_wb(unsigned long phys, unsigned long size);
 
+/*
+ * Intel Xeon Phi x200 codenamed Knights Landing has an issue that a thread
+ * setting A or D bits may not do so atomically against checking the present
+ * bit. A thread which is going to page fault may still set those
+ * bits, even though the present bit was already atomically cleared.
+ *
+ * This implies that when the kernel clears present atomically,
+ * some time later the supposed to be zero entry could be corrupted
+ * with stray A or D bits.
+ */
+#define ARCH_HAS_NEEDS_SWAP_PTL 1
+static inline bool arch_needs_swap_ptl(void)
+{
+       return static_cpu_has_bug(X86_BUG_PTE_LEAK);
+}
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* _ASM_X86_PGTABLE_64_H */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 6e2ffbe..b5f5bab 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -7,12 +7,17 @@
 #include <linux/thread_info.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
+#include <linux/mm_types.h>
 
 #include <asm/cpufeature.h>
 #include <asm/pgtable.h>
 #include <asm/msr.h>
 #include <asm/bugs.h>
 #include <asm/cpu.h>
+#include <asm/intel-family.h>
+#include <asm/tlbflush.h>
+
+#include <trace/events/tlb.h>
 
 #ifdef CONFIG_X86_64
 #include <linux/topology.h>
@@ -60,6 +65,35 @@ void check_mpx_erratum(struct cpuinfo_x86 *c)
 	}
 }
 
+#ifdef CONFIG_X86_64
+/*
+ * Knights Landing has an issue that a thread setting A or D bits
+ * may not do so atomically against checking the present bit.
+ * A thread which is going to page fault may still set those
+ * bits, even though the present bit was already atomically cleared.
+ *
+ * Entering here, the current CPU just cleared the PTE.  But,
+ * another thread may have raced and set the A or D bits, or be
+ * _about_ to set the bits.  Shooting their TLB entry down will
+ * ensure they see the cleared PTE and will not set A or D and
+ * won't corrupt swap entries.
+ */
+void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
+{
+	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
+		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
+		flush_tlb_others(mm_cpumask(mm), mm, addr,
+				 addr + PAGE_SIZE);
+		mb();
+		/*
+		 * Clear the PTE one more time, in case the other thread set A/D
+		 * before we sent the TLB flush.
+		 */
+		set_pte(ptep, __pte(0));
+	}
+}
+#endif
+
 static void early_init_intel(struct cpuinfo_x86 *c)
 {
 	u64 misc_enable;
@@ -181,6 +215,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 		}
 	}
 
+#ifdef CONFIG_X86_64
+	if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL) {
+		pr_info_once("x86/intel: Enabling PTE leaking workaround\n");
+		set_cpu_bug(c, X86_BUG_PTE_LEAK);
+	}
+#endif
+
 	/*
 	 * Intel Quark Core DevMan_001.pdf section 6.4.11
 	 * "The operating system also is required to invalidate (i.e., flush)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5df5feb..5c80fe09 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2404,6 +2404,10 @@ static inline bool debug_guardpage_enabled(void) { return false; }
 static inline bool page_is_guard(struct page *page) { return false; }
 #endif /* CONFIG_DEBUG_PAGEALLOC */
 
+#ifndef ARCH_HAS_NEEDS_SWAP_PTL
+static inline bool arch_needs_swap_ptl(void) { return false; }
+#endif
+
 #if MAX_NUMNODES > 1
 void __init setup_nr_node_ids(void);
 #else
diff --git a/mm/memory.c b/mm/memory.c
index 15322b7..0d6ef39 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1960,7 +1960,8 @@ static inline int pte_unmap_same(struct mm_struct *mm, pmd_t *pmd,
 {
 	int same = 1;
 #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT)
-	if (sizeof(pte_t) > sizeof(unsigned long)) {
+	if (arch_needs_swap_ptl() ||
+	    sizeof(pte_t) > sizeof(unsigned long)) {
 		spinlock_t *ptl = pte_lockptr(mm, pmd);
 		spin_lock(ptl);
 		same = pte_same(*page_table, orig_pte);
-- 
1.8.3.1

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

#1424268 — Re: [PATCH v3] Linux VM workaround for Knights Landing A/D leak

Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
> +{
> +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
> +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
This tracing seems incorrect since you don’t perform a local flush.
I don’t think you need any tracing - native_flush_tlb_others will do it for you.

> +		flush_tlb_others(mm_cpumask(mm), mm, addr,
> +				 addr + PAGE_SIZE);
> +		mb();
Why do you need the memory barrier?

Regards,
Nadav

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

#1424382 — Re: [PATCH v3] Linux VM workaround for Knights Landing A/D leak

On 06/16/2016 08:14 AM, Lukasz Anaczkowski wrote:
> For reclaim this brings the performance back to before Mel's
> flushing changes, but for unmap it disables batching.

This turns out to be pretty catastrophic for unmap.  In a workload that
uses, say 200 hardware threads and alloc/frees() a few MB/sec, this ends
up costing hundreds of thousands of extra received IPIs.  10MB=~2500
ptes, and at with 200 threads, that's 250,000 IPIs received just to free
10MB of memory.

The initial testing we did on this was on a *bunch* of threads all doing
alloc/free.  But this is bottlenecked on other things, like mmap_sem
being held for write.

The scenario that we really needed to test here was on lots of threads
doing processing and 1 thread doing alloc/free.

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

#1422113

On 06/14/2016 09:47 AM, Nadav Amit wrote:
> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
> 
>> > From: Andi Kleen <ak@linux.intel.com>
>> > +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>> > +{
> Here there should be a call to smp_mb__after_atomic() to synchronize with
> switch_mm. I submitted a similar patch, which is still pending (hint).
> 
>> > +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>> > +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>> > +		flush_tlb_others(mm_cpumask(mm), mm, addr,
>> > +				 addr + PAGE_SIZE);
>> > +		mb();
>> > +		set_pte(ptep, __pte(0));
>> > +	}
>> > +}

Shouldn't that barrier be incorporated in the TLB flush code itself and
not every single caller (like this code is)?

It is insane to require individual TLB flushers to be concerned with the
barriers.

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

#1422289

Dave Hansen <dave.hansen@linux.intel.com> wrote:

> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>> 
>>>> From: Andi Kleen <ak@linux.intel.com>
>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>> +{
>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>> switch_mm. I submitted a similar patch, which is still pending (hint).
>> 
>>>> +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>> +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>> +		flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>> +				 addr + PAGE_SIZE);
>>>> +		mb();
>>>> +		set_pte(ptep, __pte(0));
>>>> +	}
>>>> +}
> 
> Shouldn't that barrier be incorporated in the TLB flush code itself and
> not every single caller (like this code is)?
> 
> It is insane to require individual TLB flushers to be concerned with the
> barriers.

IMHO it is best to use existing flushing interfaces instead of creating
new ones. 

In theory, fix_pte_leak could have used flush_tlb_page. But the problem
is that flush_tlb_page requires the vm_area_struct as an argument, which
ptep_get_and_clear (and others) do not have.

I don’t know which architecture needs the vm_area_struct, since x86 and
some others I looked at (e.g., ARM) only need the mm_struct.

Nadav

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

#1422362

On 06/14/2016 01:16 PM, Nadav Amit wrote:
> Dave Hansen <dave.hansen@linux.intel.com> wrote:
> 
>> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>>>
>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>>> +{
>>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>>> switch_mm. I submitted a similar patch, which is still pending (hint).
>>>
>>>>> +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>>> +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>>> +		flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>>> +				 addr + PAGE_SIZE);
>>>>> +		mb();
>>>>> +		set_pte(ptep, __pte(0));
>>>>> +	}
>>>>> +}
>>
>> Shouldn't that barrier be incorporated in the TLB flush code itself and
>> not every single caller (like this code is)?
>>
>> It is insane to require individual TLB flushers to be concerned with the
>> barriers.
> 
> IMHO it is best to use existing flushing interfaces instead of creating
> new ones. 

Yeah, or make these things a _little_ harder to get wrong.  That little
snippet above isn't so crazy that we should be depending on open-coded
barriers to get it right.

Should we just add a barrier to mm_cpumask() itself?  That should stop
the race.  Or maybe we need a new primitive like:

/*
 * Call this if a full barrier has been executed since the last
 * pagetable modification operation.
 */
static int __other_cpus_need_tlb_flush(struct mm_struct *mm)
{
	/* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */
	return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) <
		nr_cpu_ids;
}


static int other_cpus_need_tlb_flush(struct mm_struct *mm)
{
	/*
	 * Synchronizes with switch_mm.  Makes sure that we do not
	 * observe a bit having been cleared in mm_cpumask() before
 	 * the other processor has seen our pagetable update.  See
	 * switch_mm().
	 */
	smp_mb__after_atomic();

	return __other_cpus_need_tlb_flush(mm)
}

We should be able to deploy other_cpus_need_tlb_flush() in most of the
cases where we are doing "cpumask_any_but(mm_cpumask(mm),
smp_processor_id()) < nr_cpu_ids".

Right?

> In theory, fix_pte_leak could have used flush_tlb_page. But the problem
> is that flush_tlb_page requires the vm_area_struct as an argument, which
> ptep_get_and_clear (and others) do not have.

That, and we do not want/need to flush the _current_ processor's TLB.
flush_tlb_page() would have done that unnecessarily.  That's not the end
of the world here, but it is a downside.

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

#1422505

On Tue, Jun 14, 2016 at 2:37 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 06/14/2016 01:16 PM, Nadav Amit wrote:
>> Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>
>>> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>>>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>>>>
>>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>>>> +{
>>>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>>>> switch_mm. I submitted a similar patch, which is still pending (hint).
>>>>
>>>>>> + if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>>>> +         trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>>>> +         flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>>>> +                          addr + PAGE_SIZE);
>>>>>> +         mb();
>>>>>> +         set_pte(ptep, __pte(0));
>>>>>> + }
>>>>>> +}
>>>
>>> Shouldn't that barrier be incorporated in the TLB flush code itself and
>>> not every single caller (like this code is)?
>>>
>>> It is insane to require individual TLB flushers to be concerned with the
>>> barriers.
>>
>> IMHO it is best to use existing flushing interfaces instead of creating
>> new ones.
>
> Yeah, or make these things a _little_ harder to get wrong.  That little
> snippet above isn't so crazy that we should be depending on open-coded
> barriers to get it right.
>
> Should we just add a barrier to mm_cpumask() itself?  That should stop
> the race.  Or maybe we need a new primitive like:
>
> /*
>  * Call this if a full barrier has been executed since the last
>  * pagetable modification operation.
>  */
> static int __other_cpus_need_tlb_flush(struct mm_struct *mm)
> {
>         /* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */
>         return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) <
>                 nr_cpu_ids;
> }
>
>
> static int other_cpus_need_tlb_flush(struct mm_struct *mm)
> {
>         /*
>          * Synchronizes with switch_mm.  Makes sure that we do not
>          * observe a bit having been cleared in mm_cpumask() before
>          * the other processor has seen our pagetable update.  See
>          * switch_mm().
>          */
>         smp_mb__after_atomic();
>
>         return __other_cpus_need_tlb_flush(mm)
> }
>
> We should be able to deploy other_cpus_need_tlb_flush() in most of the
> cases where we are doing "cpumask_any_but(mm_cpumask(mm),
> smp_processor_id()) < nr_cpu_ids".

IMO this is a bit nuts.  smp_mb__after_atomic() doesn't do anything on
x86.  And, even if it did, why should the flush code assume that the
previous store was atomic?

What's the issue being fixed / worked around here?

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

#1422511

Andy Lutomirski <luto@amacapital.net> wrote:

> On Tue, Jun 14, 2016 at 2:37 PM, Dave Hansen
> <dave.hansen@linux.intel.com> wrote:
>> On 06/14/2016 01:16 PM, Nadav Amit wrote:
>>> Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>> 
>>>> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>>>>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>>>>> 
>>>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>>>>> +{
>>>>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>>>>> switch_mm. I submitted a similar patch, which is still pending (hint).
>>>>> 
>>>>>>> + if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>>>>> +         trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>>>>> +         flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>>>>> +                          addr + PAGE_SIZE);
>>>>>>> +         mb();
>>>>>>> +         set_pte(ptep, __pte(0));
>>>>>>> + }
>>>>>>> +}
>>>> 
>>>> Shouldn't that barrier be incorporated in the TLB flush code itself and
>>>> not every single caller (like this code is)?
>>>> 
>>>> It is insane to require individual TLB flushers to be concerned with the
>>>> barriers.
>>> 
>>> IMHO it is best to use existing flushing interfaces instead of creating
>>> new ones.
>> 
>> Yeah, or make these things a _little_ harder to get wrong.  That little
>> snippet above isn't so crazy that we should be depending on open-coded
>> barriers to get it right.
>> 
>> Should we just add a barrier to mm_cpumask() itself?  That should stop
>> the race.  Or maybe we need a new primitive like:
>> 
>> /*
>> * Call this if a full barrier has been executed since the last
>> * pagetable modification operation.
>> */
>> static int __other_cpus_need_tlb_flush(struct mm_struct *mm)
>> {
>>        /* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */
>>        return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) <
>>                nr_cpu_ids;
>> }
>> 
>> 
>> static int other_cpus_need_tlb_flush(struct mm_struct *mm)
>> {
>>        /*
>>         * Synchronizes with switch_mm.  Makes sure that we do not
>>         * observe a bit having been cleared in mm_cpumask() before
>>         * the other processor has seen our pagetable update.  See
>>         * switch_mm().
>>         */
>>        smp_mb__after_atomic();
>> 
>>        return __other_cpus_need_tlb_flush(mm)
>> }
>> 
>> We should be able to deploy other_cpus_need_tlb_flush() in most of the
>> cases where we are doing "cpumask_any_but(mm_cpumask(mm),
>> smp_processor_id()) < nr_cpu_ids".
> 
> IMO this is a bit nuts.  smp_mb__after_atomic() doesn't do anything on
> x86.  And, even if it did, why should the flush code assume that the
> previous store was atomic?
> 
> What's the issue being fixed / worked around here?

It does a compiler barrier, which prevents the decision whether a
remote TLB shootdown is required to be made before the PTE is set.

I agree that PTEs may not be written atomically in certain cases
(although I am unaware of such cases, except on full-mm flush).

Having said that, I think that all the TLB flush/shootdown logic
should not be open-coded at all and be left in the arch-specific
implementation. People keep making small mistakes when they 
reimplement the flushing logic.

This patch, for example, also has a bug in the way it traces the
flush - it marks full flush, when it flushes a single page:
>>>>>>> trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);


Regards,
Nadav

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

#1422516

On Tue, Jun 14, 2016 at 7:35 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Tue, Jun 14, 2016 at 2:37 PM, Dave Hansen
>> <dave.hansen@linux.intel.com> wrote:
>>> On 06/14/2016 01:16 PM, Nadav Amit wrote:
>>>> Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>>>
>>>>> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>>>>>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>>>>>>
>>>>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>>>>>> +{
>>>>>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>>>>>> switch_mm. I submitted a similar patch, which is still pending (hint).
>>>>>>
>>>>>>>> + if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>>>>>> +         trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>>>>>> +         flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>>>>>> +                          addr + PAGE_SIZE);
>>>>>>>> +         mb();
>>>>>>>> +         set_pte(ptep, __pte(0));
>>>>>>>> + }
>>>>>>>> +}
>>>>>
>>>>> Shouldn't that barrier be incorporated in the TLB flush code itself and
>>>>> not every single caller (like this code is)?
>>>>>
>>>>> It is insane to require individual TLB flushers to be concerned with the
>>>>> barriers.
>>>>
>>>> IMHO it is best to use existing flushing interfaces instead of creating
>>>> new ones.
>>>
>>> Yeah, or make these things a _little_ harder to get wrong.  That little
>>> snippet above isn't so crazy that we should be depending on open-coded
>>> barriers to get it right.
>>>
>>> Should we just add a barrier to mm_cpumask() itself?  That should stop
>>> the race.  Or maybe we need a new primitive like:
>>>
>>> /*
>>> * Call this if a full barrier has been executed since the last
>>> * pagetable modification operation.
>>> */
>>> static int __other_cpus_need_tlb_flush(struct mm_struct *mm)
>>> {
>>>        /* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */
>>>        return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) <
>>>                nr_cpu_ids;
>>> }
>>>
>>>
>>> static int other_cpus_need_tlb_flush(struct mm_struct *mm)
>>> {
>>>        /*
>>>         * Synchronizes with switch_mm.  Makes sure that we do not
>>>         * observe a bit having been cleared in mm_cpumask() before
>>>         * the other processor has seen our pagetable update.  See
>>>         * switch_mm().
>>>         */
>>>        smp_mb__after_atomic();
>>>
>>>        return __other_cpus_need_tlb_flush(mm)
>>> }
>>>
>>> We should be able to deploy other_cpus_need_tlb_flush() in most of the
>>> cases where we are doing "cpumask_any_but(mm_cpumask(mm),
>>> smp_processor_id()) < nr_cpu_ids".
>>
>> IMO this is a bit nuts.  smp_mb__after_atomic() doesn't do anything on
>> x86.  And, even if it did, why should the flush code assume that the
>> previous store was atomic?
>>
>> What's the issue being fixed / worked around here?
>
> It does a compiler barrier, which prevents the decision whether a
> remote TLB shootdown is required to be made before the PTE is set.
>
> I agree that PTEs may not be written atomically in certain cases
> (although I am unaware of such cases, except on full-mm flush).

How about plain set_pte?  It's atomic (aligned word-sized write), but
it's not atomic in the _after_atomic sense.

--Andy

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

#1422519

Andy Lutomirski <luto@amacapital.net> wrote:

> On Tue, Jun 14, 2016 at 7:35 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
>> Andy Lutomirski <luto@amacapital.net> wrote:
>> 
>>> On Tue, Jun 14, 2016 at 2:37 PM, Dave Hansen
>>> <dave.hansen@linux.intel.com> wrote:
>>>> On 06/14/2016 01:16 PM, Nadav Amit wrote:
>>>>> Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>>>> 
>>>>>> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>>>>>>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>>>>>>> 
>>>>>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>>>>>>> +{
>>>>>>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>>>>>>> switch_mm. I submitted a similar patch, which is still pending (hint).
>>>>>>> 
>>>>>>>>> + if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>>>>>>> +         trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>>>>>>> +         flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>>>>>>> +                          addr + PAGE_SIZE);
>>>>>>>>> +         mb();
>>>>>>>>> +         set_pte(ptep, __pte(0));
>>>>>>>>> + }
>>>>>>>>> +}
>>>>>> 
>>>>>> Shouldn't that barrier be incorporated in the TLB flush code itself and
>>>>>> not every single caller (like this code is)?
>>>>>> 
>>>>>> It is insane to require individual TLB flushers to be concerned with the
>>>>>> barriers.
>>>>> 
>>>>> IMHO it is best to use existing flushing interfaces instead of creating
>>>>> new ones.
>>>> 
>>>> Yeah, or make these things a _little_ harder to get wrong.  That little
>>>> snippet above isn't so crazy that we should be depending on open-coded
>>>> barriers to get it right.
>>>> 
>>>> Should we just add a barrier to mm_cpumask() itself?  That should stop
>>>> the race.  Or maybe we need a new primitive like:
>>>> 
>>>> /*
>>>> * Call this if a full barrier has been executed since the last
>>>> * pagetable modification operation.
>>>> */
>>>> static int __other_cpus_need_tlb_flush(struct mm_struct *mm)
>>>> {
>>>>       /* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */
>>>>       return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) <
>>>>               nr_cpu_ids;
>>>> }
>>>> 
>>>> 
>>>> static int other_cpus_need_tlb_flush(struct mm_struct *mm)
>>>> {
>>>>       /*
>>>>        * Synchronizes with switch_mm.  Makes sure that we do not
>>>>        * observe a bit having been cleared in mm_cpumask() before
>>>>        * the other processor has seen our pagetable update.  See
>>>>        * switch_mm().
>>>>        */
>>>>       smp_mb__after_atomic();
>>>> 
>>>>       return __other_cpus_need_tlb_flush(mm)
>>>> }
>>>> 
>>>> We should be able to deploy other_cpus_need_tlb_flush() in most of the
>>>> cases where we are doing "cpumask_any_but(mm_cpumask(mm),
>>>> smp_processor_id()) < nr_cpu_ids".
>>> 
>>> IMO this is a bit nuts.  smp_mb__after_atomic() doesn't do anything on
>>> x86.  And, even if it did, why should the flush code assume that the
>>> previous store was atomic?
>>> 
>>> What's the issue being fixed / worked around here?
>> 
>> It does a compiler barrier, which prevents the decision whether a
>> remote TLB shootdown is required to be made before the PTE is set.
>> 
>> I agree that PTEs may not be written atomically in certain cases
>> (although I am unaware of such cases, except on full-mm flush).
> 
> How about plain set_pte?  It's atomic (aligned word-sized write), but
> it's not atomic in the _after_atomic sense.

Can you point me to a place where set_pte is used before a TLB
invalidation/shootdown, excluding this patch and the fullmm case?

I am not claiming there is no such case, but I am unaware of such
one. PTEs are cleared on SMP using xchg, and similarly the dirty bit
is cleared with an atomic operation.

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

#1422526

On Tue, Jun 14, 2016 at 7:44 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Tue, Jun 14, 2016 at 7:35 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
>>> Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>>> On Tue, Jun 14, 2016 at 2:37 PM, Dave Hansen
>>>> <dave.hansen@linux.intel.com> wrote:
>>>>> On 06/14/2016 01:16 PM, Nadav Amit wrote:
>>>>>> Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>>>>>
>>>>>>> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>>>>>>>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>>>>>>>>
>>>>>>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>>>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>>>>>>>> +{
>>>>>>>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>>>>>>>> switch_mm. I submitted a similar patch, which is still pending (hint).
>>>>>>>>
>>>>>>>>>> + if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>>>>>>>> +         trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>>>>>>>> +         flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>>>>>>>> +                          addr + PAGE_SIZE);
>>>>>>>>>> +         mb();
>>>>>>>>>> +         set_pte(ptep, __pte(0));
>>>>>>>>>> + }
>>>>>>>>>> +}
>>>>>>>
>>>>>>> Shouldn't that barrier be incorporated in the TLB flush code itself and
>>>>>>> not every single caller (like this code is)?
>>>>>>>
>>>>>>> It is insane to require individual TLB flushers to be concerned with the
>>>>>>> barriers.
>>>>>>
>>>>>> IMHO it is best to use existing flushing interfaces instead of creating
>>>>>> new ones.
>>>>>
>>>>> Yeah, or make these things a _little_ harder to get wrong.  That little
>>>>> snippet above isn't so crazy that we should be depending on open-coded
>>>>> barriers to get it right.
>>>>>
>>>>> Should we just add a barrier to mm_cpumask() itself?  That should stop
>>>>> the race.  Or maybe we need a new primitive like:
>>>>>
>>>>> /*
>>>>> * Call this if a full barrier has been executed since the last
>>>>> * pagetable modification operation.
>>>>> */
>>>>> static int __other_cpus_need_tlb_flush(struct mm_struct *mm)
>>>>> {
>>>>>       /* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */
>>>>>       return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) <
>>>>>               nr_cpu_ids;
>>>>> }
>>>>>
>>>>>
>>>>> static int other_cpus_need_tlb_flush(struct mm_struct *mm)
>>>>> {
>>>>>       /*
>>>>>        * Synchronizes with switch_mm.  Makes sure that we do not
>>>>>        * observe a bit having been cleared in mm_cpumask() before
>>>>>        * the other processor has seen our pagetable update.  See
>>>>>        * switch_mm().
>>>>>        */
>>>>>       smp_mb__after_atomic();
>>>>>
>>>>>       return __other_cpus_need_tlb_flush(mm)
>>>>> }
>>>>>
>>>>> We should be able to deploy other_cpus_need_tlb_flush() in most of the
>>>>> cases where we are doing "cpumask_any_but(mm_cpumask(mm),
>>>>> smp_processor_id()) < nr_cpu_ids".
>>>>
>>>> IMO this is a bit nuts.  smp_mb__after_atomic() doesn't do anything on
>>>> x86.  And, even if it did, why should the flush code assume that the
>>>> previous store was atomic?
>>>>
>>>> What's the issue being fixed / worked around here?
>>>
>>> It does a compiler barrier, which prevents the decision whether a
>>> remote TLB shootdown is required to be made before the PTE is set.
>>>
>>> I agree that PTEs may not be written atomically in certain cases
>>> (although I am unaware of such cases, except on full-mm flush).
>>
>> How about plain set_pte?  It's atomic (aligned word-sized write), but
>> it's not atomic in the _after_atomic sense.
>
> Can you point me to a place where set_pte is used before a TLB
> invalidation/shootdown, excluding this patch and the fullmm case?
>
> I am not claiming there is no such case, but I am unaware of such
> one. PTEs are cleared on SMP using xchg, and similarly the dirty bit
> is cleared with an atomic operation.
>

Hmm, you may be right.  I still think this is all disgusting, but I
don't have any better ideas.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

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

#1422537

Dave Hansen <dave.hansen@linux.intel.com> wrote:

> On 06/14/2016 01:16 PM, Nadav Amit wrote:
>> Dave Hansen <dave.hansen@linux.intel.com> wrote:
>> 
>>> On 06/14/2016 09:47 AM, Nadav Amit wrote:
>>>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:
>>>> 
>>>>>> From: Andi Kleen <ak@linux.intel.com>
>>>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>>>>>> +{
>>>> Here there should be a call to smp_mb__after_atomic() to synchronize with
>>>> switch_mm. I submitted a similar patch, which is still pending (hint).
>>>> 
>>>>>> +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>>>>>> +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>>>>>> +		flush_tlb_others(mm_cpumask(mm), mm, addr,
>>>>>> +				 addr + PAGE_SIZE);
>>>>>> +		mb();
>>>>>> +		set_pte(ptep, __pte(0));
>>>>>> +	}
>>>>>> +}
>>> 
>>> Shouldn't that barrier be incorporated in the TLB flush code itself and
>>> not every single caller (like this code is)?
>>> 
>>> It is insane to require individual TLB flushers to be concerned with the
>>> barriers.
>> 
>> IMHO it is best to use existing flushing interfaces instead of creating
>> new ones.
> 
> Yeah, or make these things a _little_ harder to get wrong.  That little
> snippet above isn't so crazy that we should be depending on open-coded
> barriers to get it right.
> 
> Should we just add a barrier to mm_cpumask() itself?  That should stop
> the race.  Or maybe we need a new primitive like:
> 
> /*
> * Call this if a full barrier has been executed since the last
> * pagetable modification operation.
> */
> static int __other_cpus_need_tlb_flush(struct mm_struct *mm)
> {
> 	/* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */
> 	return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) <
> 		nr_cpu_ids;
> }
> 
> 
> static int other_cpus_need_tlb_flush(struct mm_struct *mm)
> {
> 	/*
> 	 * Synchronizes with switch_mm.  Makes sure that we do not
> 	 * observe a bit having been cleared in mm_cpumask() before
> 	 * the other processor has seen our pagetable update.  See
> 	 * switch_mm().
> 	 */
> 	smp_mb__after_atomic();
> 
> 	return __other_cpus_need_tlb_flush(mm)
> }
> 
> We should be able to deploy other_cpus_need_tlb_flush() in most of the
> cases where we are doing "cpumask_any_but(mm_cpumask(mm),
> smp_processor_id()) < nr_cpu_ids".
> 
> Right?
This approach may work, but I doubt other_cpus_need_tlb_flush() would
be needed by anyone, excluding this "hacky" workaround. There are already
five interfaces for invalidation of: a single page, a userspace range,
a whole task, a kernel range, and full flush including kernel (global)
entries.

> 
>> In theory, fix_pte_leak could have used flush_tlb_page. But the problem
>> is that flush_tlb_page requires the vm_area_struct as an argument, which
>> ptep_get_and_clear (and others) do not have.
> 
> That, and we do not want/need to flush the _current_ processor's TLB.
> flush_tlb_page() would have done that unnecessarily.  That's not the end
> of the world here, but it is a downside.

Oops, I missed the fact a local flush is not needed in this case.

Nadav

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

#1422102

[Multipart message — attachments visible in raw view] — view raw

Hi,

[auto build test ERROR on v4.7-rc3]
[also build test ERROR on next-20160614]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lukasz-Anaczkowski/Linux-VM-workaround-for-Knights-Landing-A-D-leak/20160615-000610
config: i386-randconfig-s0-201624 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/built-in.o: In function `__ptep_modify_prot_start':
>> paravirt.c:(.text+0x346f4): undefined reference to `fix_pte_leak'
   mm/built-in.o: In function `unmap_page_range':
>> (.text+0x1dd4c): undefined reference to `fix_pte_leak'
   mm/built-in.o: In function `move_page_tables':
   (.text+0x274f2): undefined reference to `fix_pte_leak'
   mm/built-in.o: In function `vunmap_page_range':
>> vmalloc.c:(.text+0x2aa78): undefined reference to `fix_pte_leak'
   mm/built-in.o: In function `ptep_clear_flush':
   (.text+0x2cfa7): undefined reference to `fix_pte_leak'
   mm/built-in.o:(.text+0x30dff): more undefined references to `fix_pte_leak' follow

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1422129

...
> +extern void fix_pte_leak(struct mm_struct *mm, unsigned long addr,
> +			 pte_t *ptep);
> +
>  static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>  					    unsigned long addr, pte_t *ptep)
>  {
> -	return ptep_get_and_clear(mm, addr, ptep);
> +	pte_t pte = ptep_get_and_clear(mm, addr, ptep);
> +
> +	if (boot_cpu_has_bug(X86_BUG_PTE_LEAK))
> +		fix_pte_leak(mm, addr, ptep);
> +	return pte;
>  }
>  
>  static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 1a27396..9769355 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -794,11 +794,16 @@ extern int ptep_test_and_clear_young(struct vm_area_struct *vma,
>  extern int ptep_clear_flush_young(struct vm_area_struct *vma,
>  				  unsigned long address, pte_t *ptep);
>  
> +extern void fix_pte_leak(struct mm_struct *mm, unsigned long addr,
> +			 pte_t *ptep);
> +
>  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
>  static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
>  				       pte_t *ptep)
>  {
>  	pte_t pte = native_ptep_get_and_clear(ptep);
> +	if (boot_cpu_has_bug(X86_BUG_PTE_LEAK))
> +		fix_pte_leak(mm, addr, ptep);
>  	pte_update(mm, addr, ptep);
>  	return pte;
>  }

Doesn't hugetlb.h somehow #include pgtable.h?  So why double-define
fix_pte_leak()?

> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
> index 2ee7811..6fa4079 100644
> --- a/arch/x86/include/asm/pgtable_64.h
> +++ b/arch/x86/include/asm/pgtable_64.h
> @@ -178,6 +178,12 @@ extern void cleanup_highmap(void);
>  extern void init_extra_mapping_uc(unsigned long phys, unsigned long size);
>  extern void init_extra_mapping_wb(unsigned long phys, unsigned long size);
>  
> +#define ARCH_HAS_NEEDS_SWAP_PTL 1
> +static inline bool arch_needs_swap_ptl(void)
> +{
> +       return boot_cpu_has_bug(X86_BUG_PTE_LEAK);
> +}
> +
>  #endif /* !__ASSEMBLY__ */

I think we need a comment on this sucker.  I'm not sure we should lean
solely on the commit message to record why we need this until the end of
time.

Or, refer over to fix_pte_leak() for a full description of what is going on.

>  #endif /* _ASM_X86_PGTABLE_64_H */
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 6e2ffbe..f499513 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -181,6 +181,16 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  		}
>  	}
>  
> +	if (c->x86_model == 87) {
> +		static bool printed;
> +
> +		if (!printed) {
> +			pr_info("Enabling PTE leaking workaround\n");
> +			printed = true;
> +		}
> +		set_cpu_bug(c, X86_BUG_PTE_LEAK);
> +	}

Please use the macros in here for the model id:

> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/tree/arch/x86/include/asm/intel-family.h

We also probably want to prefix the pr_info() with something like
"x86/intel:".

> +/*
> + * Workaround for KNL issue:

Please be specific about what this "KNL issue" *is*.  Refer to the
public documentation of the erratum, please.

> + * A thread that is going to page fault due to P=0, may still
> + * non atomically set A or D bits, which could corrupt swap entries.
> + * Always flush the other CPUs and clear the PTE again to avoid
> + * this leakage. We are excluded using the pagetable lock.
> + */
> +
> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
> +{
> +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
> +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
> +		flush_tlb_others(mm_cpumask(mm), mm, addr,
> +				 addr + PAGE_SIZE);
> +		mb();
> +		set_pte(ptep, __pte(0));
> +	}
> +}

I think the comment here is a bit sparse.  Can we add some more details,
like:

	Entering here, the current CPU just cleared the PTE.  But,
	another thread may have raced and set the A or D bits, or be
	_about_ to set the bits.  Shooting their TLB entry down will
	ensure they see the cleared PTE and will not set A or D.

and by the set_pte():

	Clear the PTE one more time, in case the other thread set A/D
	before we sent the TLB flush.

>  #endif /* CONFIG_SMP */
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 5df5feb..5c80fe09 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2404,6 +2404,10 @@ static inline bool debug_guardpage_enabled(void) { return false; }
>  static inline bool page_is_guard(struct page *page) { return false; }
>  #endif /* CONFIG_DEBUG_PAGEALLOC */
>  
> +#ifndef ARCH_HAS_NEEDS_SWAP_PTL
> +static inline bool arch_needs_swap_ptl(void) { return false; }
> +#endif
> +
>  #if MAX_NUMNODES > 1
>  void __init setup_nr_node_ids(void);
>  #else
> diff --git a/mm/memory.c b/mm/memory.c
> index 15322b7..0d6ef39 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1960,7 +1960,8 @@ static inline int pte_unmap_same(struct mm_struct *mm, pmd_t *pmd,
>  {
>  	int same = 1;
>  #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT)
> -	if (sizeof(pte_t) > sizeof(unsigned long)) {
> +	if (arch_needs_swap_ptl() ||
> +	    sizeof(pte_t) > sizeof(unsigned long)) {
>  		spinlock_t *ptl = pte_lockptr(mm, pmd);
>  		spin_lock(ptl);
>  		same = pte_same(*page_table, orig_pte);
> 

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

#1422995

From: Dave Hansen [mailto:dave.hansen@linux.intel.com] 
Sent: Tuesday, June 14, 2016 7:20 PM

>> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
...
>> +extern void fix_pte_leak(struct mm_struct *mm, unsigned long addr,
>> +			 pte_t *ptep);

> Doesn't hugetlb.h somehow #include pgtable.h?  So why double-define
> fix_pte_leak()?

It's other way round - pgtable.h somehow includes hugetlb.h. I've removed
duplicated fix_pte_leak() declaration.

>> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
>> index 2ee7811..6fa4079 100644
>> --- a/arch/x86/include/asm/pgtable_64.h
>> +++ b/arch/x86/include/asm/pgtable_64.h
>> @@ -178,6 +178,12 @@ extern void cleanup_highmap(void);
>>  extern void init_extra_mapping_uc(unsigned long phys, unsigned long size);
>>  extern void init_extra_mapping_wb(unsigned long phys, unsigned long size);
>>  
>> +#define ARCH_HAS_NEEDS_SWAP_PTL 1
>> +static inline bool arch_needs_swap_ptl(void)
>> +{
>> +       return boot_cpu_has_bug(X86_BUG_PTE_LEAK);
>> +}
>> +
>>  #endif /* !__ASSEMBLY__ */

> I think we need a comment on this sucker.  I'm not sure we should lean
> solely on the commit message to record why we need this until the end of
> time.

OK.

>> +	if (c->x86_model == 87) {

> Please use the macros in here for the model id:

OK.

> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/tree/arch/x86/include/asm/intel-family.h

> We also probably want to prefix the pr_info() with something like
> "x86/intel:".

OK

>> +/*
>> + * Workaround for KNL issue:

> Please be specific about what this "KNL issue" *is*. 

OK

>> + * A thread that is going to page fault due to P=0, may still
>> + * non atomically set A or D bits, which could corrupt swap entries.
>> + * Always flush the other CPUs and clear the PTE again to avoid
>> + * this leakage. We are excluded using the pagetable lock.
>> + */
>> +
>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>> +{
>> +	if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) {
>> +		trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
>> +		flush_tlb_others(mm_cpumask(mm), mm, addr,
>> +				 addr + PAGE_SIZE);
>> +		mb();
>> +		set_pte(ptep, __pte(0));
>> +	}
>> +}
>
> I think the comment here is a bit sparse.  Can we add some more details,
> like:
>
>	Entering here, the current CPU just cleared the PTE.  But,
>	another thread may have raced and set the A or D bits, or be
>	_about_ to set the bits.  Shooting their TLB entry down will
>	ensure they see the cleared PTE and will not set A or D.
>
> and by the set_pte():
>
>	Clear the PTE one more time, in case the other thread set A/D
>	before we sent the TLB flush.

Thanks,
Lukasz

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web