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

From: Peter Horton <[email-masked]>
Date: Sat, 13 Dec 2003 11:41:34 +0000

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.

---

From: Ralf Baechle <[email-masked]>
Date: Sat, 13 Dec 2003 17:05:36 +0100

> 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

---

From: Peter Horton <[email-masked]>
Date: Sat, 13 Dec 2003 18:08:28 +0000

> 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:~$

---

From: Jamie Lokier <[email-masked]>
Date: Sat, 13 Dec 2003 22:26:26 +0000

> 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

---

From: Linus Torvalds <[email-masked]>
Date: Sat, 13 Dec 2003 17:41:16 -0800 (PST)

> have the same behaviour on those platforms: allowing a mapping that

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

---

From: Jamie Lokier <[email-masked]>
Date: Sun, 14 Dec 2003 04:20:22 +0000

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

---

From: Peter Horton <[email-masked]>
Date: Sun, 14 Dec 2003 10:38:03 +0000

> Don't blame the kernel - the kernel is only doing what the user asked it

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.

---

From: Jamie Lokier <[email-masked]>
Date: Sun, 14 Dec 2003 17:16:37 +0000

> 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

---

From: Linus Torvalds <[email-masked]>
Date: Sun, 14 Dec 2003 10:05:03 -0800 (PST)

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

> 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

---

From: Ralf Baechle <[email-masked]>
Date: Thu, 25 Dec 2003 14:03:16 +0100

> 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