Messages in this thread | | | Subject | Re: [PATCH] Fix OOPS in mmap_region() when merging adjacent VM_LOCKED file segments | From | Peter Zijlstra <> | Date | Fri, 30 Jan 2009 09:34:10 +0100 |
| |
On Thu, 2009-01-29 at 12:48 -0800, Linus Torvalds wrote: > 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;
You've made checkpatch unhappy ;-)
So we don't bother with anonymous only, always attempt the merge.
> @@ -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;
And here we don't bother merging because that would have been done before. Assuming ->mmap() doesn't go wild, in which case it ought to have set a VM_SPECIAL bit anyway to discourage merging.
[ And even if it didn't, failing to merge shouldn't be a problem, as minimizing the vmas is an optimization, not a strict requirement afaik. ]
The obvious glaring difference is the vma_policy() cruft. But staring at the code a bit I can't see how the new vma can have acquired a vm_policy here, so it ought to not matter.
Looks ok to my eyes, so I guess:
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
| |