[git pull] x86 page fault checker

[git pull] x86 page fault checker

[Steven Rostedt]

Ingo,

This is not an urgent fix, but I based it on your urgent branch.
The patch keeps the page fault handler from entering an infinite loop
if the PMD does not match the PTE, and the PTE has the correct
permissions but the PMD does not.

With your latest change, this should not happen again. But if there's
some other code out there that does have this bug, or if some future
change creates it (never know with all the changes in virtualization)
Perhaps it is still a good idea to have this check.

This is not a fast path, and it should not hurt to have this level
of paranoia.

-- Steve

Please pull the latest tip/x86/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/x86/urgent


Steven Rostedt (1):
      x86: check PMD in spurious_fault handler

----
 arch/x86/mm/fault.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
---------------------------
commit 8ef2333f1bdcc4a43cb37b1b5d8febf8e3d8cdc7
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Thu Feb 19 11:46:36 2009 -0500

    x86: check PMD in spurious_fault handler
    
    Impact: fix to prevent hard lockup on bad PMD permissions
    
    If the PMD does not have the correct permissions for a page access,
    but the PTE does, the spurious fault handler will mistake the fault
    as a lazy TLB transaction. This will result in an infinite loop of:
    
     fault -> spurious_fault check (pass) -> return to code -> fault
    
    This patch adds a check and a warn on if the PTE passes the permissions
    but the PMD does not.
    
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index c76ef1d..7b579a6 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -455,6 +455,7 @@ static int spurious_fault(unsigned long address,
 	pud_t *pud;
 	pmd_t *pmd;
 	pte_t *pte;
+	int ret;
 
 	/* Reserved-bit violation or user access to kernel space? */
 	if (error_code & (PF_USER | PF_RSVD))
@@ -482,7 +483,17 @@ static int spurious_fault(unsigned long address,
 	if (!pte_present(*pte))
 		return 0;
 
-	return spurious_fault_check(error_code, pte);
+	ret = spurious_fault_check(error_code, pte);
+	if (!ret)
+		return 0;
+
+	/*
+	 * Make sure we have permissions in PMD
+	 * If not, then there's a bug in the page tables.
+	 */
+	ret = spurious_fault_check(error_code, (pte_t *) pmd);
+	WARN_ON(!ret);
+	return ret;
 }
 
 /*

Re: [git pull] x86 page fault checker

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> This is not an urgent fix, but I based it on your urgent 
> branch. The patch keeps the page fault handler from entering 
> an infinite loop if the PMD does not match the PTE, and the 
> PTE has the correct permissions but the PMD does not.
> 
> With your latest change, this should not happen again. But if 
> there's some other code out there that does have this bug, or 
> if some future change creates it (never know with all the 
> changes in virtualization) Perhaps it is still a good idea to 
> have this check.
> 
> This is not a fast path, and it should not hurt to have this 
> level of paranoia.
> 
> -- Steve
> 
> Please pull the latest tip/x86/urgent tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/x86/urgent
> 
> 
> Steven Rostedt (1):
>       x86: check PMD in spurious_fault handler
> 
> ----
>  arch/x86/mm/fault.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> ---------------------------
> commit 8ef2333f1bdcc4a43cb37b1b5d8febf8e3d8cdc7
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Thu Feb 19 11:46:36 2009 -0500
> 
>     x86: check PMD in spurious_fault handler
>     
>     Impact: fix to prevent hard lockup on bad PMD permissions
>     
>     If the PMD does not have the correct permissions for a page access,
>     but the PTE does, the spurious fault handler will mistake the fault
>     as a lazy TLB transaction. This will result in an infinite loop of:
>     
>      fault -> spurious_fault check (pass) -> return to code -> fault
>     
>     This patch adds a check and a warn on if the PTE passes the permissions
>     but the PMD does not.
>     
>     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index c76ef1d..7b579a6 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -455,6 +455,7 @@ static int spurious_fault(unsigned long address,
>  	pud_t *pud;
>  	pmd_t *pmd;
>  	pte_t *pte;
> +	int ret;
>  
>  	/* Reserved-bit violation or user access to kernel space? */
>  	if (error_code & (PF_USER | PF_RSVD))
> @@ -482,7 +483,17 @@ static int spurious_fault(unsigned long address,
>  	if (!pte_present(*pte))
>  		return 0;
>  
> -	return spurious_fault_check(error_code, pte);
> +	ret = spurious_fault_check(error_code, pte);
> +	if (!ret)
> +		return 0;
> +
> +	/*
> +	 * Make sure we have permissions in PMD
> +	 * If not, then there's a bug in the page tables.
> +	 */
> +	ret = spurious_fault_check(error_code, (pte_t *) pmd);
> +	WARN_ON(!ret);
> +	return ret;
>  }

i guess we could do this - but i'd rather have it as a 
WARN_ONCE(), with some text - so that if it ever triggers it's 
one surgical message.

	Ingo

Re: [git pull] x86 page fault checker

[Steven Rostedt]

On Fri, 20 Feb 2009, Ingo Molnar wrote:
> > -	return spurious_fault_check(error_code, pte);
> > +	ret = spurious_fault_check(error_code, pte);
> > +	if (!ret)
> > +		return 0;
> > +
> > +	/*
> > +	 * Make sure we have permissions in PMD
> > +	 * If not, then there's a bug in the page tables.
> > +	 */
> > +	ret = spurious_fault_check(error_code, (pte_t *) pmd);
> > +	WARN_ON(!ret);
> > +	return ret;
> >  }
> 
> i guess we could do this - but i'd rather have it as a 
> WARN_ONCE(), with some text - so that if it ever triggers it's 
> one surgical message.

OK, I'll make this update and rebase it.

-- Steve

[git pull v2] x86 page fault checker

[Steven Rostedt]

Ingo,

I added your suggestion, and even tested it out. I cherry picked
a branch to remove your split_large fix, added this output, and then
added the changes that triggered the original bug. I got a very nasty 
warning, but the kernel resumed fine.

------------[ cut here ]------------
WARNING: at arch/x86/mm/fault.c:495 do_page_fault+0x2aa/0x890()
Hardware name: Precision WorkStation 470   
PMD has incorrect permission bits
[...]


Thanks,

-- Steve

Please pull the latest tip/x86/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/x86/urgent


Steven Rostedt (1):
      x86: check PMD in spurious_fault handler

----
 arch/x86/mm/fault.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
---------------------------
commit 3c3e5694add02e665bbbd0fecfbbdcc0b903097a
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Thu Feb 19 11:46:36 2009 -0500

    x86: check PMD in spurious_fault handler
    
    Impact: fix to prevent hard lockup on bad PMD permissions
    
    If the PMD does not have the correct permissions for a page access,
    but the PTE does, the spurious fault handler will mistake the fault
    as a lazy TLB transaction. This will result in an infinite loop of:
    
     fault -> spurious_fault check (pass) -> return to code -> fault
    
    This patch adds a check and a warn on if the PTE passes the permissions
    but the PMD does not.
    
    [ Updated: Ingo Molnar suggested using WARN_ONCE with some text ]
    
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index c76ef1d..278d645 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -455,6 +455,7 @@ static int spurious_fault(unsigned long address,
 	pud_t *pud;
 	pmd_t *pmd;
 	pte_t *pte;
+	int ret;
 
 	/* Reserved-bit violation or user access to kernel space? */
 	if (error_code & (PF_USER | PF_RSVD))
@@ -482,7 +483,17 @@ static int spurious_fault(unsigned long address,
 	if (!pte_present(*pte))
 		return 0;
 
-	return spurious_fault_check(error_code, pte);
+	ret = spurious_fault_check(error_code, pte);
+	if (!ret)
+		return 0;
+
+	/*
+	 * Make sure we have permissions in PMD
+	 * If not, then there's a bug in the page tables.
+	 */
+	ret = spurious_fault_check(error_code, (pte_t *) pmd);
+	WARN_ONCE(!ret, "PMD has incorrect permission bits\n");
+	return ret;
 }
 
 /*

Re: [git pull v2] x86 page fault checker

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> I added your suggestion, and even tested it out. I cherry picked
> a branch to remove your split_large fix, added this output, and then
> added the changes that triggered the original bug. I got a very nasty 
> warning, but the kernel resumed fine.

Pulled into tip:x86/mm. It's not urgent in terms of .29 
material, agreed?

> ------------[ cut here ]------------
> WARNING: at arch/x86/mm/fault.c:495 do_page_fault+0x2aa/0x890()
> Hardware name: Precision WorkStation 470   
> PMD has incorrect permission bits
> [...]

Tested trees are much apprecitated! :-)

	Ingo

Re: [git pull v2] x86 page fault checker

[Steven Rostedt]

On Fri, 20 Feb 2009, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Ingo,
> > 
> > I added your suggestion, and even tested it out. I cherry picked
> > a branch to remove your split_large fix, added this output, and then
> > added the changes that triggered the original bug. I got a very nasty 
> > warning, but the kernel resumed fine.
> 
> Pulled into tip:x86/mm. It's not urgent in terms of .29 
> material, agreed?

Yeah, it is just an added precaution. It can wait till .30

-- Steve