Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Peter Horton]

The current MIPS 2.4 kernel (from CVS) currently allows fixed shared
mappings to violate D-cache aliasing constraints.

The check for illegal fixed mappings is done in
arch_get_unmapped_area(), but these mappings are granted in
get_unmapped_area() and arch_get_unmapped_area() is never called.

A quick look at sparc and sparc64 seem to show the same problem.

P.

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Ralf Baechle]

On Sat, Dec 13, 2003 at 11:41:34AM +0000, Peter Horton wrote:

> The current MIPS 2.4 kernel (from CVS) currently allows fixed shared
> mappings to violate D-cache aliasing constraints.
> 
> The check for illegal fixed mappings is done in
> arch_get_unmapped_area(), but these mappings are granted in
> get_unmapped_area() and arch_get_unmapped_area() is never called.
> 
> A quick look at sparc and sparc64 seem to show the same problem.

Ehh...  <asm/pgtable.h> defines HAVE_ARCH_UNMAPPED_AREA therefore
get_unmapped_area calls the arch's version of arch_get_unmapped_area
instead of the generic version in mm/mmap.c

  Ralf

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Peter Horton]

On Sat, Dec 13, 2003 at 05:05:36PM +0100, Ralf Baechle wrote:
> On Sat, Dec 13, 2003 at 11:41:34AM +0000, Peter Horton wrote:
> 
> > The current MIPS 2.4 kernel (from CVS) currently allows fixed shared
> > mappings to violate D-cache aliasing constraints.
> > 
> > The check for illegal fixed mappings is done in
> > arch_get_unmapped_area(), but these mappings are granted in
> > get_unmapped_area() and arch_get_unmapped_area() is never called.
> > 
> > A quick look at sparc and sparc64 seem to show the same problem.
> 
> Ehh...  <asm/pgtable.h> defines HAVE_ARCH_UNMAPPED_AREA therefore
> get_unmapped_area calls the arch's version of arch_get_unmapped_area
> instead of the generic version in mm/mmap.c
> 

arch_get_unmapped_area() never get called because get_unmapped_area()
notices the MAP_FIXED flag and returns success.

In the example below the second mmap() should fail because it violates
the shm_align_mask.

P.

pdh@qube2:~$ uname -a
Linux qube2 2.4.23 #2 Sat Dec 13 18:03:10 GMT 2003 mips unknown
pdh@qube2:~$ ./shared
0xdeadbeef 0
pdh@qube2:~$ cat shared.c
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/user.h>

int main(int argc, char *argv[])
{
        static char zero;
        void *p1, *p2;
        int fd;

        fd = open("/tmp/test.shared", O_CREAT|O_RDWR|O_TRUNC, 0664);
        if(fd == -1)
                return 1;
        unlink("/tmp/test.shared");

        lseek(fd, PAGE_SIZE - 1, SEEK_SET);
        if(write(fd, &zero, 1) != 1)
                return 1;

        p1 = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, fd, 0);
        if(p1 == MAP_FAILED)
                return 1;

        p2 = mmap(p1 + PAGE_SIZE, PAGE_SIZE, PROT_WRITE, MAP_SHARED|MAP_FIXED, fd, 0);
        if(p2 == MAP_FAILED || p2 - p1 != PAGE_SIZE)
                return 1;

        *(int *) p2 = 0xdeadbeef;

        printf("%#x %#x\n", *(int *) p2, *(int *) p1);

        return 0;
}
pdh@qube2:~$

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Jamie Lokier]

Peter Horton wrote:
> A quick look at sparc and sparc64 seem to show the same problem.

D-cache incoherence with unsuitably aligned multiple MAP_FIXED
mappings is also observed on SH4, SH5, PA-RISC 1.1d.  The kernel may
have the same behaviour on those platforms: allowing a mapping that
should not be allowed.

On some ARM and m68k boxes, incoherence is observed independent of
alignment, for multiple mappings of a page in the same user memory
space.

-- Jamie

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Linus Torvalds]

On Sat, 13 Dec 2003, Jamie Lokier wrote:
>
> Peter Horton wrote:
> > A quick look at sparc and sparc64 seem to show the same problem.
>
> D-cache incoherence with unsuitably aligned multiple MAP_FIXED
> mappings is also observed on SH4, SH5, PA-RISC 1.1d.  The kernel may
> have the same behaviour on those platforms: allowing a mapping that
> should not be allowed.

Why?

If the user asks for it, it's the users own damn fault. Nobody guarantees
cache coherency to users who require fixed addresses.

Just document it as a bug in the user program if this causes problems.
Don't blame the kernel - the kernel is only doing what the user asked it
to do.

		Linus

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Jamie Lokier]

Linus Torvalds wrote:
> > D-cache incoherence with unsuitably aligned multiple MAP_FIXED
> > mappings is also observed on SH4, SH5, PA-RISC 1.1d.  The kernel may
> > have the same behaviour on those platforms: allowing a mapping that
> > should not be allowed.
> 
> Why?
> 
> If the user asks for it, it's the users own damn fault. Nobody guarantees
> cache coherency to users who require fixed addresses.

I agree, the users asks for it and in rare cases it's even useful.

If that's your view, then several architectures have code in
arch_get_unmapped_base which tests MAP_FIXED - but that function isn't
called in the MAP_FIXED case any more.  The attached patch removes
those branches.  Trivial, but untested.

(It would be nice to be able to query the kernel to learn what mapping
alignment, if any, can be used to avoid aliasing problems though.
Something like "SHMLBA: 16k" or "SHMLBA: none" in /proc/cpuinfo and
whatever other interfaces present CPU info.  On some architectures the
value depends on the CPU model, determined at run time.  On some
architectures, there isn't a value).

-- Jamie

Patch: irq_idle-2.6.0-test4-01jl
Summary: Remove unused MAP_FIXED branches from arch_get_unmapped_area

Several architectures have code in arch_get_unmapped_base which tests
MAP_FIXED - but that function isn't called in the MAP_FIXED case any more.

For example, this code in arch/sparc/kernel/sys_sparc.c is quite typical:

        if (flags & MAP_FIXED) {
                /* We do not accept a shared mapping if it would violate
                 * cache aliasing constraints.
                 */
                if ((flags & MAP_SHARED) && (addr & (SHMLBA - 1)))
                        return -EINVAL;
                return addr;
        }

The function containing that test isn't called with MAP_FIXED these
days, so it's redundant and misleading.  This patch removes those tests
from all the arch_get_unmapped_area functions which have them.


diff -urN --exclude-from=dontdiff orig-2.6.0-test11/arch/mips/kernel/syscall.c mapalign-2.6.0-test11/arch/mips/kernel/syscall.c
--- orig-2.6.0-test11/arch/mips/kernel/syscall.c	2003-09-02 20:49:13.000000000 +0100
+++ mapalign-2.6.0-test11/arch/mips/kernel/syscall.c	2003-12-14 04:08:10.000000000 +0000
@@ -63,16 +63,6 @@
 	struct vm_area_struct * vmm;
 	int do_color_align;
 
-	if (flags & MAP_FIXED) {
-		/*
-		 * We do not accept a shared mapping if it would violate
-		 * cache aliasing constraints.
-		 */
-		if ((flags & MAP_SHARED) && (addr & shm_align_mask))
-			return -EINVAL;
-		return addr;
-	}
-
 	if (len > TASK_SIZE)
 		return -ENOMEM;
 	do_color_align = 0;
diff -urN --exclude-from=dontdiff orig-2.6.0-test11/arch/sh/kernel/sys_sh.c mapalign-2.6.0-test11/arch/sh/kernel/sys_sh.c
--- orig-2.6.0-test11/arch/sh/kernel/sys_sh.c	2003-07-08 21:55:03.000000000 +0100
+++ mapalign-2.6.0-test11/arch/sh/kernel/sys_sh.c	2003-12-14 04:07:20.000000000 +0000
@@ -54,15 +54,6 @@
 {
 	struct vm_area_struct *vma;
 
-	if (flags & MAP_FIXED) {
-		/* We do not accept a shared mapping if it would violate
-		 * cache aliasing constraints.
-		 */
-		if ((flags & MAP_SHARED) && (addr & (SHMLBA - 1)))
-			return -EINVAL;
-		return addr;
-	}
-
 	if (len > TASK_SIZE)
 		return -ENOMEM;
 	if (!addr)
diff -urN --exclude-from=dontdiff orig-2.6.0-test11/arch/sparc/kernel/sys_sparc.c mapalign-2.6.0-test11/arch/sparc/kernel/sys_sparc.c
--- orig-2.6.0-test11/arch/sparc/kernel/sys_sparc.c	2003-08-27 22:55:57.000000000 +0100
+++ mapalign-2.6.0-test11/arch/sparc/kernel/sys_sparc.c	2003-12-14 04:06:33.000000000 +0000
@@ -40,15 +40,6 @@
 {
 	struct vm_area_struct * vmm;
 
-	if (flags & MAP_FIXED) {
-		/* We do not accept a shared mapping if it would violate
-		 * cache aliasing constraints.
-		 */
-		if ((flags & MAP_SHARED) && (addr & (SHMLBA - 1)))
-			return -EINVAL;
-		return addr;
-	}
-
 	/* See asm-sparc/uaccess.h */
 	if (len > TASK_SIZE - PAGE_SIZE)
 		return -ENOMEM;
diff -urN --exclude-from=dontdiff orig-2.6.0-test11/arch/sparc64/kernel/sys_sparc.c mapalign-2.6.0-test11/arch/sparc64/kernel/sys_sparc.c
--- orig-2.6.0-test11/arch/sparc64/kernel/sys_sparc.c	2003-08-27 22:55:57.000000000 +0100
+++ mapalign-2.6.0-test11/arch/sparc64/kernel/sys_sparc.c	2003-12-14 04:06:46.000000000 +0000
@@ -52,15 +52,6 @@
 	unsigned long start_addr;
 	int do_color_align;
 
-	if (flags & MAP_FIXED) {
-		/* We do not accept a shared mapping if it would violate
-		 * cache aliasing constraints.
-		 */
-		if ((flags & MAP_SHARED) && (addr & (SHMLBA - 1)))
-			return -EINVAL;
-		return addr;
-	}
-
 	if (test_thread_flag(TIF_32BIT))
 		task_size = 0xf0000000UL;
 	if (len > task_size || len > -PAGE_OFFSET)

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Peter Horton]

On Sat, Dec 13, 2003 at 05:41:16PM -0800, Linus Torvalds wrote:
> 
> On Sat, 13 Dec 2003, Jamie Lokier wrote:
> >
> > Peter Horton wrote:
> > > A quick look at sparc and sparc64 seem to show the same problem.
> >
> > D-cache incoherence with unsuitably aligned multiple MAP_FIXED
> > mappings is also observed on SH4, SH5, PA-RISC 1.1d.  The kernel may
> > have the same behaviour on those platforms: allowing a mapping that
> > should not be allowed.
> 
> Why?
> 
> If the user asks for it, it's the users own damn fault. Nobody guarantees
> cache coherency to users who require fixed addresses.
> 
> Just document it as a bug in the user program if this causes problems.
> Don't blame the kernel - the kernel is only doing what the user asked it
> to do.
> 

I've seen code written for X86 use MAP_FIXED to create self wrapping
ring buffers. Surely it's better to fail the mmap() on other archs
rather than for the code to fail in unexpected ways?

It's a bug either way ... either the test should be fixed up or it
should be removed from arch_get_unmapped_area() to save confusion.

P.

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Jamie Lokier]

Peter Horton wrote:
> I've seen code written for X86 use MAP_FIXED to create self wrapping
> ring buffers. Surely it's better to fail the mmap() on other archs
> rather than for the code to fail in unexpected ways?

Such code should test the buffers or just not create ring buffers on
architectures it doesn't know about.  (You can usually simulate them
by copying data).  On some architectures there is _no_ alignment which
works, and even on x86 aligning aliases to 32k results in faster
memory accesses on some chips (AMD ones).

Also, sometimes a self wrapping ring buffer can work even when the
separation isn't coherent, provided the code using it forces cache
line flushes at the appropriate points.

-- Jamie

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Ralf Baechle]

On Sun, Dec 14, 2003 at 05:16:37PM +0000, Jamie Lokier wrote:

> Peter Horton wrote:
> > I've seen code written for X86 use MAP_FIXED to create self wrapping
> > ring buffers. Surely it's better to fail the mmap() on other archs
> > rather than for the code to fail in unexpected ways?
> 
> Such code should test the buffers or just not create ring buffers on
> architectures it doesn't know about.  (You can usually simulate them
> by copying data).  On some architectures there is _no_ alignment which
> works, and even on x86 aligning aliases to 32k results in faster
> memory accesses on some chips (AMD ones).
> 
> Also, sometimes a self wrapping ring buffer can work even when the
> separation isn't coherent, provided the code using it forces cache
> line flushes at the appropriate points.

Still I don't see why we shouldn't simply return EINVAL if a user is
trying to something obviously stupid - assuming full coherency in
application is a somewhat common thing and there's better things to waste
time on.  And yes while we could support coherency for arbitrary mappings
I agree it's a bad idea - but there's a huge difference between just
checking arguments and adding the large extra complexity of supporting
arbitrary combinations of addresses for mappings.

  Ralf

Re: Possible shared mapping bug in 2.4.23 (at least MIPS/Sparc)

[Linus Torvalds]

On Sun, 14 Dec 2003, Peter Horton wrote:
> >
> > Just document it as a bug in the user program if this causes problems.
> > Don't blame the kernel - the kernel is only doing what the user asked it
> > to do.
>
> I've seen code written for X86 use MAP_FIXED to create self wrapping
> ring buffers. Surely it's better to fail the mmap() on other archs
> rather than for the code to fail in unexpected ways?

Yes and no.

In _that_ case it would clearly be polite to just fail the mmap(). No
question about that - it's always nice if the kernel can find problems
early rather than late.

However, there are other ways you can screw yourself in user space, and
I'm not convinced it is always wrong to create a unaligned shared mapping.
For example, I can see that being useful exactly because you want to write
some CPU test in user space, for example.

So I'm generally opposed to the kernel saying "you can't do that" if there
isn't some really fundamental reason (security or stability) for it to be
really a no-no. It's often better to give the user rope to hang himself:
that rope might be used for interesting things too.

> It's a bug either way ... either the test should be fixed up or it
> should be removed from arch_get_unmapped_area() to save confusion.

I agree that we might as well remove it, but it's not 2.6.x material.

		Linus