* [PATCH v2] MIPS: Remove race window in page fault handling
@ 2014-08-08 13:47 Lars Persson
2014-08-08 16:55 ` David Daney
0 siblings, 1 reply; 8+ messages in thread
From: Lars Persson @ 2014-08-08 13:47 UTC (permalink / raw
To: ralf; +Cc: linux-mips, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] MIPS: Remove race window in page fault handling
2014-08-08 13:47 [PATCH v2] MIPS: Remove race window in page fault handling Lars Persson
@ 2014-08-08 16:55 ` David Daney
2014-08-08 20:47 ` Ralf Baechle
0 siblings, 1 reply; 8+ messages in thread
From: David Daney @ 2014-08-08 16:55 UTC (permalink / raw
To: Lars Persson; +Cc: ralf, linux-mips, Lars Persson
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);
> +}
> +
> +
>
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] MIPS: Remove race window in page fault handling
2014-08-08 16:55 ` David Daney
@ 2014-08-08 20:47 ` Ralf Baechle
2014-08-15 8:03 ` Lars Persson
0 siblings, 1 reply; 8+ messages in thread
From: Ralf Baechle @ 2014-08-08 20:47 UTC (permalink / raw
To: David Daney; +Cc: Lars Persson, linux-mips, Lars Persson
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] MIPS: Remove race window in page fault handling
2014-08-08 20:47 ` Ralf Baechle
@ 2014-08-15 8:03 ` Lars Persson
2014-08-15 11:01 ` Ralf Baechle
0 siblings, 1 reply; 8+ messages in thread
From: Lars Persson @ 2014-08-15 8:03 UTC (permalink / raw
To: Ralf Baechle; +Cc: David Daney, linux-mips@linux-mips.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] MIPS: Remove race window in page fault handling
2014-08-15 8:03 ` Lars Persson
@ 2014-08-15 11:01 ` Ralf Baechle
2014-08-15 12:08 ` Lars Persson
0 siblings, 1 reply; 8+ messages in thread
From: Ralf Baechle @ 2014-08-15 11:01 UTC (permalink / raw
To: Lars Persson; +Cc: David Daney, linux-mips@linux-mips.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] MIPS: Remove race window in page fault handling
2014-08-15 11:01 ` Ralf Baechle
@ 2014-08-15 12:08 ` Lars Persson
2014-08-19 15:22 ` Ralf Baechle
2014-08-19 15:47 ` Ralf Baechle
0 siblings, 2 replies; 8+ messages in thread
From: Lars Persson @ 2014-08-15 12:08 UTC (permalink / raw
To: Ralf Baechle; +Cc: David Daney, linux-mips@linux-mips.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] MIPS: Remove race window in page fault handling
2014-08-15 12:08 ` Lars Persson
@ 2014-08-19 15:22 ` Ralf Baechle
2014-08-19 15:47 ` Ralf Baechle
1 sibling, 0 replies; 8+ messages in thread
From: Ralf Baechle @ 2014-08-19 15:22 UTC (permalink / raw
To: Lars Persson; +Cc: David Daney, linux-mips@linux-mips.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] MIPS: Remove race window in page fault handling
2014-08-15 12:08 ` Lars Persson
2014-08-19 15:22 ` Ralf Baechle
@ 2014-08-19 15:47 ` Ralf Baechle
1 sibling, 0 replies; 8+ messages in thread
From: Ralf Baechle @ 2014-08-19 15:47 UTC (permalink / raw
To: Lars Persson; +Cc: David Daney, linux-mips@linux-mips.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-08-19 15:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-08 13:47 [PATCH v2] MIPS: Remove race window in page fault handling Lars Persson
2014-08-08 16:55 ` David Daney
2014-08-08 20:47 ` Ralf Baechle
2014-08-15 8:03 ` Lars Persson
2014-08-15 11:01 ` Ralf Baechle
2014-08-15 12:08 ` Lars Persson
2014-08-19 15:22 ` Ralf Baechle
2014-08-19 15:47 ` Ralf Baechle
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.