diff mbox

pte_fsl_booke: fix instruction TLB error permission check

Message ID 1273221514-8683-1-git-send-email-leoli@freescale.com (mailing list archive)
State Accepted, archived
Commit fa6bd996db2fbecd7a9a408c158105c55a51fe41
Delegated to: Kumar Gala
Headers show

Commit Message

Yang Li May 7, 2010, 8:38 a.m. UTC 
Check the user/supervisor execution permission base on the code address.
This fixes the following oops on module loading or removing.

Unable to handle kernel paging request for instruction fetch
Faulting instruction address: 0xf938d040
Oops: Kernel access of bad area, sig: 11 [#1]

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Jin Qing <b24347@freescale.com>
---
 arch/powerpc/kernel/head_fsl_booke.S |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

Comments

Kumar Gala May 13, 2010, 7:14 p.m. UTC | #1 
On May 7, 2010, at 3:38 AM, Li Yang wrote:

> Check the user/supervisor execution permission base on the code address.
> This fixes the following oops on module loading or removing.
> 
> Unable to handle kernel paging request for instruction fetch
> Faulting instruction address: 0xf938d040
> Oops: Kernel access of bad area, sig: 11 [#1]
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Jin Qing <b24347@freescale.com>
> ---
> arch/powerpc/kernel/head_fsl_booke.S |   13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)

applied to merge.

I've updated the commit message to be a bit more clear on why we need to do this.

Thanks for figuring this out.

- k
Benjamin Herrenschmidt May 13, 2010, 9:46 p.m. UTC | #2 
On Thu, 2010-05-13 at 14:14 -0500, Kumar Gala wrote:
> On May 7, 2010, at 3:38 AM, Li Yang wrote:
> 
> > Check the user/supervisor execution permission base on the code address.
> > This fixes the following oops on module loading or removing.
> > 
> > Unable to handle kernel paging request for instruction fetch
> > Faulting instruction address: 0xf938d040
> > Oops: Kernel access of bad area, sig: 11 [#1]
> > 
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Jin Qing <b24347@freescale.com>
> > ---
> > arch/powerpc/kernel/head_fsl_booke.S |   13 ++++++++++---
> > 1 files changed, 10 insertions(+), 3 deletions(-)
> 
> applied to merge.
> 
> I've updated the commit message to be a bit more clear on why we need to do this.

Not looking at the code right now ... but do we have the same issue on
64e ?

Cheers,
Ben.
Liu Dave-R63238 May 14, 2010, 12:53 a.m. UTC | #3 
> I've updated the commit message to be a bit more clear on why 
> we need to do this.

I'm curious why the _PAGE_EXEC have different definition in pte-book3e.h
and pte-fsl-booke.h?

It is UX permission in pte-book3e, but is SX permission in
pte-fsl-booke.h.

Thanks, Dave
Liu Dave-R63238 May 14, 2010, 2:16 a.m. UTC | #4 
> Not looking at the code right now ... but do we have the same 
> issue on 64e ?

Aaron pointed the issue on FSL BookE.
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079738.html
Benjamin Herrenschmidt May 14, 2010, 3:36 a.m. UTC | #5 
On Thu, 2010-05-13 at 14:14 -0500, Kumar Gala wrote:
> On May 7, 2010, at 3:38 AM, Li Yang wrote:
> 
> > Check the user/supervisor execution permission base on the code address.
> > This fixes the following oops on module loading or removing.
> > 
> > Unable to handle kernel paging request for instruction fetch
> > Faulting instruction address: 0xf938d040
> > Oops: Kernel access of bad area, sig: 11 [#1]
> > 
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Jin Qing <b24347@freescale.com>
> > ---
> > arch/powerpc/kernel/head_fsl_booke.S |   13 ++++++++++---
> > 1 files changed, 10 insertions(+), 3 deletions(-)
> 
> applied to merge.
> 
> I've updated the commit message to be a bit more clear on why we need to do this.
> 
> Thanks for figuring this out.

Should I pull & send to Linus ?

Cheers,
Ben.
Benjamin Herrenschmidt May 14, 2010, 3:42 a.m. UTC | #6 
On Fri, 2010-05-14 at 08:53 +0800, Liu Dave-R63238 wrote:
> > I've updated the commit message to be a bit more clear on why 
> > we need to do this.
> 
> I'm curious why the _PAGE_EXEC have different definition in pte-book3e.h
> and pte-fsl-booke.h?
> 
> It is UX permission in pte-book3e, but is SX permission in
> pte-fsl-booke.h.

Oh well, there's a whole history here :-)

I'm not 100% sure of the "old" fsl-booke, but pte-book3e uses UX since
that's what _PAGE_EXEC really represents from a kernel standpoint,
user-execute.

On book3e, we need to account for the HW tablewalk, which means that the
PTE can be loaded as-is by the HW. We thus don't get a chance to test
whether we are user or supervisor or change the bits accordingly. We
thus need _PAGE_EXEC to fall right into UX since that's really what the
kernel manipulates it for.

It also means that user pages will not get execute permission in
supervisor mode which is a good thing :-)

For kernel-execute, we have a specific permission, which should be
obtained via PAGE_KERNEL_EXEC in vmalloc_exec().

Cheers,
Ben.
Kumar Gala May 14, 2010, 4:19 a.m. UTC | #7 
On May 13, 2010, at 10:36 PM, Benjamin Herrenschmidt wrote:

> On Thu, 2010-05-13 at 14:14 -0500, Kumar Gala wrote:
>> On May 7, 2010, at 3:38 AM, Li Yang wrote:
>> 
>>> Check the user/supervisor execution permission base on the code address.
>>> This fixes the following oops on module loading or removing.
>>> 
>>> Unable to handle kernel paging request for instruction fetch
>>> Faulting instruction address: 0xf938d040
>>> Oops: Kernel access of bad area, sig: 11 [#1]
>>> 
>>> Signed-off-by: Li Yang <leoli@freescale.com>
>>> Signed-off-by: Jin Qing <b24347@freescale.com>
>>> ---
>>> arch/powerpc/kernel/head_fsl_booke.S |   13 ++++++++++---
>>> 1 files changed, 10 insertions(+), 3 deletions(-)
>> 
>> applied to merge.
>> 
>> I've updated the commit message to be a bit more clear on why we need to do this.
>> 
>> Thanks for figuring this out.
> 
> Should I pull & send to Linus ?

No, I've got another patch to fix a FTRACE issue with SMP CPU bringup.  Will post a tree for you here shortly.

- k
Benjamin Herrenschmidt May 14, 2010, 9:08 a.m. UTC | #8 
On Fri, 2010-05-14 at 10:16 +0800, Liu Dave-R63238 wrote:
> > Not looking at the code right now ... but do we have the same 
> > issue on 64e ?
> 
> Aaron pointed the issue on FSL BookE.
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079738.html

Right, but I think 64-bit BookE has the same issue. We can fix that
later tho.

Cheers
Ben
Liu Dave-R63238 May 14, 2010, 9:14 a.m. UTC | #9 
> > Aaron pointed the issue on FSL BookE.
> > 
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079738.htm
> > l
> 
> Right, but I think 64-bit BookE has the same issue. We can 
> fix that later tho.

Ben,

We also find the _PAGE_USER is different between pte-book3e and old
fsl-booke
It is UR in old fsl-booke, and is _PAGE_BAP_UR | _PAGE_BAP_SR in
pte-book3e,
which will cause the ioremap_flags issue.

Did you have a test your commit
[ea3cc330ac0cd521ff07c7cd432a1848c19a7e92]?

Thanks, Dave
Benjamin Herrenschmidt May 14, 2010, 9:25 a.m. UTC | #10 
On Fri, 2010-05-14 at 17:14 +0800, Liu Dave-R63238 wrote:
> > > Aaron pointed the issue on FSL BookE.
> > > 
> > http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079738.htm
> > > l
> > 
> > Right, but I think 64-bit BookE has the same issue. We can 
> > fix that later tho.
> 
> Ben,
> 
> We also find the _PAGE_USER is different between pte-book3e and old
> fsl-booke
> It is UR in old fsl-booke, and is _PAGE_BAP_UR | _PAGE_BAP_SR in
> pte-book3e,
> which will cause the ioremap_flags issue.
> 
> Did you have a test your commit
> [ea3cc330ac0cd521ff07c7cd432a1848c19a7e92]?

I tested on some things :-) I keep asking Kumar to send me some e500
gear so I can actually test on it :-)

Cheers,
Ben.
diff mbox

Patch

diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
index 78aac7b..61b6687 100644
--- a/arch/powerpc/kernel/head_fsl_booke.S
+++ b/arch/powerpc/kernel/head_fsl_booke.S
@@ -684,6 +684,13 @@  interrupt_base:
 	rlwinm	r12,r12,0,16,1
 	mtspr	SPRN_MAS1,r12
 
+	/* Make up the required permissions for kernel code */
+#ifdef CONFIG_PTE_64BIT
+	li	r13,_PAGE_PRESENT | _PAGE_BAP_SX
+	oris	r13,r13,_PAGE_ACCESSED@h
+#else
+	li	r13,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
+#endif
 	b	4f
 
 	/* Get the PGD for the current thread */
@@ -691,15 +698,15 @@  interrupt_base:
 	mfspr	r11,SPRN_SPRG_THREAD
 	lwz	r11,PGDIR(r11)
 
-4:
-	/* Make up the required permissions */
+	/* Make up the required permissions for user code */
 #ifdef CONFIG_PTE_64BIT
-	li	r13,_PAGE_PRESENT | _PAGE_EXEC
+	li	r13,_PAGE_PRESENT | _PAGE_BAP_UX
 	oris	r13,r13,_PAGE_ACCESSED@h
 #else
 	li	r13,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
 #endif
 
+4:
 	FIND_PTE
 	andc.	r13,r13,r11		/* Check permission */