Re: [patch] mmap bug with drivers that adjust vm_start

[Benjamin LaHaise <bcrl@redhat.com>]

On Tue, Mar 26, 2002 at 08:15:02PM +0100, Andrea Arcangeli wrote:
> > we could BUG() on getting a vma back from the new find_vma_prepare call.
> 
> yes, it sounds a good idea to verify there's no other mapping in the way
> of the relocation (until a better fix is implemented), it's a slow path
> so we won't hurt performance.

Okay, updated patch for this is below.  I also added a printk to give us 
which mmap operation was invoked to aid in debugging.

> The good reason, is that currently we're literally corrupting the
> userspace with the senseless do_munmap call in the add<->addr+len area
> before the ->mmap lowlevel callback. And such an munmap is certainly not
> required to maintain source and binary compatibility (otherwise it would
> be insane in the first place :).

Ah, right.  I disallow MAP_FIXED addresses that are not the "correct" offset, 
so this case would fail, albeit with the do_munmap occurring.  Personally, 
I would rather see an mmap fail if it would collide with an existing mapping, 
but that might break some applications.

Thanks for looking this over.  Cheers,

		-ben
-- 
"A man with a bass just walked in,
 and he's putting it down
 on the floor."

:r ~/patches/v2.4.19-pre4-mmap_fix-2.diff
--- retest.3/mm/mmap.c.org	Mon Mar 25 19:38:10 2002
+++ retest.3/mm/mmap.c	Tue Mar 26 15:01:47 2002
@@ -14,6 +14,7 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/personality.h>
+#include <linux/compiler.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgalloc.h>
@@ -548,7 +549,19 @@
 	 * Answer: Yes, several device drivers can do it in their
 	 *         f_op->mmap method. -DaveM
 	 */
-	addr = vma->vm_start;
+	if (addr != vma->vm_start) {
+		/* Since addr changed, we rely on the mmap op to prevent 
+		 * collisions with existing vmas and just use find_vma_prepare 
+		 * to update the tree pointers.
+		 */
+		addr = vma->vm_start;
+		if (unlikely(NULL != find_vma_prepare(mm, addr, &prev,
+						&rb_link, &rb_parent))) {
+			printk(KERN_ERR "buggy mmap operation: [<%p>]\n",
+				file ? file->f_op->mmap : NULL);
+			BUG();
+		}
+	}
 
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 	if (correct_wcount)