Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Wed, 2007-07-11 at 20:18 +0200, Segher Boessenkool wrote:
> Some old software on ppc32 executes from pages it hasn't marked
> executable.  Since "classic" hardware doesn't distinguish between
> execute and read accesses, the do_page_fault() code shouldn't
> either.  This makes glibc-2.2 work again on such hardware.
> 
> Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> ---
> Tested by Scott on 32-bit, glibc-2.2.5 and glibc-2.3.3 (no new
> failures and problem solved), needs confirmation from Johannes
> on his glibc-2.4 "---p" testcase.  Could use testing on ppc64
> and BookE too, for good measure.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

I tested this patch and it does fix the problem I was seeing.

> This reverts the previous change and makes the bugfix behave
> more like the arch/ppc code.
>  arch/powerpc/mm/fault.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 115b25f..5d7add0 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -278,14 +278,17 @@ good_area:
>  		goto bad_area;
>  #endif /* CONFIG_8xx */
>  
> +#ifdef CONFIG_PPC64
>  	if (is_exec) {
> -#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
>  		/* protection fault */
>  		if (error_code & DSISR_PROTFAULT)
>  			goto bad_area;
>  		if (!(vma->vm_flags & VM_EXEC))
>  			goto bad_area;
> -#else
> +	} else
> +		/* A read or write, code continues below...  */
> +#elsif defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> +	if (is_exec) {
>  		pte_t *ptep;
>  		pmd_t *pmdp;
>  
> @@ -310,9 +313,12 @@ good_area:
>  			}
>  			pte_unmap_unlock(ptep, ptl);
>  		}
> +	} else
> +		/* A read or write, code continues below...  */
>  #endif
> -	/* a write */
> -	} else if (is_write) {
> +
> +	/* A read or write.  Classic PPC32 execute is considered a read.  */
> +	if (is_write) {
>  		if (!(vma->vm_flags & VM_WRITE))
>  			goto bad_area;
>  	/* a read */

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Segher Boessenkool]

Some old software on ppc32 executes from pages it hasn't marked
executable.  Since "classic" hardware doesn't distinguish between
execute and read accesses, the do_page_fault() code shouldn't
either.  This makes glibc-2.2 work again on such hardware.

Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
---
[Resend again, ozlabs' greylisting doesn't like me at all.]

Tested by Scott on 32-bit, glibc-2.2.5 and glibc-2.3.3 (no new
failures and problem solved), and by Johannes on his glibc-2.4
"---p" testcase.  Could use testing on ppc64 and BookE too, for
good measure.

This reverts the previous change and makes the bugfix behave
more like the arch/ppc code.
 arch/powerpc/mm/fault.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 115b25f..5d7add0 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -278,14 +278,17 @@ good_area:
 		goto bad_area;
 #endif /* CONFIG_8xx */
 
+#ifdef CONFIG_PPC64
 	if (is_exec) {
-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
 		/* protection fault */
 		if (error_code & DSISR_PROTFAULT)
 			goto bad_area;
 		if (!(vma->vm_flags & VM_EXEC))
 			goto bad_area;
-#else
+	} else
+		/* A read or write, code continues below...  */
+#elsif defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
+	if (is_exec) {
 		pte_t *ptep;
 		pmd_t *pmdp;
 
@@ -310,9 +313,12 @@ good_area:
 			}
 			pte_unmap_unlock(ptep, ptl);
 		}
+	} else
+		/* A read or write, code continues below...  */
 #endif
-	/* a write */
-	} else if (is_write) {
+
+	/* A read or write.  Classic PPC32 execute is considered a read.  */
+	if (is_write) {
 		if (!(vma->vm_flags & VM_WRITE))
 			goto bad_area;
 	/* a read */
-- 
1.5.2.1.144.gabc40-dirty

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Paul Mackerras]

Segher Boessenkool writes:

> Some old software on ppc32 executes from pages it hasn't marked
> executable.  Since "classic" hardware doesn't distinguish between
> execute and read accesses, the do_page_fault() code shouldn't
> either.  This makes glibc-2.2 work again on such hardware.
> 
> Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> ---
> [Resend again, ozlabs' greylisting doesn't like me at all.]
> 
> Tested by Scott on 32-bit, glibc-2.2.5 and glibc-2.3.3 (no new
> failures and problem solved), and by Johannes on his glibc-2.4
> "---p" testcase.  Could use testing on ppc64 and BookE too, for
> good measure.

Hmmm.  The dangling else clauses are pretty gross, and in fact we have
the same problem on POWER3 and RS64 processors (to be fair, we had
the problem before and didn't notice, but we should still fix it).

How about this instead?  Could people test it please?  (Note that
CPU_FTR_NOEXECUTE is 0 in 32-bit kernels.)

Paul.

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 0ece513..99c3093 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -283,7 +283,13 @@ good_area:
 		/* protection fault */
 		if (error_code & DSISR_PROTFAULT)
 			goto bad_area;
-		if (!(vma->vm_flags & VM_EXEC))
+		/*
+		 * Allow execution from readable areas if the MMU does not
+		 * provide separate controls over reading and executing.
+		 */
+		if (!(vma->vm_flags & VM_EXEC) &&
+		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
+		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
 			goto bad_area;
 #else
 		pte_t *ptep;

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Jon Loeliger]

On Wed, 2007-07-18 at 19:00, Paul Mackerras wrote:

> Hmmm.  The dangling else clauses are pretty gross, and in fact we have
> the same problem on POWER3 and RS64 processors (to be fair, we had
> the problem before and didn't notice, but we should still fix it).
> 
> How about this instead?  Could people test it please?  (Note that
> CPU_FTR_NOEXECUTE is 0 in 32-bit kernels.)
> 
> Paul.
> 
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 0ece513..99c3093 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c

Acked-by: Jon Loeliger <jdl@freescale.com>

Tested on 8641HPCN.

Thanks,
jdl

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Segher Boessenkool]

>> Hmmm.  The dangling else clauses are pretty gross, and in fact we  
>> have
>> the same problem on POWER3 and RS64 processors (to be fair, we had
>> the problem before and didn't notice, but we should still fix it).
>>
>> How about this instead?  Could people test it please?  (Note that
>> CPU_FTR_NOEXECUTE is 0 in 32-bit kernels.)
>>
>> Paul.
>>
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0ece513..99c3093 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>
> Acked-by: Jon Loeliger <jdl@freescale.com>
>
> Tested on 8641HPCN.

Which glibc versions?  glibc-2.4 and newer are fine without the
patch already, glibc-2.3 seems to get away by accident; but 2.2
(and before) are the problematic ones.

No other userland program has been identified as needing this
fix, btw -- thankfully, or the testing matrix would be _huge_.


Segher

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Segher Boessenkool]

> Hmmm.  The dangling else clauses are pretty gross,

I hoped you wouldn't notice.  I guess I shouldn't have
commented them :-)

"It was the cleanest thing I could come up with".  Every
other thing I tried ended up as a maze of #ifdefs or some
incomprehensible cross-jumping mess; and I was aiming for
a minimal fix, too.

Or, perhaps, it was just a ploy to trick you into writing
a patch yourself.

> and in fact we have
> the same problem on POWER3 and RS64 processors

Right.  Too bad there is no public documentation for either :-/

> (to be fair, we had
> the problem before and didn't notice, but we should still fix it).

Yeah.

> How about this instead?

It's the better way forward, consider my patch withdrawn :-)

> -		if (!(vma->vm_flags & VM_EXEC))
> +		/*
> +		 * Allow execution from readable areas if the MMU does not
> +		 * provide separate controls over reading and executing.
> +		 */
> +		if (!(vma->vm_flags & VM_EXEC) &&
> +		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
> +		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;

Should you really be testing VM_READ|VM_WRITE, or should it just
be VM_READ?

Oh, and that conditional might benefit from being split into
two separate "if" statements, it's a bit hard to read this way.


Segher

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Jon Loeliger]

On Thu, 2007-07-19 at 12:16, Segher Boessenkool wrote:

> > Tested on 8641HPCN.
> 
> Which glibc versions?  glibc-2.4 and newer are fine without the
> patch already, glibc-2.3 seems to get away by accident; but 2.2
> (and before) are the problematic ones.
> 
> No other userland program has been identified as needing this
> fix, btw -- thankfully, or the testing matrix would be _huge_.
> 
> 
> Segher

Segher,

Needless to say, one that showed the problem beforehand. :-)
But to be precise:

[root:~] ls -lsa /usr/lib/libglib*
   0 lrwxrwxrwx    1 18005314 24012          21 Aug 15  2005 /usr/lib/libglib-1.2.so.0 -> libglib-1.2.so.0.0.10*
 536 -rwxrwxr-x    1 18005314 24012      540931 Apr  8  2003 /usr/lib/libglib-1.2.so.0.0.10*
1064 -rw-rw-r--    1 18005314 24012     1084256 Apr  8  2003 /usr/lib/libglib.a
   4 -rwxrwxr-x    1 18005314 24012         662 Apr  8  2003 /usr/lib/libglib.la*
   0 lrwxrwxrwx    1 18005314 24012          21 Aug 15  2005 /usr/lib/libglib.so -> libglib-1.2.so.0.0.10*


Thanks,
jdl

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Scott Wood]

On Thu, Jul 19, 2007 at 01:57:00PM -0500, Jon Loeliger wrote:
> Needless to say, one that showed the problem beforehand. :-)
> But to be precise:
> 
> [root:~] ls -lsa /usr/lib/libglib*
>    0 lrwxrwxrwx    1 18005314 24012          21 Aug 15  2005 /usr/lib/libglib-1.2.so.0 -> libglib-1.2.so.0.0.10*
>  536 -rwxrwxr-x    1 18005314 24012      540931 Apr  8  2003 /usr/lib/libglib-1.2.so.0.0.10*
> 1064 -rw-rw-r--    1 18005314 24012     1084256 Apr  8  2003 /usr/lib/libglib.a
>    4 -rwxrwxr-x    1 18005314 24012         662 Apr  8  2003 /usr/lib/libglib.la*
>    0 lrwxrwxrwx    1 18005314 24012          21 Aug 15  2005 /usr/lib/libglib.so -> libglib-1.2.so.0.0.10*

glib != glibc.

-Scott

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Jon Loeliger]

On Thu, 2007-07-19 at 14:02, Scott Wood wrote:

> glib != glibc.
> 
> -Scott

D'oh.

So, It doesn't say what version it is.
But it is also dated 8-Apr-2003.

jdl

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Kumar Gala]

On Jul 19, 2007, at 2:10 PM, Jon Loeliger wrote:

> On Thu, 2007-07-19 at 14:02, Scott Wood wrote:
>
>> glib != glibc.
>>
>> -Scott
>
> D'oh.
>
> So, It doesn't say what version it is.
> But it is also dated 8-Apr-2003.

The glibc jdl tested against is 2.2.5.

- k

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Paul Mackerras]

Segher Boessenkool writes:

> Should you really be testing VM_READ|VM_WRITE, or should it just
> be VM_READ?

We test VM_READ | VM_WRITE | VM_EXEC in the read case below, and that
is because we have no HPTE encoding to say "writable but not readable"
or "executable but not readable".  Similarly we have no encoding to
say "writable but not executable" on classic processors, so if you
have just VM_WRITE set, you get a page that is readable, writable and
executable.

Paul.

Re: [PATCH] Treat ISI faults as read faults on classic 32-bit PowerPC

[Segher Boessenkool]

>> Should you really be testing VM_READ|VM_WRITE, or should it just
>> be VM_READ?
>
> We test VM_READ | VM_WRITE | VM_EXEC in the read case below, and that
> is because we have no HPTE encoding to say "writable but not readable"
> or "executable but not readable".  Similarly we have no encoding to
> say "writable but not executable" on classic processors, so if you
> have just VM_WRITE set, you get a page that is readable, writable and
> executable.

Ah yes.  I thought "executable requires readable", but
that is with the CPU its flags, not the Linux flags.

Would it be a good idea to map Linux flags to CPU flags
somewhere early in this function?  It might simplify some
code, and things certainly would become more readable.


Segher