diff --git a/mm/vma.c b/mm/vma.c index 393bef83260485..8d1686fc8d5ab8 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -10,14 +10,6 @@ static inline bool is_mergeable_vma(struct vma_merge_struct *vmg, bool merge_next) { struct vm_area_struct *vma = merge_next ? vmg->next : vmg->prev; - /* - * If the vma has a ->close operation then the driver probably needs to - * release per-vma resources, so we don't attempt to merge those if the - * caller indicates the current vma may be removed as part of the merge, - * which is the case if we are attempting to merge the next VMA into - * this one. - */ - bool may_remove_vma = merge_next; if (!mpol_equal(vmg->policy, vma_policy(vma))) return false; @@ -33,8 +25,6 @@ static inline bool is_mergeable_vma(struct vma_merge_struct *vmg, bool merge_nex return false; if (vma->vm_file != vmg->file) return false; - if (may_remove_vma && vma->vm_ops && vma->vm_ops->close) - return false; if (!is_mergeable_vm_userfaultfd_ctx(vma, vmg->uffd_ctx)) return false; if (!anon_vma_name_eq(anon_vma_name(vma), vmg->anon_name)) @@ -632,6 +622,12 @@ static int commit_merge(struct vma_merge_struct *vmg, return 0; } +/* We can only remove VMAs when merging if they do not have a close hook. */ +static bool can_merge_remove_vma(struct vm_area_struct *vma) +{ + return !vma->vm_ops || !vma->vm_ops->close; +} + /* * vma_merge_existing_range - Attempt to merge VMAs based on a VMA having its * attributes modified. @@ -725,12 +721,30 @@ static struct vm_area_struct *vma_merge_existing_range(struct vma_merge_struct * merge_both = merge_left && merge_right; /* If we span the entire VMA, a merge implies it will be deleted. */ merge_will_delete_vma = left_side && right_side; + + /* + * If we need to remove vma in its entirety but are unable to do so, + * we have no sensible recourse but to abort the merge. + */ + if (merge_will_delete_vma && !can_merge_remove_vma(vma)) + return NULL; + /* * If we merge both VMAs, then next is also deleted. This implies * merge_will_delete_vma also. */ merge_will_delete_next = merge_both; + /* + * If we cannot delete next, then we can reduce the operation to merging + * prev and vma (thereby deleting vma). + */ + if (merge_will_delete_next && !can_merge_remove_vma(next)) { + merge_will_delete_next = false; + merge_right = false; + merge_both = false; + } + /* No matter what happens, we will be adjusting vma. */ vma_start_write(vma); @@ -772,21 +786,12 @@ static struct vm_area_struct *vma_merge_existing_range(struct vma_merge_struct * vmg->start = prev->vm_start; vmg->pgoff = prev->vm_pgoff; - if (merge_will_delete_vma) { - /* - * can_vma_merge_after() assumed we would not be - * removing vma, so it skipped the check for - * vm_ops->close, but we are removing vma. - */ - if (vma->vm_ops && vma->vm_ops->close) - err = -EINVAL; - } else { + if (!merge_will_delete_vma) { adjust = vma; adj_start = vmg->end - vma->vm_start; } - if (!err) - err = dup_anon_vma(prev, vma, &anon_dup); + err = dup_anon_vma(prev, vma, &anon_dup); } else { /* merge_right */ /* * |<----->| OR @@ -940,6 +945,14 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) vmg->vma = prev; vmg->pgoff = prev->vm_pgoff; + /* + * If this merge would result in removal of the next VMA but we + * are not permitted to do so, reduce the operation to merging + * prev and vma. + */ + if (can_merge_right && !can_merge_remove_vma(next)) + vmg->end = end; + vma_prev(vmg->vmi); /* Equivalent to going to the previous range */ } @@ -994,6 +1007,8 @@ int vma_expand(struct vma_merge_struct *vmg) int ret; remove_next = true; + /* This should already have been checked by this point. */ + VM_WARN_ON(!can_merge_remove_vma(next)); vma_start_write(next); ret = dup_anon_vma(vma, next, &anon_dup); if (ret) diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c index 25a95d9901ea9f..c53f220eb6cc03 100644 --- a/tools/testing/vma/vma.c +++ b/tools/testing/vma/vma.c @@ -387,6 +387,9 @@ static bool test_merge_new(void) struct anon_vma_chain dummy_anon_vma_chain_d = { .anon_vma = &dummy_anon_vma, }; + const struct vm_operations_struct vm_ops = { + .close = dummy_close, + }; int count; struct vm_area_struct *vma, *vma_a, *vma_b, *vma_c, *vma_d; bool merged; @@ -430,6 +433,7 @@ static bool test_merge_new(void) * 0123456789abc * AA*B DD CC */ + vma_a->vm_ops = &vm_ops; /* This should have no impact. */ vma_b->anon_vma = &dummy_anon_vma; vma = try_merge_new_vma(&mm, &vmg, 0x2000, 0x3000, 2, flags, &merged); ASSERT_EQ(vma, vma_a); @@ -466,6 +470,7 @@ static bool test_merge_new(void) * AAAAA *DD CC */ vma_d->anon_vma = &dummy_anon_vma; + vma_d->vm_ops = &vm_ops; /* This should have no impact. */ vma = try_merge_new_vma(&mm, &vmg, 0x6000, 0x7000, 6, flags, &merged); ASSERT_EQ(vma, vma_d); /* Prepend. */ @@ -483,6 +488,7 @@ static bool test_merge_new(void) * 0123456789abc * AAAAA*DDD CC */ + vma_d->vm_ops = NULL; /* This would otherwise degrade the merge. */ vma = try_merge_new_vma(&mm, &vmg, 0x5000, 0x6000, 5, flags, &merged); ASSERT_EQ(vma, vma_a); /* Merge with A, delete D. */ @@ -640,13 +646,11 @@ static bool test_vma_merge_with_close(void) const struct vm_operations_struct vm_ops = { .close = dummy_close, }; - struct vm_area_struct *vma_next = - alloc_and_link_vma(&mm, 0x2000, 0x3000, 2, flags); - struct vm_area_struct *vma; + struct vm_area_struct *vma_prev, *vma_next, *vma; /* - * When we merge VMAs we sometimes have to delete others as part of the - * operation. + * When merging VMAs we are not permitted to remove any VMA that has a + * vm_ops->close() hook. * * Considering the two possible adjacent VMAs to which a VMA can be * merged: @@ -697,28 +701,52 @@ static bool test_vma_merge_with_close(void) * would be set too, and thus scenario A would pick this up. */ - ASSERT_NE(vma_next, NULL); - /* - * SCENARIO A + * The only case of a new VMA merge that results in a VMA being deleted + * is one where both the previous and next VMAs are merged - in this + * instance the next VMA is deleted, and the previous VMA is extended. * - * 0123 - * *N + * If we are unable to do so, we reduce the operation to simply + * extending the prev VMA and not merging next. + * + * 0123456789 + * PPP**NNNN + * -> + * 0123456789 + * PPPPPPNNN */ - /* Make the next VMA have a close() callback. */ + vma_prev = alloc_and_link_vma(&mm, 0, 0x3000, 0, flags); + vma_next = alloc_and_link_vma(&mm, 0x5000, 0x9000, 5, flags); vma_next->vm_ops = &vm_ops; - /* Our proposed VMA has characteristics that would otherwise be merged. */ - vmg_set_range(&vmg, 0x1000, 0x2000, 1, flags); + vmg_set_range(&vmg, 0x3000, 0x5000, 3, flags); + ASSERT_EQ(merge_new(&vmg), vma_prev); + ASSERT_EQ(vmg.state, VMA_MERGE_SUCCESS); + ASSERT_EQ(vma_prev->vm_start, 0); + ASSERT_EQ(vma_prev->vm_end, 0x5000); + ASSERT_EQ(vma_prev->vm_pgoff, 0); - /* The next VMA having a close() operator should cause the merge to fail.*/ - ASSERT_EQ(merge_new(&vmg), NULL); - ASSERT_EQ(vmg.state, VMA_MERGE_NOMERGE); + ASSERT_EQ(cleanup_mm(&mm, &vmi), 2); - /* Now create the VMA so we can merge via modified flags */ - vmg_set_range(&vmg, 0x1000, 0x2000, 1, flags); - vma = alloc_and_link_vma(&mm, 0x1000, 0x2000, 1, flags); + /* + * When modifying an existing VMA there are further cases where we + * delete VMAs. + * + * <> + * 0123456789 + * PPPVV + * + * In this instance, if vma has a close hook, the merge simply cannot + * proceed. + */ + + vma_prev = alloc_and_link_vma(&mm, 0, 0x3000, 0, flags); + vma = alloc_and_link_vma(&mm, 0x3000, 0x5000, 3, flags); + vma->vm_ops = &vm_ops; + + vmg_set_range(&vmg, 0x3000, 0x5000, 3, flags); + vmg.prev = vma_prev; vmg.vma = vma; /* @@ -728,38 +756,90 @@ static bool test_vma_merge_with_close(void) ASSERT_EQ(merge_existing(&vmg), NULL); ASSERT_EQ(vmg.state, VMA_MERGE_NOMERGE); - /* SCENARIO B + ASSERT_EQ(cleanup_mm(&mm, &vmi), 2); + + /* + * This case is mirrored if merging with next. * - * 0123 - * P* + * <> + * 0123456789 + * VVNNNN * - * In order for this scenario to trigger, the VMA currently being - * modified must also have a .close(). + * In this instance, if vma has a close hook, the merge simply cannot + * proceed. */ - /* Reset VMG state. */ - vmg_set_range(&vmg, 0x1000, 0x2000, 1, flags); - /* - * Make next unmergeable, and don't let the scenario A check pick this - * up, we want to reproduce scenario B only. - */ - vma_next->vm_ops = NULL; - vma_next->__vm_flags &= ~VM_MAYWRITE; - /* Allocate prev. */ - vmg.prev = alloc_and_link_vma(&mm, 0, 0x1000, 0, flags); - /* Assign a vm_ops->close() function to VMA explicitly. */ + vma = alloc_and_link_vma(&mm, 0x3000, 0x5000, 3, flags); + vma_next = alloc_and_link_vma(&mm, 0x5000, 0x9000, 5, flags); vma->vm_ops = &vm_ops; + + vmg_set_range(&vmg, 0x3000, 0x5000, 3, flags); vmg.vma = vma; - /* Make sure merge does not occur. */ ASSERT_EQ(merge_existing(&vmg), NULL); /* * Initially this is misapprehended as an out of memory report, as the * close() check is handled in the same way as anon_vma duplication * failures, however a subsequent patch resolves this. */ - ASSERT_EQ(vmg.state, VMA_MERGE_ERROR_NOMEM); + ASSERT_EQ(vmg.state, VMA_MERGE_NOMERGE); + + ASSERT_EQ(cleanup_mm(&mm, &vmi), 2); + + /* + * Finally, we consider two variants of the case where we modify a VMA + * to merge with both the previous and next VMAs. + * + * The first variant is where vma has a close hook. In this instance, no + * merge can proceed. + * + * <> + * 0123456789 + * PPPVVNNNN + */ + + vma_prev = alloc_and_link_vma(&mm, 0, 0x3000, 0, flags); + vma = alloc_and_link_vma(&mm, 0x3000, 0x5000, 3, flags); + vma_next = alloc_and_link_vma(&mm, 0x5000, 0x9000, 5, flags); + vma->vm_ops = &vm_ops; + + vmg_set_range(&vmg, 0x3000, 0x5000, 3, flags); + vmg.prev = vma_prev; + vmg.vma = vma; + + ASSERT_EQ(merge_existing(&vmg), NULL); + ASSERT_EQ(vmg.state, VMA_MERGE_NOMERGE); + + ASSERT_EQ(cleanup_mm(&mm, &vmi), 3); + + /* + * The second variant is where next has a close hook. In this instance, + * we reduce the operation to a merge between prev and vma. + * + * <> + * 0123456789 + * PPPVVNNNN + * -> + * 0123456789 + * PPPPPNNNN + */ + + vma_prev = alloc_and_link_vma(&mm, 0, 0x3000, 0, flags); + vma = alloc_and_link_vma(&mm, 0x3000, 0x5000, 3, flags); + vma_next = alloc_and_link_vma(&mm, 0x5000, 0x9000, 5, flags); + vma_next->vm_ops = &vm_ops; + + vmg_set_range(&vmg, 0x3000, 0x5000, 3, flags); + vmg.prev = vma_prev; + vmg.vma = vma; + + ASSERT_EQ(merge_existing(&vmg), vma_prev); + ASSERT_EQ(vmg.state, VMA_MERGE_SUCCESS); + ASSERT_EQ(vma_prev->vm_start, 0); + ASSERT_EQ(vma_prev->vm_end, 0x5000); + ASSERT_EQ(vma_prev->vm_pgoff, 0); + + ASSERT_EQ(cleanup_mm(&mm, &vmi), 2); - cleanup_mm(&mm, &vmi); return true; } @@ -828,6 +908,9 @@ static bool test_merge_existing(void) .mm = &mm, .vmi = &vmi, }; + const struct vm_operations_struct vm_ops = { + .close = dummy_close, + }; /* * Merge right case - partial span. @@ -840,7 +923,9 @@ static bool test_merge_existing(void) * VNNNNNN */ vma = alloc_and_link_vma(&mm, 0x2000, 0x6000, 2, flags); + vma->vm_ops = &vm_ops; /* This should have no impact. */ vma_next = alloc_and_link_vma(&mm, 0x6000, 0x9000, 6, flags); + vma_next->vm_ops = &vm_ops; /* This should have no impact. */ vmg_set_range(&vmg, 0x3000, 0x6000, 3, flags); vmg.vma = vma; vmg.prev = vma; @@ -873,6 +958,7 @@ static bool test_merge_existing(void) */ vma = alloc_and_link_vma(&mm, 0x2000, 0x6000, 2, flags); vma_next = alloc_and_link_vma(&mm, 0x6000, 0x9000, 6, flags); + vma_next->vm_ops = &vm_ops; /* This should have no impact. */ vmg_set_range(&vmg, 0x2000, 0x6000, 2, flags); vmg.vma = vma; vma->anon_vma = &dummy_anon_vma; @@ -899,7 +985,9 @@ static bool test_merge_existing(void) * PPPPPPV */ vma_prev = alloc_and_link_vma(&mm, 0, 0x3000, 0, flags); + vma_prev->vm_ops = &vm_ops; /* This should have no impact. */ vma = alloc_and_link_vma(&mm, 0x3000, 0x7000, 3, flags); + vma->vm_ops = &vm_ops; /* This should have no impact. */ vmg_set_range(&vmg, 0x3000, 0x6000, 3, flags); vmg.prev = vma_prev; vmg.vma = vma; @@ -932,6 +1020,7 @@ static bool test_merge_existing(void) * PPPPPPP */ vma_prev = alloc_and_link_vma(&mm, 0, 0x3000, 0, flags); + vma_prev->vm_ops = &vm_ops; /* This should have no impact. */ vma = alloc_and_link_vma(&mm, 0x3000, 0x7000, 3, flags); vmg_set_range(&vmg, 0x3000, 0x7000, 3, flags); vmg.prev = vma_prev; @@ -960,6 +1049,7 @@ static bool test_merge_existing(void) * PPPPPPPPPP */ vma_prev = alloc_and_link_vma(&mm, 0, 0x3000, 0, flags); + vma_prev->vm_ops = &vm_ops; /* This should have no impact. */ vma = alloc_and_link_vma(&mm, 0x3000, 0x7000, 3, flags); vma_next = alloc_and_link_vma(&mm, 0x7000, 0x9000, 7, flags); vmg_set_range(&vmg, 0x3000, 0x7000, 3, flags);