From 01c373e9a5ce2273812eaf83036c5357829fb3f7 Mon Sep 17 00:00:00 2001 From: Lorenzo Stoakes Date: Fri, 30 Aug 2024 19:10:22 +0100 Subject: mm: rework vm_ops->close() handling on VMA merge In commit 714965ca8252 ("mm/mmap: start distinguishing if vma can be removed in mergeability test") we relaxed the VMA merge rules for VMAs possessing a vm_ops->close() hook, permitting this operation in instances where we wouldn't delete the VMA as part of the merge operation. This was later corrected in commit fc0c8f9089c2 ("mm, mmap: fix vma_merge() case 7 with vma_ops->close") to account for a subtle case that the previous commit had not taken into account. In both instances, we first rely on is_mergeable_vma() to determine whether we might be dealing with a VMA that might be removed, taking advantage of the fact that a 'previous' VMA will never be deleted, only VMAs that follow it. The second patch corrects the instance where a merge of the previous VMA into a subsequent one did not correctly check whether the subsequent VMA had a vm_ops->close() handler. Both changes prevent merge cases that are actually permissible (for instance a merge of a VMA into a following VMA with a vm_ops->close(), but with no previous VMA, which would result in the next VMA being extended, not deleted). In addition, both changes fail to consider the case where a VMA that would otherwise be merged with the previous and next VMA might have vm_ops->close(), on the assumption that for this to be the case, all three would have to have the same vma->vm_file to be mergeable and thus the same vm_ops. And in addition both changes operate at 50,000 feet, trying to guess whether a VMA will be deleted. As we have majorly refactored the VMA merge operation and de-duplicated code to the point where we know precisely where deletions will occur, this patch removes the aforementioned checks altogether and instead explicitly checks whether a VMA will be deleted. In cases where a reduced merge is still possible (where we merge both previous and next VMA but the next VMA has a vm_ops->close hook, meaning we could just merge the previous and current VMA), we do so, otherwise the merge is not permitted. We take advantage of our userland testing to assert that this functions correctly - replacing the previous limited vm_ops->close() tests with tests for every single case where we delete a VMA. We also update all testing for both new and modified VMAs to set vma->vm_ops->close() in every single instance where this would not prevent the merge, to assert that we never do so. Link: https://lkml.kernel.org/r/9f96b8cfeef3d14afabddac3d6144afdfbef2e22.1725040657.git.lorenzo.stoakes@oracle.com Signed-off-by: Lorenzo Stoakes Acked-by: Vlastimil Babka Cc: Liam R. Howlett Cc: Mark Brown Cc: Bert Karwatzki Cc: Jeff Xu Cc: Jiri Olsa Cc: Kees Cook Cc: Lorenzo Stoakes Cc: Matthew Wilcox Cc: "Paul E. McKenney" Cc: Paul Moore Cc: Sidhartha Kumar Cc: Suren Baghdasaryan Signed-off-by: Andrew Morton --- mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 21 deletions(-) (limited to 'mm/vma.c') diff --git a/mm/vma.c b/mm/vma.c index 393bef832604..8d1686fc8d5a 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) -- cgit v1.2.3