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

Linus Torvalds
2009-01-29T20:49:52Z

On Thu, 29 Jan 2009, Linus Torvalds wrote:
>
> THIS PATCH IS TOTALLY UNTESTED!

Well, it boots. FWIW. I've not really tested anything interesting with it,
but any potential breakage is at least not catastrophic and immediate.

> 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;
> +

.. and I think this part of it is actually unnecessary, because what
happens is that a shared anon mapping is turned into a shmem mapping when
it is inserted, and that actually ends up allocating a file for it. So the
vma->vm_file for anon mappings will not match a NULL file pointer
_anyway_, so there's no way it would end up merging.

So my patch can be further simplified, I think, to just the following.
Even more total lines removed.

I still want somebody else to look at and think about it, though.

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)