On 8/5/24 14:13, Lorenzo Stoakes wrote:
> 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.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
Amazing!
Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> @@ -710,9 +706,30 @@ static struct vm_area_struct *vma_merge_modified(struct vma_merge_struct *vmg)
>
> /* If we span the entire VMA, a merge implies it will be deleted. */
> merge_will_delete_vma = left_side && right_side;
> - /* If we merge both VMAs, then next is also deleted. */
Nit: This comment ...
> +
> + /*
> + * 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.
> + */
... changed to this comment. Seems spurious, could have been like that
before already? I don't see how the new "This implies" part became relevant
now? We already tested merge_will_delete_vma above.
> 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);
>
