On 10/9/23 22:53, Lorenzo Stoakes wrote:
> mprotect() and other functions which change VMA parameters over a range
> each employ a pattern of:-
>
> 1. Attempt to merge the range with adjacent VMAs.
> 2. If this fails, and the range spans a subset of the VMA, split it
> accordingly.
>
> This is open-coded and duplicated in each case. Also in each case most of
> the parameters passed to vma_merge() remain the same.
>
> Create a new function, vma_modify(), which abstracts this operation,
> accepting only those parameters which can be changed.
>
> To avoid the mess of invoking each function call with unnecessary
> parameters, create inline wrapper functions for each of the modify
> operations, parameterised only by what is required to perform the action.
>
> Note that the userfaultfd_release() case works even though it does not
> split VMAs - since start is set to vma->vm_start and end is set to
> vma->vm_end, the split logic does not trigger.
>
> In addition, since we calculate pgoff to be equal to vma->vm_pgoff + (start
> - vma->vm_start) >> PAGE_SHIFT, and start - vma->vm_start will be 0 in this
> instance, this invocation will remain unchanged.
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
some nits below:
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2437,6 +2437,51 @@ int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
> return __split_vma(vmi, vma, addr, new_below);
> }
>
> +/*
> + * We are about to modify one or multiple of a VMA's flags, policy, userfaultfd
> + * context and anonymous VMA name within the range [start, end).
> + *
> + * As a result, we might be able to merge the newly modified VMA range with an
> + * adjacent VMA with identical properties.
> + *
> + * If no merge is possible and the range does not span the entirety of the VMA,
> + * we then need to split the VMA to accommodate the change.
> + */
This could describe the return value too? It's not entirely trivial.
But I also wonder if we could just return 'vma' for the split_vma() cases
and the callers could simply stop distinguishing whether there was a merge
or split, and their code would become even simpler?
It seems to me most callers don't care, except mprotect, see below...
> +struct vm_area_struct *vma_modify(struct vma_iterator *vmi,
> + struct vm_area_struct *prev,
> + struct vm_area_struct *vma,
> + unsigned long start, unsigned long end,
> + unsigned long vm_flags,
> + struct mempolicy *policy,
> + struct vm_userfaultfd_ctx uffd_ctx,
> + struct anon_vma_name *anon_name)
> +{
> + pgoff_t pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> + struct vm_area_struct *merged;
> +
> + merged = vma_merge(vmi, vma->vm_mm, prev, start, end, vm_flags,
> + vma->anon_vma, vma->vm_file, pgoff, policy,
> + uffd_ctx, anon_name);
> + if (merged)
> + return merged;
> +
> + if (vma->vm_start < start) {
> + int err = split_vma(vmi, vma, start, 1);
> +
> + if (err)
> + return ERR_PTR(err);
> + }
> +
> + if (vma->vm_end > end) {
> + int err = split_vma(vmi, vma, end, 0);
> +
> + if (err)
> + return ERR_PTR(err);
> + }
> +
> + return NULL;
> +}
> +
> /*
> * do_vmi_align_munmap() - munmap the aligned region from @start to @end.
> * @vmi: The vma iterator
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index b94fbb45d5c7..6f85d99682ab 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -581,7 +581,7 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
> long nrpages = (end - start) >> PAGE_SHIFT;
> unsigned int mm_cp_flags = 0;
> unsigned long charged = 0;
> - pgoff_t pgoff;
> + struct vm_area_struct *merged;
> int error;
>
> if (newflags == oldflags) {
> @@ -625,34 +625,19 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
> }
> }
>
> - /*
> - * First try to merge with previous and/or next vma.
> - */
> - pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> - *pprev = vma_merge(vmi, mm, *pprev, start, end, newflags,
> - vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> - vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> - if (*pprev) {
> - vma = *pprev;
> - VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY);
> - goto success;
> + merged = vma_modify_flags(vmi, *pprev, vma, start, end, newflags);
> + if (IS_ERR(merged)) {
> + error = PTR_ERR(merged);
> + goto fail;
> }
>
> - *pprev = vma;
> -
> - if (start != vma->vm_start) {
> - error = split_vma(vmi, vma, start, 1);
> - if (error)
> - goto fail;
> - }
> -
> - if (end != vma->vm_end) {
> - error = split_vma(vmi, vma, end, 0);
> - if (error)
> - goto fail;
> + if (merged) {
> + vma = *pprev = merged;
> + VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY);
This VM_WARN_ON() is AFAICS the only piece of code that cares about merged
vs split. Would it be ok to call it for the split vma cases as well, or
maybe remove it?
> + } else {
> + *pprev = vma;
> }
>
> -success:
> /*
> * vm_flags and vm_page_prot are protected by the mmap_lock
> * held in write mode.
