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

[Li Xinhai <lixinhai.lxh@gmail.com>]

On 12/31/20 4:52 AM, John Hubbard wrote:
> 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? >
The main issue is that in vma_to_size() there are code like below

	if (new_len == old_len)
		return vma;

	...
	locked += new_len - old_len;
	...


	unsigned long charged = (new_len - old_len) >> PAGE_SHIFT;
	...

the test didn't cover new_len < old_len case, then just do 'new_len - 
old_len'. That looks like hiding potential bug. So this need be fixed.

I tends to move out the code after the test into a separate function 
which is only for new_len > old_len case, currently there are various 
calculation/check around that test. So, we see which checks are for all 
new_len and old_len cases, and which only for new_len > old_len case, 
more clear when further change this part of code.

maybe better name than vma_to_remap()? or keep using vma_to_resize()?

> 
> 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;
>>