[patch 1/1] make PROT_WRITE imply PROT_READ

[patch 1/1] make PROT_WRITE imply PROT_READ

[akpm]

From: Jason Baron <jbaron@redhat.com>

(akpm: needs to be split into nine patches and fed through arch trees. 
Later.)

(err, make it's easier to just collect some acks?)

Make PROT_WRITE imply PROT_READ for a number of architectures which don't
support write only in hardware.

While looking at this, I noticed that some architectures which do not
support write only mappings already take the exact same approach.  For
example, in arch/alpha/mm/fault.c:

"
        if (cause < 0) {
                if (!(vma->vm_flags & VM_EXEC))
                        goto bad_area;
        } else if (!cause) {
                /* Allow reads even for write-only mappings */
                if (!(vma->vm_flags & (VM_READ | VM_WRITE)))
                        goto bad_area;
        } else {
                if (!(vma->vm_flags & VM_WRITE))
                        goto bad_area;
        }
"

Thus, this patch brings other architectures which do not support write only
mappings in-line and consistent with the rest.  I've verified the patch on
ia64, x86_64 and x86.


Additional discussion:

Several architectures, including x86, can not support write-only mappings. 
The pte for x86 reserves a single bit for protection and its two states are
read only or read/write.  Thus, write only is not supported in h/w.  

Currently, if i 'mmap' a page write-only, the first read attempt on that page
creates a page fault and will SEGV.  That check is enforced in
arch/blah/mm/fault.c.  However, if i first write that page it will fault in
and the pte will be set to read/write.  Thus, any subsequent reads to the page
will succeed.  It is this inconsistency in behavior that this patch is
attempting to address.  Furthermore, if the page is swapped out, and then
brought back the first read will also cause a SEGV.  Thus, any arbitrary read
on a page can potentially result in a SEGV.

According to the SuSv3 spec, "if the application requests only PROT_WRITE, the
implementation may also allow read access." Also as mentioned, some
archtectures, such as alpha, shown above already take the approach that i am
suggesting.

The counter-argument to this raised by Arjan, is that the kernel is enforcing
the write only mapping the best it can given the h/w limitations.  This is
true, however Alan Cox, and myself would argue that the inconsitency in
behavior, that is applications can sometimes work/sometimes fails is highly
undesireable.  If you read through the thread, i think people, came to an
agreement on the last patch i posted, as nobody has objected to it...

Signed-off-by: Jason Baron <jbaron@redhat.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: "Luck, Tony" <tony.luck@intel.com>
Cc: Hugh Dickins <hugh@veritas.com>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Andi Kleen <ak@muc.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Kazumoto Kojima <kkojima@rr.iij4u.or.jp>
Cc: Ian Molton <spyro@f2s.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 arch/arm/mm/fault.c     |    2 +-
 arch/arm26/mm/fault.c   |    2 +-
 arch/i386/mm/fault.c    |    2 +-
 arch/ia64/mm/fault.c    |    6 ++++--
 arch/m68k/mm/fault.c    |    2 +-
 arch/powerpc/mm/fault.c |    2 +-
 arch/ppc/mm/fault.c     |    2 +-
 arch/sh/mm/fault.c      |    2 +-
 arch/x86_64/mm/fault.c  |    2 +-
 9 files changed, 12 insertions(+), 10 deletions(-)

diff -puN arch/arm/mm/fault.c~make-prot_write-imply-prot_read arch/arm/mm/fault.c
--- a/arch/arm/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/arm/mm/fault.c
@@ -170,7 +170,7 @@ good_area:
 	if (fsr & (1 << 11)) /* write? */
 		mask = VM_WRITE;
 	else
-		mask = VM_READ|VM_EXEC;
+		mask = VM_READ|VM_EXEC|VM_WRITE;
 
 	fault = VM_FAULT_BADACCESS;
 	if (!(vma->vm_flags & mask))
diff -puN arch/arm26/mm/fault.c~make-prot_write-imply-prot_read arch/arm26/mm/fault.c
--- a/arch/arm26/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/arm26/mm/fault.c
@@ -155,7 +155,7 @@ __do_page_fault(struct mm_struct *mm, un
 	 */
 good_area:
 	if (READ_FAULT(fsr)) /* read? */
-		mask = VM_READ|VM_EXEC;
+		mask = VM_READ|VM_EXEC|VM_WRITE;
 	else
 		mask = VM_WRITE;
 
diff -puN arch/i386/mm/fault.c~make-prot_write-imply-prot_read arch/i386/mm/fault.c
--- a/arch/i386/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/i386/mm/fault.c
@@ -440,7 +440,7 @@ good_area:
 		case 1:		/* read, present */
 			goto bad_area;
 		case 0:		/* read, not present */
-			if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
+			if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
 				goto bad_area;
 	}
 
diff -puN arch/ia64/mm/fault.c~make-prot_write-imply-prot_read arch/ia64/mm/fault.c
--- a/arch/ia64/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/ia64/mm/fault.c
@@ -146,9 +146,11 @@ ia64_do_page_fault (unsigned long addres
 #		error File is out of sync with <linux/mm.h>.  Please update.
 #	endif
 
+	if (((isr >> IA64_ISR_R_BIT) & 1UL) && (!(vma->vm_flags & (VM_READ | VM_WRITE))))
+		goto bad_area;
+
 	mask = (  (((isr >> IA64_ISR_X_BIT) & 1UL) << VM_EXEC_BIT)
-		| (((isr >> IA64_ISR_W_BIT) & 1UL) << VM_WRITE_BIT)
-		| (((isr >> IA64_ISR_R_BIT) & 1UL) << VM_READ_BIT));
+		| (((isr >> IA64_ISR_W_BIT) & 1UL) << VM_WRITE_BIT));
 
 	if ((vma->vm_flags & mask) != mask)
 		goto bad_area;
diff -puN arch/m68k/mm/fault.c~make-prot_write-imply-prot_read arch/m68k/mm/fault.c
--- a/arch/m68k/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/m68k/mm/fault.c
@@ -144,7 +144,7 @@ good_area:
 		case 1:		/* read, present */
 			goto acc_err;
 		case 0:		/* read, not present */
-			if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
+			if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
 				goto acc_err;
 	}
 
diff -puN arch/powerpc/mm/fault.c~make-prot_write-imply-prot_read arch/powerpc/mm/fault.c
--- a/arch/powerpc/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/powerpc/mm/fault.c
@@ -333,7 +333,7 @@ good_area:
 		/* protection fault */
 		if (error_code & 0x08000000)
 			goto bad_area;
-		if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
+		if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
 			goto bad_area;
 	}
 
diff -puN arch/ppc/mm/fault.c~make-prot_write-imply-prot_read arch/ppc/mm/fault.c
--- a/arch/ppc/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/ppc/mm/fault.c
@@ -239,7 +239,7 @@ good_area:
 		/* protection fault */
 		if (error_code & 0x08000000)
 			goto bad_area;
-		if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
+		if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
 			goto bad_area;
 	}
 
diff -puN arch/sh/mm/fault.c~make-prot_write-imply-prot_read arch/sh/mm/fault.c
--- a/arch/sh/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/sh/mm/fault.c
@@ -80,7 +80,7 @@ good_area:
 		if (!(vma->vm_flags & VM_WRITE))
 			goto bad_area;
 	} else {
-		if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
+		if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
 			goto bad_area;
 	}
 
diff -puN arch/x86_64/mm/fault.c~make-prot_write-imply-prot_read arch/x86_64/mm/fault.c
--- a/arch/x86_64/mm/fault.c~make-prot_write-imply-prot_read
+++ a/arch/x86_64/mm/fault.c
@@ -464,7 +464,7 @@ good_area:
 		case PF_PROT:		/* read, present */
 			goto bad_area;
 		case 0:			/* read, not present */
-			if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
+			if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
 				goto bad_area;
 	}
 
_

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Arjan van de Ven]

> Additional discussion:
> 
> Several architectures, including x86, can not support write-only mappings. 

actually on x86 the kernel can support it, just that it's highly expensive in
terms of performance so we don't currently do it. We could if we wanted to though.

This patch will create a userspace ABI precedent that will hurt us if we ever
decide to implement this, or if Intel or AMD add support for this to the cpu...

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Alan Cox]

Ar Gwe, 2006-09-15 am 06:53 +0200, ysgrifennodd Arjan van de Ven:
> This patch will create a userspace ABI precedent that will hurt us if we ever
> decide to implement this, or if Intel or AMD add support for this to the cpu...

You mean like noexec didn't. It's just a question of binary type if such
a mapping type comes into existance. At the moment the behaviour is
essentially random.

Alan

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Geert Uytterhoeven]

On Thu, 14 Sep 2006 akpm@osdl.org wrote:
> According to the SuSv3 spec, "if the application requests only PROT_WRITE,
> the implementation may also allow read access." Also as mentioned, some
> archtectures, such as alpha, shown above already take the approach that i am
> suggesting.
>
> The counter-argument to this raised by Arjan, is that the kernel is enforcing
> the write only mapping the best it can given the h/w limitations.  This is
> true, however Alan Cox, and myself would argue that the inconsitency in
> behavior, that is applications can sometimes work/sometimes fails is highly
> undesireable.  If you read through the thread, i think people, came to an
> agreement on the last patch i posted, as nobody has objected to it...

The same can be said about not initializing local variables, using delete vs.
delete [], ...: sometimes it works, sometimes it fails. It's still an
application bug to rely on `may' behavior.

So I tend to agree with Arjan.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Alan Cox]

Ar Gwe, 2006-09-15 am 10:06 +0200, ysgrifennodd Geert Uytterhoeven:
> The same can be said about not initializing local variables, using delete vs.
> delete [], ...: sometimes it works, sometimes it fails. It's still an
> application bug to rely on `may' behavior.

And as a result programmers have to resort to tools like valgrind. Even
valgrind can't save you in this case and AFAIK there is no debugging
tool for this problem.

Programming is *hard* (correct programming), don't make it any harder
when it is easy to make it more predictable. Murphy's law says that the
user who hits the actual bug will be on the other side of the globe,
busy and not speak the same language...

Alan

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Andi Kleen]

On Thu, Sep 14, 2006 at 06:39:47PM -0700, akpm@osdl.org wrote:
> From: Jason Baron <jbaron@redhat.com>
> 
> (akpm: needs to be split into nine patches and fed through arch trees. 
> Later.)
> 
> (err, make it's easier to just collect some acks?)

x86_64 part is ok for me.

-Andi

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Alan Cox]

Ar Iau, 2006-09-14 am 18:39 -0700, ysgrifennodd akpm@osdl.org:
> From: Jason Baron <jbaron@redhat.com>

Acked-by: Alan Cox <alan@redhat.com>

> Signed-off-by: Jason Baron <jbaron@redhat.com>

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Ralf Baechle]

On Thu, Sep 14, 2006 at 06:39:47PM -0700, akpm@osdl.org wrote:

> (akpm: needs to be split into nine patches and fed through arch trees. 
> Later.)
> 
> (err, make it's easier to just collect some acks?)

MIPS (with the exception of the 4KSd) is also affected; I've just commited
below patch.

  Ralf

diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index e3a6172..a4f8c45 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -89,7 +89,7 @@ good_area:
 		if (!(vma->vm_flags & VM_WRITE))
 			goto bad_area;
 	} else {
-		if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
+		if (!(vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)))
 			goto bad_area;
 	}
 

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Paul Mundt]

On Thu, Sep 14, 2006 at 06:39:47PM -0700, akpm@osdl.org wrote:
> From: Jason Baron <jbaron@redhat.com>
> 
> (akpm: needs to be split into nine patches and fed through arch trees. 
> Later.)
> 
> (err, make it's easier to just collect some acks?)
> 
sh part is fine.

Acked-by: Paul Mundt <lethal@linux-sh.org>

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Chris Wedgwood]

On Thu, Sep 14, 2006 at 06:39:47PM -0700, akpm@osdl.org wrote:

> Make PROT_WRITE imply PROT_READ for a number of architectures which
> don't support write only in hardware.

Why don't we WARN where PROT_WRITE is used w/o PROT_READ?  Do
non-trivial or non-contrived applications really use PROT_WRITE and
assume reads are OK?

It seems once we do this there will be little or no chance of ever
doing write-only mappings should we want to in the future using this
mechanism.

We could just update the definition of PROT_WRITE in the userspace
headers...

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Arjan van de Ven]

Chris Wedgwood wrote:
> On Thu, Sep 14, 2006 at 06:39:47PM -0700, akpm@osdl.org wrote:
> 
>> Make PROT_WRITE imply PROT_READ for a number of architectures which
>> don't support write only in hardware.
> 
> Why don't we WARN where PROT_WRITE is used w/o PROT_READ?  Do
> non-trivial or non-contrived applications really use PROT_WRITE and
> assume reads are OK?
> 
> It seems once we do this there will be little or no chance of ever
> doing write-only mappings should we want to in the future using this
> mechanism.
> 
> We could just update the definition of PROT_WRITE in the userspace
> headers...

btw PROT_WRITE does make sense in principle for MMIO mappings, especially
uncachable ones. Not per se on native hardware, but in the hardware enabled
virtualization (Intel VT or AMD-V) case this suddenly becomes very easily
enforcable and probably even worth enforcing (since in that case the hypervisor
traps on each access to the memory and simulates the instruction anyway;
only writes means it's a lot simpler in terms of IOMMU and cache coherency etc
etc)

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Alan Cox]

Ar Sul, 2006-09-17 am 13:24 -0700, ysgrifennodd Chris Wedgwood:
> On Thu, Sep 14, 2006 at 06:39:47PM -0700, akpm@osdl.org wrote:
> 
> > Make PROT_WRITE imply PROT_READ for a number of architectures which
> > don't support write only in hardware.
> 
> Why don't we WARN where PROT_WRITE is used w/o PROT_READ?  Do
> non-trivial or non-contrived applications really use PROT_WRITE and
> assume reads are OK?

Unfortunately yes. This was discovered in the real world.

> It seems once we do this there will be little or no chance of ever
> doing write-only mappings should we want to in the future using this
> mechanism.

Executable types already let us handle that, or as you suggest but the
other way around you add PROT_REALLYLIKEWRITEONLYOK as a new mmap type
(and as PROT_WRITE) for a new binary format later if the CPU ever
supports it. Frankly I think the odds of Intel cpus growing write-only
are remote....

Alan

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Arjan van de Ven]

Alan Cox wrote:

> Frankly I think the odds of Intel cpus growing write-only
> are remote....

in the VT virtualization case it's there already basically...
(at least for MMIO regions, for memory regions it's probably not
done for performance reasons)

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Chris Wedgwood]

On Sun, Sep 17, 2006 at 10:14:06PM +0100, Alan Cox wrote:

> Unfortunately yes. This was discovered in the real world.

Are there a lot of these applications and does it break badly?  Do you
know which ones they are?

It's not an intuitive change and as the current behavior isn't
entirely consistent and (presumably) these applications work, so is
this change really needed?

Fixing userspace assumptions but setting a (counter-intuitive)
precedent in the kernel seems wrong.

> Executable types already let us handle that, or as you suggest but
> the other way around you add PROT_REALLYLIKEWRITEONLYOK as a new
> mmap type (and as PROT_WRITE) for a new binary format later if the
> CPU ever supports it.

True, but it's more complexity further down the line.

> Frankly I think the odds of Intel cpus growing write-only are
> remote....

Even with VT/Pacifica?  Might those not provide support to catch reads
and writes to certain regions?  At some point I imagine there will be
in-kernel awareness of those technologies when the kernel isn't
running uder a hypervisor to make things like running a VM inside
Linux easier (I would assume vmware is/has done this already perhaps).

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Paul Mackerras]

Chris Wedgwood writes:

> Are there a lot of these applications and does it break badly?  Do you
> know which ones they are?
> 
> It's not an intuitive change and as the current behavior isn't
> entirely consistent and (presumably) these applications work, so is
> this change really needed?

I think the problem is that these applications break randomly,
depending on exactly when page faults happen, and these patches make
the behaviour consistent and deterministic.

Paul.

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Chris Wedgwood]

On Mon, Sep 18, 2006 at 12:03:51PM +1000, Paul Mackerras wrote:

> I think the problem is that these applications break randomly,

which means they are broken surely?

> depending on exactly when page faults happen, and these patches make
> the behaviour consistent and deterministic.

so would detecting PROT_WRITE w/o PROT_READ and returning -EINVAL
surely then?

it would still be broken so no loss there and we wouldn't have to make
such a long term change

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Geert Uytterhoeven]

On Sun, 17 Sep 2006, Chris Wedgwood wrote:
> On Mon, Sep 18, 2006 at 12:03:51PM +1000, Paul Mackerras wrote:
> > I think the problem is that these applications break randomly,
> 
> which means they are broken surely?
> 
> > depending on exactly when page faults happen, and these patches make
> > the behaviour consistent and deterministic.
> 
> so would detecting PROT_WRITE w/o PROT_READ and returning -EINVAL
> surely then?
> 
> it would still be broken so no loss there and we wouldn't have to make
> such a long term change

That would break apps that use PROT_WRITE correctly (i.e. for writing only).

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [patch 1/1] make PROT_WRITE imply PROT_READ

[Alan Cox]

Ar Sul, 2006-09-17 am 17:57 -0700, ysgrifennodd Chris Wedgwood:
> On Sun, Sep 17, 2006 at 10:14:06PM +0100, Alan Cox wrote:
> 
> > Unfortunately yes. This was discovered in the real world.
> 
> Are there a lot of these applications and does it break badly?  Do you
> know which ones they are?

I don't know precisely which applications but Red Hat has customers
hitting the random behaviour. By choice I'd like that to turn into
consistent failure but its outside the hardware capabilities right now.

> It's not an intuitive change and as the current behavior isn't
> entirely consistent and (presumably) these applications work, so is
> this change really needed?
> 
> Fixing userspace assumptions but setting a (counter-intuitive)
> precedent in the kernel seems wrong.

The current behaviour is "randomly fails". The expected behaviour is
"consistently fails" or "consistently succeeds". POSIX explicitly says
that passing just PROT_WRITE may give you a readable mapping (just as
PROT_READ may give you exec in turn).

The change would thus be logically consistent and in keeping with the
standard.

> Even with VT/Pacifica?  

For VT at the moment the answer appears to be no. 

Alan