mm: shift VM_GROWS* check from mmap_region() to do_mmap_pgoff()
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v2 Latest1 patch 1 replyIn review
- v13 patches 2 repliesSuperseded
Patches in v2 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/1 | mm: shift VM_GROWS* check from mmap_region() to do_mmap_pgoff()mm/ | +4 | 1 reply |
Reviews trailers per patch
The patch has a Reviewed-by or Acked-by.
| Patch | Hugh Dickins |
|---|---|
| 1/1 | A-b |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.
Cover letter [PATCH v2 0/1] mm: shift VM_GROWS* check from mmap_region() to do_mmap_pgoff()
Andrew,
This patch replaces the buggy
mm-shift-vm_grows-check-from-mmap_region-to-do_mmap_pgoff.patch
I do not send the *-fix.patch because I'd like to update the changelog.
However it needs the explicit ack from Hugh.
Quoted 9 lines · Hugh Dickins
Quoted 5 lines · Andrew Morton
Quoted 3 lines · On Sun, 14 Jul 2013 18:54:51 +0200 Oleg Nesterov <[email protected]> wrote:
mmap()doesn't allow the non-anonymous mappings with VM_GROWS* bit set. In particular this means that mmap_region()->vma_merge(file,vm_flags) must always fail ifvm_flags&VM_GROWS.I didn't understand that sentence: if file is non-NULL perhaps?
Yes, this looks confusing, sorry.
I meant, vma_merge() must fail if "vm_flags & VM_GROWS" is set incorrectly. is_mergeable_vma() compares both vm_file and vm_flags.
Even if file == NULL (at this stage), "VM_SHARED | VM_GROWS" is not correct too, and vma_merge() can't succeed.
And, to clarify, I only mentioned this because I tried to convince myself that this change (if correct) can't make any difference except "avoid the not-really-correct do_munmap".
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
So it does not make sense to
check VM_GROWS* after we already allocated the new vma, the only caller,do_mmap_pgoff(), which can pass this flag can do the check itself.And this looks a bit more correct,
mmap_region()already unmapped the old mapping at this stage. But ifmmap()is going to fail, it should avoiddo_munmap()if possible.I agree with the sentiment, but the patch looks wrong to me.
Heh. You are right of course.
Quoted 3 lines
It did need staring, yes, but it looks NOK to me: this change permits
mmap(addr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_GROWSDOWN, fd, off)where fd is for a real file:
And note that "Only MAP_PRIVATE|MAP_ANONYMOUS can use MAP_GROWS" even tries to document that "MAP_PRIVATE && file" is not allowed too.
I have no idea how I managed to forget that MAP_PRIVATE never sets VM_MAYSHARE.
Thanks a lot Hugh.
Oleg.