Re: [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand

[John Hubbard <jhubbard@nvidia.com>]

On 12/29/20 11:56 PM, Li Xinhai wrote:
> Function vma_to_resize)() is called to find the vma to be remapped and
> also check if expand size is allowed or not. This function assume that all
> call sites should make sure new_len >= old_len, and currently this
> assumption is fullfilled at those two call sites, so no real problem at
> present.
> 
> After this patch, we explicitly check new_len < old_len case, and separate
> a new function for checking if expand size is allowed or not. Also rename
> vma_to_resize to vma_to_remap, since the vma to be remapped would not
> always require resize.

I don't see any clear motivation for this code churn, either above, or
implicitly in the patch itself. The new function names are not an improvement.

Probably best to just drop this, unless there is some sort of benefit that
I'm missing?


thanks,
-- 
John Hubbard
NVIDIA

> 
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> ---
>   mm/mremap.c | 79 ++++++++++++++++++++++++++++++-----------------------
>   1 file changed, 45 insertions(+), 34 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c5590afe7165..22eb4e9f35d6 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -621,13 +621,52 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>   	return new_addr;
>   }
>   
> -static struct vm_area_struct *vma_to_resize(unsigned long addr,
> +static struct vm_area_struct *vma_allow_expand(struct vm_area_struct *vma,
> +	unsigned long addr, unsigned long old_len, unsigned long new_len,
> +	unsigned long *p)
> +{
> +	struct mm_struct *mm = current->mm;
> +	unsigned long pgoff;
> +
> +	pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
> +	pgoff += vma->vm_pgoff;
> +	if (pgoff + (new_len >> PAGE_SHIFT) < pgoff)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
> +		return ERR_PTR(-EFAULT);
> +
> +	if (vma->vm_flags & VM_LOCKED) {
> +		unsigned long locked, lock_limit;
> +
> +		locked = mm->locked_vm << PAGE_SHIFT;
> +		lock_limit = rlimit(RLIMIT_MEMLOCK);
> +		locked += new_len - old_len;
> +		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> +			return ERR_PTR(-EAGAIN);
> +	}
> +
> +	if (!may_expand_vm(mm, vma->vm_flags,
> +				(new_len - old_len) >> PAGE_SHIFT))
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (vma->vm_flags & VM_ACCOUNT) {
> +		unsigned long charged = (new_len - old_len) >> PAGE_SHIFT;
> +
> +		if (security_vm_enough_memory_mm(mm, charged))
> +			return ERR_PTR(-ENOMEM);
> +		*p = charged;
> +	}
> +
> +	return vma;
> +}
> +
> +static struct vm_area_struct *vma_to_remap(unsigned long addr,
>   	unsigned long old_len, unsigned long new_len, unsigned long flags,
>   	unsigned long *p)
>   {
>   	struct mm_struct *mm = current->mm;
>   	struct vm_area_struct *vma = find_vma(mm, addr);
> -	unsigned long pgoff;
>   
>   	if (!vma || vma->vm_start > addr)
>   		return ERR_PTR(-EFAULT);
> @@ -656,39 +695,11 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>   	if (old_len > vma->vm_end - addr)
>   		return ERR_PTR(-EFAULT);
>   
> -	if (new_len == old_len)
> +	if (new_len <= old_len)
>   		return vma;
>   
>   	/* Need to be careful about a growing mapping */
> -	pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
> -	pgoff += vma->vm_pgoff;
> -	if (pgoff + (new_len >> PAGE_SHIFT) < pgoff)
> -		return ERR_PTR(-EINVAL);
> -
> -	if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
> -		return ERR_PTR(-EFAULT);
> -
> -	if (vma->vm_flags & VM_LOCKED) {
> -		unsigned long locked, lock_limit;
> -		locked = mm->locked_vm << PAGE_SHIFT;
> -		lock_limit = rlimit(RLIMIT_MEMLOCK);
> -		locked += new_len - old_len;
> -		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> -			return ERR_PTR(-EAGAIN);
> -	}
> -
> -	if (!may_expand_vm(mm, vma->vm_flags,
> -				(new_len - old_len) >> PAGE_SHIFT))
> -		return ERR_PTR(-ENOMEM);
> -
> -	if (vma->vm_flags & VM_ACCOUNT) {
> -		unsigned long charged = (new_len - old_len) >> PAGE_SHIFT;
> -		if (security_vm_enough_memory_mm(mm, charged))
> -			return ERR_PTR(-ENOMEM);
> -		*p = charged;
> -	}
> -
> -	return vma;
> +	return vma_allow_expand(vma, addr, old_len, new_len, p);
>   }
>   
>   static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
> @@ -743,7 +754,7 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>   		old_len = new_len;
>   	}
>   
> -	vma = vma_to_resize(addr, old_len, new_len, flags, &charged);
> +	vma = vma_to_remap(addr, old_len, new_len, flags, &charged);
>   	if (IS_ERR(vma)) {
>   		ret = PTR_ERR(vma);
>   		goto out;
> @@ -894,7 +905,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>   	/*
>   	 * Ok, we need to grow..
>   	 */
> -	vma = vma_to_resize(addr, old_len, new_len, flags, &charged);
> +	vma = vma_to_remap(addr, old_len, new_len, flags, &charged);
>   	if (IS_ERR(vma)) {
>   		ret = PTR_ERR(vma);
>   		goto out;
>