Patchwork powerpc: fix ioremap_flags() with book3e pte definition

login
register
mail settings
Submitter Yang Li
Date Feb. 4, 2010, 4:53 p.m.
Message ID <1265302382-15142-1-git-send-email-leoli@freescale.com>
Download mbox | patch
Permalink /patch/44533/
State Accepted
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Yang Li - Feb. 4, 2010, 4:53 p.m.
We can't just clear the user read permission in book3e pte, because
that will also clear supervisor read permission.  This surely isn't
desired.  Fix the problem by adding the supervisor read back.

Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/mm/pgtable_32.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Benjamin Herrenschmidt - Feb. 8, 2010, 4:21 a.m.
On Fri, 2010-02-05 at 00:53 +0800, Li Yang wrote:
> We can't just clear the user read permission in book3e pte, because
> that will also clear supervisor read permission.  This surely isn't
> desired.  Fix the problem by adding the supervisor read back.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
>  arch/powerpc/mm/pgtable_32.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> index cb96cb2..aff7c04 100644
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -144,6 +144,11 @@ ioremap_flags(phys_addr_t addr, unsigned long size, unsigned long flags)
>  	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
>  	flags &= ~(_PAGE_USER | _PAGE_EXEC);
>  
> +#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
> +	/* supervisor read permission has just been cleared, add back */
> +	flags |= _PAGE_BAP_SR;
> +#endif
> +

So this is a bit fishy indeed. pgtable_64.c seems to have the same
problem in fact.

It boils down to the "hack" I have in the new PTE format which consists
of having _SR be part of _PAGE_USER. I wonder if I should change that
instead. Kumar, what do you reckon ?

_Maybe_ an option is to have _PAGE_NO_USER that defaults to be
~_PAGE_USER in pte-common.h and can be set in pte-book3e.h to
only strip out UR and UW ?

Cheers,
Ben.
Kumar Gala - May 14, 2010, 5:57 p.m.
On Feb 7, 2010, at 10:21 PM, Benjamin Herrenschmidt wrote:

> On Fri, 2010-02-05 at 00:53 +0800, Li Yang wrote:
>> We can't just clear the user read permission in book3e pte, because
>> that will also clear supervisor read permission.  This surely isn't
>> desired.  Fix the problem by adding the supervisor read back.
>> 
>> Signed-off-by: Li Yang <leoli@freescale.com>
>> ---
>> arch/powerpc/mm/pgtable_32.c |    5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
>> index cb96cb2..aff7c04 100644
>> --- a/arch/powerpc/mm/pgtable_32.c
>> +++ b/arch/powerpc/mm/pgtable_32.c
>> @@ -144,6 +144,11 @@ ioremap_flags(phys_addr_t addr, unsigned long size, unsigned long flags)
>> 	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
>> 	flags &= ~(_PAGE_USER | _PAGE_EXEC);
>> 
>> +#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
>> +	/* supervisor read permission has just been cleared, add back */
>> +	flags |= _PAGE_BAP_SR;
>> +#endif
>> +
> 
> So this is a bit fishy indeed. pgtable_64.c seems to have the same
> problem in fact.
> 
> It boils down to the "hack" I have in the new PTE format which consists
> of having _SR be part of _PAGE_USER. I wonder if I should change that
> instead. Kumar, what do you reckon ?
> 
> _Maybe_ an option is to have _PAGE_NO_USER that defaults to be
> ~_PAGE_USER in pte-common.h and can be set in pte-book3e.h to
> only strip out UR and UW ?

We never closed on this.  I think lets add _PAGE_NO_USER

- k

Patch

diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index cb96cb2..aff7c04 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -144,6 +144,11 @@  ioremap_flags(phys_addr_t addr, unsigned long size, unsigned long flags)
 	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
 	flags &= ~(_PAGE_USER | _PAGE_EXEC);
 
+#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
+	/* supervisor read permission has just been cleared, add back */
+	flags |= _PAGE_BAP_SR;
+#endif
+
 	return __ioremap_caller(addr, size, flags, __builtin_return_address(0));
 }
 EXPORT_SYMBOL(ioremap_flags);