[PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[Li Yang]

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
+
 	return __ioremap_caller(addr, size, flags, __builtin_return_address(0));
 }
 EXPORT_SYMBOL(ioremap_flags);
-- 
1.6.6-rc1.GIT

Re: [PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[Benjamin Herrenschmidt]

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.

Re: [PATCH] powerpc: fix ioremap_flags() with book3e pte definition

[Kumar Gala]

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.
>>=20
>> Signed-off-by: Li Yang <leoli@freescale.com>
>> ---
>> arch/powerpc/mm/pgtable_32.c |    5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>=20
>> 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 &=3D ~(_PAGE_USER | _PAGE_EXEC);
>>=20
>> +#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
>> +	/* supervisor read permission has just been cleared, add back */
>> +	flags |=3D _PAGE_BAP_SR;
>> +#endif
>> +
>=20
> So this is a bit fishy indeed. pgtable_64.c seems to have the same
> problem in fact.
>=20
> 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 ?
>=20
> _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] powerpc: Fix ioremap_flags() with book3e pte definition

[Kumar Gala]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

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.

BenH: Slightly simplified the ifdef and applied to ppc64 too

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
For 2.6.34 stable commit 55052eeca6d71d76f7c3f156c0501814d8e5e6d3

 arch/powerpc/mm/pgtable_32.c |    8 ++++++++
 arch/powerpc/mm/pgtable_64.c |    8 ++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index b9243e7..767b0cf 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -146,6 +146,14 @@ 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);
 
+#ifdef _PAGE_BAP_SR
+	/* _PAGE_USER contains _PAGE_BAP_SR on BookE using the new PTE format
+	 * which means that we just cleared supervisor access... oops ;-) This
+	 * restores it
+	 */
+	flags |= _PAGE_BAP_SR;
+#endif
+
 	return __ioremap_caller(addr, size, flags, __builtin_return_address(0));
 }
 EXPORT_SYMBOL(ioremap_flags);
diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index d95679a..d050fc8 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c
@@ -265,6 +265,14 @@ void __iomem * ioremap_flags(phys_addr_t addr, unsigned long size,
 	/* we don't want to let _PAGE_USER and _PAGE_EXEC leak out */
 	flags &= ~(_PAGE_USER | _PAGE_EXEC);
 
+#ifdef _PAGE_BAP_SR
+	/* _PAGE_USER contains _PAGE_BAP_SR on BookE using the new PTE format
+	 * which means that we just cleared supervisor access... oops ;-) This
+	 * restores it
+	 */
+	flags |= _PAGE_BAP_SR;
+#endif
+
 	if (ppc_md.ioremap)
 		return ppc_md.ioremap(addr, size, flags, caller);
 	return __ioremap_caller(addr, size, flags, caller);
-- 
1.6.0.6

Re: [PATCH] powerpc: Fix ioremap_flags() with book3e pte definition

[Kumar Gala]

On May 24, 2010, at 1:38 PM, Kumar Gala wrote:

> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>=20
> 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.
>=20
> BenH: Slightly simplified the ifdef and applied to ppc64 too
>=20
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> For 2.6.34 stable commit 55052eeca6d71d76f7c3f156c0501814d8e5e6d3
>=20
> arch/powerpc/mm/pgtable_32.c |    8 ++++++++
> arch/powerpc/mm/pgtable_64.c |    8 ++++++++
> 2 files changed, 16 insertions(+), 0 deletions(-)

Wondering what happened to this patch (and 2 other) getting into stable =
queue for .34.x

- k=

Re: [PATCH] powerpc: Fix ioremap_flags() with book3e pte definition

[Greg KH]

On Wed, Jun 02, 2010 at 04:59:16PM -0500, Kumar Gala wrote:
> 
> On May 24, 2010, at 1:38 PM, Kumar Gala wrote:
> 
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > 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.
> > 
> > BenH: Slightly simplified the ifdef and applied to ppc64 too
> > 
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > For 2.6.34 stable commit 55052eeca6d71d76f7c3f156c0501814d8e5e6d3
> > 
> > arch/powerpc/mm/pgtable_32.c |    8 ++++++++
> > arch/powerpc/mm/pgtable_64.c |    8 ++++++++
> > 2 files changed, 16 insertions(+), 0 deletions(-)
> 
> Wondering what happened to this patch (and 2 other) getting into stable queue for .34.x

They are still in the "to-apply" queue.  Please be patient, I was busy
with the .35-rc1 merge window and now am catching up on the stable
trees.  There's over 130 patches that people seem to want in the first
.34 stable release, so it's taking some time to get them all in and
tested.

thanks,

greg k-h