The existing logic is very confusing so improve things. Firstly rename the
confusing faulted_in_anon_vma variable to can_self_merge and update this
when the page offset is updated.
What is being checked for is a 'self-merge' - that is between the VMA being
remapped and its prior VMA (remember that this is copy_vma() - if a
non-MREMAP_DONTUNMAP remap the original VMA is only removed afterwards).
This can happen if the VMA is moved immediately adjacent to
itself, either before or after it:
|----------------|----------------|
| | |
v | v
|...............||---------------||...............|
| new || old || new |
|...............||---------------||---------------|
In these cases the old VMA is simply expanded to cover the new range.
It is also possible for the move to both self-merge and merge with a prior
VMA if it is placed between a preceding VMA and its old self:
|---------------|
| |
v |
|---------------||...............||---------------|
| prev || new || old |
|---------------||...............||---------------|
In this case, the old VMA is removed and 'prev' is expanded and replaces
it.
Since copy_vma_and_data() which calls copy_vma() intends to reference the
old VMA after the merge, it must have this pointer updated.
This kind of self-merge is not possible with a succeeding merge, as the
merge always prefers to expand the preceding VMA if possible.
copy_vma() accounts for this by explicitly checking to see if a self-merge
occurred and updating the vmap pointer if so. However it incorrect did so
even for a subsequent merge (this is simply a noop so it had no impact).
So change this to only check for the case which matters - a backwards
merge - and rearrange the parameters to make it clearer we're doing that -
i.e. check new_vma->vm_start < old_vma_start (having already renamed
vma_start to old_vma_start to make it clear this is the previous VMA).
Also update the existing wall-of-text comment to be a lot clearer.
While we're here, replace the VM_BUG_ON_VMA() with a VM_WARN_ON_ONCE_VMA()
and update the VMA userland tests accordingly.
No functional change intended.
Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]>
---
mm/vma.c | 35 ++++++++++++++++-------------------
tools/testing/vma/vma_internal.h | 1 +
2 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/mm/vma.c b/mm/vma.c
index b5bc3eec961c..18c8f2546765 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -1911,10 +1911,10 @@ struct vm_area_struct *copy_vma(struct vm_area_struct
**vmap,
bool *need_rmap_locks)
{
struct vm_area_struct *vma = *vmap;
- unsigned long vma_start = vma->vm_start;
+ unsigned long old_vma_start = vma->vm_start;
struct mm_struct *mm = vma->vm_mm;
struct vm_area_struct *new_vma;
- bool faulted_in_anon_vma = true;
+ bool can_self_merge = false;
VMA_ITERATOR(vmi, mm, addr);
VMG_VMA_STATE(vmg, &vmi, NULL, vma, addr, addr + len);
@@ -1924,7 +1924,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct
**vmap,
*/
if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) {
pgoff = addr >> PAGE_SHIFT;
- faulted_in_anon_vma = false;
+ can_self_merge = true;
}
/*
@@ -1944,24 +1944,21 @@ struct vm_area_struct *copy_vma(struct vm_area_struct
**vmap,
new_vma = vma_merge_copied_range(&vmg);
if (new_vma) {
- /*
- * Source vma may have been merged into new_vma
- */
- if (unlikely(vma_start >= new_vma->vm_start &&
- vma_start < new_vma->vm_end)) {
+ /* Self-merged and VMA replaced. */
+ if (unlikely(new_vma->vm_start < old_vma_start &&
+ new_vma->vm_end > old_vma_start)) {
/*
- * The only way we can get a vma_merge with
- * self during an mremap is if the vma hasn't
- * been faulted in yet and we were allowed to
- * reset the dst vma->vm_pgoff to the
- * destination address of the mremap to allow
- * the merge to happen. mremap must change the
- * vm_pgoff linearity between src and dst vmas
- * (in turn preventing a vma_merge) to be
- * safe. It is only safe to keep the vm_pgoff
- * linear if there are no pages mapped yet.
+ * The only way a VMA can both self-merge and be
+ * replaced is if the remap places the new VMA
+ * immediately prior to its old self ('next') and
+ * immediately after another VMA ('prev') causing the
+ * next to be removed and prev to be expanded to cover
+ * the entire range.
+ *
+ * This should only be possible if the page offset was
+ * updated, i.e. the VMA is unfaulted.
*/
- VM_BUG_ON_VMA(faulted_in_anon_vma, new_vma);
+ VM_WARN_ON_ONCE_VMA(!can_self_merge, new_vma);
*vmap = vma = new_vma;
}
*need_rmap_locks =
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 4f6c5666ac07..8a48b231aa7a 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -53,6 +53,7 @@ typedef __bitwise unsigned int vm_fault_t;
#define VM_WARN_ON(_expr) (WARN_ON(_expr))
#define VM_WARN_ON_ONCE(_expr) (WARN_ON_ONCE(_expr))
+#define VM_WARN_ON_ONCE_VMA(_expr, _vma) (WARN_ON_ONCE(_expr))
#define VM_WARN_ON_VMG(_expr, _vmg) (WARN_ON(_expr))
#define VM_BUG_ON(_expr) (BUG_ON(_expr))
#define VM_BUG_ON_VMA(_expr, _vma) (BUG_ON(_expr))
--
2.55.0
