[Linux-ia64] mmap bug fix for IA-32 apps

[Linux-ia64] mmap bug fix for IA-32 apps

[Nakajima, Jun]

We found a bug with the mmap code in support of IA-32 apps. That bug was
exposed in a multi-threaded environment, and malloc(2), for example, was
causing a segmentation fault because of it.
Basically, the current code always saves and restores the user area that is
not part of the range specified by mmap(2) but on the native (i.e. IA-64)
page(s) affected by mmap() (because of the page-size difference between
IA-32 and IA-64), when it changes the protection.

Since such a user area can already have write protection, it's possible it
can cause data corruption when the kernel restores the saved (old) data. 

We tested several IA-32 applications, including Netscape.

Thanks,
Jun

diff -Nu arch/ia64/ia32/sys_ia32.c.orig arch/ia64/ia32/sys_ia32.c 
--- arch/ia64/ia32/sys_ia32.c.orig      Fri Nov 16 13:07:10 2001
+++ arch/ia64/ia32/sys_ia32.c   Fri Nov 30 13:17:55 2001
@@ -285,7 +285,16 @@
        if (!page)
                return -ENOMEM;
 
-       if (old_prot)
+       if (old_prot & VM_WRITE) {
+               if (flags & MAP_ANONYMOUS) {
+                       if (clear_user((void *) start, end - start)) {
+                               ret = -EFAULT;
+                               goto out;
+                       }
+                       goto out;
+               } else
+                       goto skip_mmap;
+       } else if (old_prot)
                copy_from_user(page, (void *) PAGE_START(start), PAGE_SIZE);
 
        down_write(&current->mm->mmap_sem);
@@ -306,6 +315,7 @@
                        copy_to_user((void *) end, page + PAGE_OFF(end),
                                     PAGE_SIZE - PAGE_OFF(end));
        }
+skip_mmap:
        if (!(flags & MAP_ANONYMOUS)) {
                /* read the file contents */
                inode = file->f_dentry->d_inode;

Re: [Linux-ia64] mmap bug fix for IA-32 apps

[David Mosberger]

Hi Jun,

Caveat: there are a couple of nasty race conditions in the IA-32 mmap
emulation code.  I tried to fix some of that when I reworked the mmap
code, but the situation is still far from perfect.  The problem is
that the IA-32 emulator needs the mmap_sem, but it needs it at times
when it could cause page faults (which would cause a deadlock).  To
fix the worst race conditions, I introduced the separate
ia32_mmap_sem, but holding it of course doesn't stop other threads for
modifying the task's mappings through page faults.

At the moment, I'm afraid that 4KB pages is the only way to get fully
thread-safe IA-32 emulation.

Now, to your patch: it doesn't look safe to me.  It patches the old
page if VM_WRITE is set.  But what if MAP_SHARED was used to map that
page?  We may have just ended up modifying a file.

I think the proper solution is to fix the race conditions.  It's
probably best to get rid of ia32_mmap_sem alltogether, because it
harms scalability, too.  I don't have a good idea at the moment how to
fix the problem though.  Perhaps we could solve these issues properly
with a recursive mmap_sem.  IIRC, there was talk about that on the
kernel list at some point (was it Manfred?).

	--david