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

[Linus Torvalds]

On Thu, 29 Jan 2009, Lee Schermerhorn wrote:
> 
> This patch provides a simplistic fix, suitable, I hope, for -stable.

So I've looked a bit at that function, and I wonder why we can't just do 
the "vma_merge()" earlier - before we allocate that whole vma to begin 
with.

We already do that for anonymous mappings _anyway_, using the exact same 
"vma_merge()" function. So how about just expanding on that logic, and 
simplifying everything at the same time?

THIS PATCH IS TOTALLY UNTESTED!

There may be some reason why we didn't do it this way to begin with, but 
this woudl actually seem to be the more logical way to fix the problem: by 
simplifying the whole function to the point where we never try to do that 
"try to merge late and then fix up the fact that we have to free the vma 
we allocated earlier" thing at all!

Hmm? This would remove a more lines than it adds, and actually makes 
things more readable. If it works. Anybody see why it wouldn't?

		Linus

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

diff --git a/mm/mmap.c b/mm/mmap.c
index 8d95902..3f78ead 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -769,6 +769,10 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
 	if (vm_flags & VM_SPECIAL)
 		return NULL;
 
+	/* Anonymous shared mappings are unsharable */
+	if ((vm_flags & VM_SHARED) && !file)
+		return NULL;
+
 	if (prev)
 		next = prev->vm_next;
 	else
@@ -1134,16 +1138,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 +1205,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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/