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

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 <redacted>
Cc: Scott Wood <redacted>
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

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 <redacted>
Cc: Scott Wood <redacted>
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;

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

Acked-by: Jon Loeliger <redacted>

Tested on 8641HPCN.

Thanks,
jdl

quoted

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 <redacted>

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

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

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

quoted

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

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

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

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

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

quoted

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

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.

quoted

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