Re: [PATCH] Fix OOPS in mmap_region() when merging adjacent VM_LOCKED file segments

[Linus Torvalds <torvalds@linux-foundation.org>]

On Thu, 29 Jan 2009, Hugh Dickins wrote:
> 
> The problem has always been that file->f_op->mmap(file, vma) on a
> special file can do various strange things, changing surprising
> fields of the struct vma passed to it, changing its mergeability.

Ahh, you're right. That's somewhat bogus, but it does explain why we have 
those two separate merge_vma() calls.

And as you also point out:

> Now it may well be that every driver which does something strange
> there already sets one of the VM_SPECIAL flags which prevent merging,
> or can easily be fixed to do so, or otherwise clearly cannot pose a
> problem.

.. and I think this is the right answer. If a device driver really does 
something as odd as play games with the offset that would make merging 
wrong, it had better set some of the VM_SPECIAL bits (presumably VM_IO) in 
order to never merge at all.

I added DaveM to the cc, since he's the one who is credited with the 
comment saying that several drivers change addr (vm_start). I only really 
see one, namely bf54x-lq043fb.c, and that one really depends on the whole 
nommu thing (ie it seems to simply require a particular virtual address, 
broken as that may be).

Btw, changing vm_start is actually very very wrong - it cannot work in 
general. Why? Because we earlier checked the "overlapping mmap" case with 
the original vm_start, an a driver that changes its vm_start in its mmap() 
routine would cause odd issues there. We also pass in "prev" to vma_link, 
so it could end up in totally the wrong place.

So changing vm_start is a sign of a driver bug, nothing less.

But there are more drivers who do change vm_pgoff, which is at least valid 
(if a bit dodgy), and is indeed a mergeability issue.  But at least a 
subset of those do already set VM_IO (eg fbmem.c - I only looked at one, 
and was happy that it already did that).

As background for Davem, here's the patch once more.

			Linus

---
 mm/mmap.c |   26 ++++++--------------------
 1 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 8d95902..d3fa10a 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1134,16 +1134,11 @@ munmap_back:
 	}
 
 	/*
-	 * Can we just expand an old private anonymous mapping?
-	 * The VM_SHARED test is necessary because shmem_zero_setup
-	 * will create the file object for a shared anonymous map below.
+	 * Can we just expand an old mapping?
 	 */
-	if (!file && !(vm_flags & VM_SHARED)) {
-		vma = vma_merge(mm, prev, addr, addr + len, vm_flags,
-					NULL, NULL, pgoff, NULL);
-		if (vma)
-			goto out;
-	}
+	vma = vma_merge(mm, prev, addr, addr + len, vm_flags, NULL, file, pgoff, NULL);
+	if (vma)
+		goto out;
 
 	/*
 	 * Determine the object being mapped and call the appropriate
@@ -1206,17 +1201,8 @@ munmap_back:
 	if (vma_wants_writenotify(vma))
 		vma->vm_page_prot = vm_get_page_prot(vm_flags & ~VM_SHARED);
 
-	if (file && vma_merge(mm, prev, addr, vma->vm_end,
-			vma->vm_flags, NULL, file, pgoff, vma_policy(vma))) {
-		mpol_put(vma_policy(vma));
-		kmem_cache_free(vm_area_cachep, vma);
-		fput(file);
-		if (vm_flags & VM_EXECUTABLE)
-			removed_exe_file_vma(mm);
-	} else {
-		vma_link(mm, vma, prev, rb_link, rb_parent);
-		file = vma->vm_file;
-	}
+	vma_link(mm, vma, prev, rb_link, rb_parent);
+	file = vma->vm_file;
 
 	/* Once vma denies write, undo our temporary denial count */
 	if (correct_wcount)