| Message ID | 20200417172556.217480-1-bgeffon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Fix MREMAP_DONTUNMAP accounting on VMA merge | expand |
On Fri, Apr 17, 2020 at 10:26 AM Brian Geffon <bgeffon@google.com> wrote: > > However, MREMAP_DONTUNMAP leaves that original portion in place which > means that the VMA which was split and then remerged is not actually > split at the end of the mremap. I was waiting to hear others comment on this, but it's been very quiet. The patch looks correct to me, and the explanation is great. I'm inclined to just apply it. HOWEVER. I started looking at copy_vma(), and noticed that we seem to have exactly one caller: move_vma(). So I do have a query: would it perhaps not be a good idea to simply remove the "vma_merge()" call from copy_vma(), and do at the end of move_vma() instead? I don't hate this patch either, and I'll happily apply it if people prefer this one, but before doing that I thought I'd ask whether maybe instead of fixing up the mess made by vma_merge() that people didn't think about, maybe we should fix it at the underlying source of the problem? Are there any advantages to merging early? Shouldn't the basic principle be that we'd strive to always do the vma_merge() at the end of an operation that might have generated a mergable sequence of vma's? Linus
On Sun, Apr 19, 2020 at 10:59 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So I do have a query: would it perhaps not be a good idea to simply > remove the "vma_merge()" call from copy_vma(), and do at the end of > move_vma() instead? I decided to apply the patch regardless, even if somebody does end up deciding "yeah, let's move the vma_merge() down" later. Linus
diff --git a/mm/mremap.c b/mm/mremap.c index a7e282ead438..c881abeba0bf 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -413,9 +413,20 @@ static unsigned long move_vma(struct vm_area_struct *vma, /* Always put back VM_ACCOUNT since we won't unmap */ vma->vm_flags |= VM_ACCOUNT; - vm_acct_memory(vma_pages(new_vma)); + vm_acct_memory(new_len >> PAGE_SHIFT); } + /* + * VMAs can actually be merged back together in copy_vma + * calling merge_vma. This can happen with anonymous vmas + * which have not yet been faulted, so if we were to consider + * this VMA split we'll end up adding VM_ACCOUNT on the + * next VMA, which is completely unrelated if this VMA + * was re-merged. + */ + if (split && new_vma == vma) + split = 0; + /* We always clear VM_LOCKED[ONFAULT] on the old vma */ vma->vm_flags &= VM_LOCKED_CLEAR_MASK;
When remapping a mapping where a portion of a VMA is remapped into another portion of the VMA it can cause the VMA to become split. During the copy_vma operation the VMA can actually be remerged if it's an anonymous VMA whose pages have not yet been faulted. This isn't normally a problem because at the end of the remap the original portion is unmapped causing it to become split again. However, MREMAP_DONTUNMAP leaves that original portion in place which means that the VMA which was split and then remerged is not actually split at the end of the mremap. This patch fixes a bug where we don't detect that the VMAs got remerged and we end up putting back VM_ACCOUNT on the next mapping which is completely unreleated. When that next mapping is unmapped it results in incorrectly unaccounting for the memory which was never accounted, and eventually we will underflow on the memory comittment. There is also another issue which is similar, we're currently accouting for the number of pages in the new_vma but that's wrong. We need to account for the length of the remap operation as that's all that is being added. If there was a mapping already at that location its comittment would have been adjusted as part of the munmap at the start of the mremap. A really simple repro can be seen in: https://gist.github.com/bgaff/e101ce99da7d9a8c60acc641d07f312c Fixes: e346b3813067 ("mm/mremap: add MREMAP_DONTUNMAP to mremap()") Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Brian Geffon <bgeffon@google.com> --- mm/mremap.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)