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

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

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

diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
index 008324d..7b175e5 100644
--- a/arch/mips/include/asm/pgtable.h
+++ b/arch/mips/include/asm/pgtable.h
@@ -95,6 +95,9 @@ extern void paging_init(void);
 
 #define pmd_page_vaddr(pmd)	pmd_val(pmd)
 
+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))
@@ -118,7 +121,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)
 {
@@ -155,7 +157,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)
 {
@@ -169,6 +170,18 @@ 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);
+
+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
+	pte_t *ptep, pte_t pteval)
+{
+	/* Make code globally visible before publishing the page
+	   table entry. */
+	if (addr < TASK_SIZE && pte_present(pteval))
+		mips_flush_dcache_from_pte(pteval);
+	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.)
diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index 9e67cde..1320afc 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -137,6 +137,16 @@ void __update_cache(struct vm_area_struct *vma, unsigned long address,
 	}
 }
 
+void mips_flush_dcache_from_pte(pte_t pteval)
+{
+	unsigned long pfn = pte_pfn(pteval);
+	if (likely(pfn_valid(pfn))) {
+		struct page *page = pfn_to_page(pfn);
+		if (Page_dcache_dirty(page))
+			flush_dcache_page(page);
+	}
+}
+
 unsigned long _page_cachable_default;
 EXPORT_SYMBOL(_page_cachable_default);
 
-- 
1.7.10.4

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

[David Daney]

On 05/31/2014 03:36 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.
>

Shouldn't this only be done on machines that suffer from the problem?

There are many SMP MIPS machines that don't need this, so they shouldn't 
have to pay the price for doing this.

David Daney



> Signed-off-by: Lars Persson <larper@axis.com>
> ---
>   arch/mips/include/asm/pgtable.h |   17 +++++++++++++++--
>   arch/mips/mm/cache.c            |   10 ++++++++++
>   2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
> index 008324d..7b175e5 100644
> --- a/arch/mips/include/asm/pgtable.h
> +++ b/arch/mips/include/asm/pgtable.h
> @@ -95,6 +95,9 @@ extern void paging_init(void);
>
>   #define pmd_page_vaddr(pmd)	pmd_val(pmd)
>
> +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))
> @@ -118,7 +121,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)
>   {
> @@ -155,7 +157,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)
>   {
> @@ -169,6 +170,18 @@ 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);
> +
> +static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> +	pte_t *ptep, pte_t pteval)
> +{
> +	/* Make code globally visible before publishing the page
> +	   table entry. */
> +	if (addr < TASK_SIZE && pte_present(pteval))
> +		mips_flush_dcache_from_pte(pteval);
> +	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.)
> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
> index 9e67cde..1320afc 100644
> --- a/arch/mips/mm/cache.c
> +++ b/arch/mips/mm/cache.c
> @@ -137,6 +137,16 @@ void __update_cache(struct vm_area_struct *vma, unsigned long address,
>   	}
>   }
>
> +void mips_flush_dcache_from_pte(pte_t pteval)
> +{
> +	unsigned long pfn = pte_pfn(pteval);
> +	if (likely(pfn_valid(pfn))) {
> +		struct page *page = pfn_to_page(pfn);
> +		if (Page_dcache_dirty(page))
> +			flush_dcache_page(page);
> +	}
> +}
> +
>   unsigned long _page_cachable_default;
>   EXPORT_SYMBOL(_page_cachable_default);
>
>

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

[Lars Persson]

Hi

Good point. Would adding !cpu_has_ic_fills_f_dc as an extra condition in set_pte_at be sufficient to address your concern ?

BR,
 Lars

> -----Original Message-----
> From: David Daney [mailto:ddaney.cavm@gmail.com]
> Sent: den 2 juni 2014 18:48
> To: Lars Persson
> Cc: linux-mips@linux-mips.org; Lars Persson
> Subject: Re: [PATCH] MIPS: Remove race window in page fault handling
> 
> On 05/31/2014 03:36 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.
> >
> 
> Shouldn't this only be done on machines that suffer from the problem?
> 
> There are many SMP MIPS machines that don't need this, so they
> shouldn't have to pay the price for doing this.
> 
> David Daney
> 
> 

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

[Ralf Baechle]

On Tue, Jun 03, 2014 at 12:29:17PM +0200, Lars Persson wrote:

> Good point. Would adding !cpu_has_ic_fills_f_dc as an extra condition in set_pte_at be sufficient to address your concern ?

Returning to this old thread ...

cpu_has_ic_fills_f_dc means a CPU's data cache does not need to be
written back to secondary cache or memory when instructions have been
updated in memory.  In other words a CPU can refill the I-cache from
the D-cache on an I-cache miss.

Only the Alchemy cores have this handy property.

The flag is also being set for XL? CPUs in

  arch/mips/include/asm/mach-netlogic/cpu-feature-overrides.h

but not anywhere in the probe code of arch/mips/mm/c-r4k.c which might
stil be correct - but it's at least a bit sloppy and suspicious so I'm
wondering if it's correct indeed.  Jayachandran?

  Ralf

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

[Jayachandran C.]

On Wed, Dec 03, 2014 at 02:37:29PM +0100, Ralf Baechle wrote:
> On Tue, Jun 03, 2014 at 12:29:17PM +0200, Lars Persson wrote:
> 
> > Good point. Would adding !cpu_has_ic_fills_f_dc as an extra condition in set_pte_at be sufficient to address your concern ?
> 
> Returning to this old thread ...
> 
> cpu_has_ic_fills_f_dc means a CPU's data cache does not need to be
> written back to secondary cache or memory when instructions have been
> updated in memory.  In other words a CPU can refill the I-cache from
> the D-cache on an I-cache miss.
> 
> Only the Alchemy cores have this handy property.
> 
> The flag is also being set for XL? CPUs in
> 
>   arch/mips/include/asm/mach-netlogic/cpu-feature-overrides.h
> 
> but not anywhere in the probe code of arch/mips/mm/c-r4k.c which might
> stil be correct - but it's at least a bit sloppy and suspicious so I'm
> wondering if it's correct indeed.  Jayachandran?

On XLR/XLP, L1 Dcache is coherent and L1 Icache is not. We don't need to
flush dcache when we change an executable page or range, just an icache
flush is sufficient. At least in c-r4k.c, setting cpu_has_ic_fills_f_dc
did what we needed.

There were 2 issues, one is the usage in copy_to_user_page, for which
a patch was posted http://patchwork.linux-mips.org/patch/6122/
(we are using this patch). Another is that we might need a "sync" before
the icache flush.

JC.