[PATCH v2] MIPS: Remove race window in page fault handling

[PATCH v2] MIPS: Remove race window in page fault handling

[Lars Persson]

Multicore MIPSes without I/D hardware coherency suffered from a race
condition in the page fault handler. The page table entry was
published before any pending lazy D-cache flush was committed, hence
it allowed execution of stale page cache data by other VPEs in the
system.

To make the cache handling safe we need to perform flushing already in
the set_pte_at function. MIPSes without coherent I-caches can get a
small increase in flushes due to the unavailability of the execute
flag in set_pte_at.

Signed-off-by: Lars Persson <larper@axis.com>
---
 arch/mips/include/asm/pgtable.h |   22 +++++++++++++++++-----
 arch/mips/mm/cache.c            |   16 ++++++++--------
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
index 027c74d..1834298 100644
--- a/arch/mips/include/asm/pgtable.h
+++ b/arch/mips/include/asm/pgtable.h
@@ -122,6 +122,9 @@ do {									\
 	}								\
 } while(0)
 
+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
+	pte_t *ptep, pte_t pteval);
+
 #if defined(CONFIG_64BIT_PHYS_ADDR) && defined(CONFIG_CPU_MIPS32)
 
 #define pte_none(pte)		(!(((pte).pte_low | (pte).pte_high) & ~_PAGE_GLOBAL))
@@ -145,7 +148,6 @@ static inline void set_pte(pte_t *ptep, pte_t pte)
 		}
 	}
 }
-#define set_pte_at(mm, addr, ptep, pteval) set_pte(ptep, pteval)
 
 static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
 {
@@ -183,7 +185,6 @@ static inline void set_pte(pte_t *ptep, pte_t pteval)
 	}
 #endif
 }
-#define set_pte_at(mm, addr, ptep, pteval) set_pte(ptep, pteval)
 
 static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
 {
@@ -198,6 +199,20 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
 }
 #endif
 
+extern void mips_flush_dcache_from_pte(pte_t pteval, unsigned long address);
+
+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
+	pte_t *ptep, pte_t pteval)
+{
+	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc) {
+		if (pte_present(pteval))
+			mips_flush_dcache_from_pte(pteval, addr);
+	}
+
+	set_pte(ptep, pteval);
+}
+
+
 /*
  * (pmds are folded into puds so this doesn't get actually called,
  * but the define is needed for a generic inline function.)
@@ -390,15 +405,12 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 
 extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
 	pte_t pte);
-extern void __update_cache(struct vm_area_struct *vma, unsigned long address,
-	pte_t pte);
 
 static inline void update_mmu_cache(struct vm_area_struct *vma,
 	unsigned long address, pte_t *ptep)
 {
 	pte_t pte = *ptep;
 	__update_tlb(vma, address, pte);
-	__update_cache(vma, address, pte);
 }
 
 static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index f7b91d3..0d0eb04 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -119,21 +119,21 @@ void __flush_anon_page(struct page *page, unsigned long vmaddr)
 
 EXPORT_SYMBOL(__flush_anon_page);
 
-void __update_cache(struct vm_area_struct *vma, unsigned long address,
-	pte_t pte)
+void mips_flush_dcache_from_pte(pte_t pteval, unsigned long address)
 {
 	struct page *page;
-	unsigned long pfn, addr;
-	int exec = (vma->vm_flags & VM_EXEC) && !cpu_has_ic_fills_f_dc;
+	unsigned long pfn = pte_pfn(pteval);
 
-	pfn = pte_pfn(pte);
 	if (unlikely(!pfn_valid(pfn)))
 		return;
+
 	page = pfn_to_page(pfn);
 	if (page_mapping(page) && Page_dcache_dirty(page)) {
-		addr = (unsigned long) page_address(page);
-		if (exec || pages_do_alias(addr, address & PAGE_MASK))
-			flush_data_cache_page(addr);
+		unsigned long page_addr = (unsigned long) page_address(page);
+
+		if (!cpu_has_ic_fills_f_dc ||
+		    pages_do_alias(page_addr, address & PAGE_MASK))
+			flush_data_cache_page(page_addr);
 		ClearPageDcacheDirty(page);
 	}
 }
-- 
1.7.10.4

Re: [PATCH v2] MIPS: Remove race window in page fault handling

[David Daney]

On 08/08/2014 06:47 AM, Lars Persson wrote:
> Multicore MIPSes without I/D hardware coherency suffered from a race
> condition in the page fault handler. The page table entry was
> published before any pending lazy D-cache flush was committed, hence
> it allowed execution of stale page cache data by other VPEs in the
> system.
>
> To make the cache handling safe we need to perform flushing already in
> the set_pte_at function. MIPSes without coherent I-caches can get a
> small increase in flushes due to the unavailability of the execute
> flag in set_pte_at.
>
> Signed-off-by: Lars Persson <larper@axis.com>
> ---
>   arch/mips/include/asm/pgtable.h |   22 +++++++++++++++++-----
>   arch/mips/mm/cache.c            |   16 ++++++++--------
>   2 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
> index 027c74d..1834298 100644
> --- a/arch/mips/include/asm/pgtable.h
> +++ b/arch/mips/include/asm/pgtable.h
> @@ -122,6 +122,9 @@ do {									\
>   	}								\
>   } while(0)
>
> +static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> +	pte_t *ptep, pte_t pteval);
> +

Is it possible to reorder the code such that this declaration is not 
necessary?


>   #if defined(CONFIG_64BIT_PHYS_ADDR) && defined(CONFIG_CPU_MIPS32)
>
>   #define pte_none(pte)		(!(((pte).pte_low | (pte).pte_high) & ~_PAGE_GLOBAL))
> @@ -145,7 +148,6 @@ static inline void set_pte(pte_t *ptep, pte_t pte)
>   		}
>   	}
>   }
> -#define set_pte_at(mm, addr, ptep, pteval) set_pte(ptep, pteval)
>
>   static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>   {
> @@ -183,7 +185,6 @@ static inline void set_pte(pte_t *ptep, pte_t pteval)
>   	}
>   #endif
>   }
> -#define set_pte_at(mm, addr, ptep, pteval) set_pte(ptep, pteval)
>
>   static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
>   {
> @@ -198,6 +199,20 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
>   }
>   #endif
>
> +extern void mips_flush_dcache_from_pte(pte_t pteval, unsigned long address);
> +
> +static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> +	pte_t *ptep, pte_t pteval)
> +{
> +	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc) {
> +		if (pte_present(pteval))
> +			mips_flush_dcache_from_pte(pteval, addr);
> +	}
> +
> +	set_pte(ptep, pteval);
> +}
> +
> +
>
[...]

Re: [PATCH v2] MIPS: Remove race window in page fault handling

[Ralf Baechle]

On Fri, Aug 08, 2014 at 09:55:00AM -0700, David Daney wrote:

> >+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> >+	pte_t *ptep, pte_t pteval);
> >+
> 
> Is it possible to reorder the code such that this declaration is not
> necessary?

That's not as obvious as one might think initially.  set_pte_at needs
to be defined after set_pte but before clear_pte which is calling set_pte_at.

Of both set_pte and clear_pte there are two #ifdefd variants.

set_pte_at is a fairly small function only but it's invoked quite a few
times so I was a little concerned about the effect on I'm experimenting with
outlining set_pte_at entirely.  ip22_defconfig with the patch applied as
posted; this is the effect on code size.

  text    data     bss     dec     hex filename
3790118  175304   84544 4049966  3dcc2e vmlinux		as posted
3789062	 175304	  84544	4048910	 3dc80e	vmlinux		set_pte_at outlined

  Ralf

Re: [PATCH v2] MIPS: Remove race window in page fault handling

[Lars Persson]

On fre, 2014-08-08 at 22:47 +0200, Ralf Baechle wrote:
> On Fri, Aug 08, 2014 at 09:55:00AM -0700, David Daney wrote:
> 
> > >+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> > >+	pte_t *ptep, pte_t pteval);
> > >+
> > 
> > Is it possible to reorder the code such that this declaration is not
> > necessary?
> 
> That's not as obvious as one might think initially.  set_pte_at needs
> to be defined after set_pte but before clear_pte which is calling set_pte_at.
> 
> Of both set_pte and clear_pte there are two #ifdefd variants.
> 
> set_pte_at is a fairly small function only but it's invoked quite a few
> times so I was a little concerned about the effect on I'm experimenting with
> outlining set_pte_at entirely.  ip22_defconfig with the patch applied as
> posted; this is the effect on code size.
> 
>   text    data     bss     dec     hex filename
> 3790118  175304   84544 4049966  3dcc2e vmlinux		as posted
> 3789062	 175304	  84544	4048910	 3dc80e	vmlinux		set_pte_at outlined
> 
>   Ralf

Hi Ralf

Should I update the patch with outlined set_pte_at ?

Best Regards,
 Lars

Re: [PATCH v2] MIPS: Remove race window in page fault handling

[Ralf Baechle]

On Fri, Aug 15, 2014 at 10:03:47AM +0200, Lars Persson wrote:

> 
> On fre, 2014-08-08 at 22:47 +0200, Ralf Baechle wrote:
> > On Fri, Aug 08, 2014 at 09:55:00AM -0700, David Daney wrote:
> > 
> > > >+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> > > >+	pte_t *ptep, pte_t pteval);
> > > >+
> > > 
> > > Is it possible to reorder the code such that this declaration is not
> > > necessary?
> > 
> > That's not as obvious as one might think initially.  set_pte_at needs
> > to be defined after set_pte but before clear_pte which is calling set_pte_at.
> > 
> > Of both set_pte and clear_pte there are two #ifdefd variants.
> > 
> > set_pte_at is a fairly small function only but it's invoked quite a few
> > times so I was a little concerned about the effect on I'm experimenting with
> > outlining set_pte_at entirely.  ip22_defconfig with the patch applied as
> > posted; this is the effect on code size.
> > 
> >   text    data     bss     dec     hex filename
> > 3790118  175304   84544 4049966  3dcc2e vmlinux		as posted
> > 3789062	 175304	  84544	4048910	 3dc80e	vmlinux		set_pte_at outlined
> > 
> >   Ralf
> 
> Hi Ralf
> 
> Should I update the patch with outlined set_pte_at ?

Not necessary; I've already done that myself.  I was just waiting for
comments.

Thanks for your work on tracking this down.  I wonder, how did you discover
this issue?

  Ralf

Re: [PATCH v2] MIPS: Remove race window in page fault handling

[Lars Persson]

On fre, 2014-08-15 at 13:01 +0200, Ralf Baechle wrote:
> On Fri, Aug 15, 2014 at 10:03:47AM +0200, Lars Persson wrote:
> 
> > 
> > On fre, 2014-08-08 at 22:47 +0200, Ralf Baechle wrote:
> > > On Fri, Aug 08, 2014 at 09:55:00AM -0700, David Daney wrote:
> > > 
> > > > >+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> > > > >+	pte_t *ptep, pte_t pteval);
> > > > >+
> > > > 
> > > > Is it possible to reorder the code such that this declaration is not
> > > > necessary?
> > > 
> > > That's not as obvious as one might think initially.  set_pte_at needs
> > > to be defined after set_pte but before clear_pte which is calling set_pte_at.
> > > 
> > > Of both set_pte and clear_pte there are two #ifdefd variants.
> > > 
> > > set_pte_at is a fairly small function only but it's invoked quite a few
> > > times so I was a little concerned about the effect on I'm experimenting with
> > > outlining set_pte_at entirely.  ip22_defconfig with the patch applied as
> > > posted; this is the effect on code size.
> > > 
> > >   text    data     bss     dec     hex filename
> > > 3790118  175304   84544 4049966  3dcc2e vmlinux		as posted
> > > 3789062	 175304	  84544	4048910	 3dc80e	vmlinux		set_pte_at outlined
> > > 
> > >   Ralf
> > 
> > Hi Ralf
> > 
> > Should I update the patch with outlined set_pte_at ?
> 
> Not necessary; I've already done that myself.  I was just waiting for
> comments.
> 
> Thanks for your work on tracking this down.  I wonder, how did you discover
> this issue?

This one was tricky to track down. We had sporadic SIGILLs in
multi-threaded apps for a long time. Eventually we got a test case that
triggered more page cache evictions and the frequency of SIGILLs
increased enough to catch it with a JTAG debugger. 

Kernel call stacks showed one thread handling an illegal instruction
exception while another thread was somewhere around the
set_pte_at/update_mmu_cache calls for the same page.

BR,
 Lars

Re: [PATCH v2] MIPS: Remove race window in page fault handling

[Ralf Baechle]

On Fri, Aug 15, 2014 at 02:08:56PM +0200, Lars Persson wrote:

> This one was tricky to track down. We had sporadic SIGILLs in
> multi-threaded apps for a long time. Eventually we got a test case that
> triggered more page cache evictions and the frequency of SIGILLs
> increased enough to catch it with a JTAG debugger. 
> 
> Kernel call stacks showed one thread handling an illegal instruction
> exception while another thread was somewhere around the
> set_pte_at/update_mmu_cache calls for the same page.

Some of those coherency bugs are almost impossibly hard to track down
and fix properly!

Anyway, I'm going to send the outlined version of your fix to Linus.

  Ralf

Re: [PATCH v2] MIPS: Remove race window in page fault handling

[Ralf Baechle]

On Fri, Aug 15, 2014 at 02:08:56PM +0200, Lars Persson wrote:

Btw, this patch applies with minor variations all the way back to
at least 2.6.12 and in even older kernel set_pte_at() doesn't exist
yet ...

  Ralf