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

Li Xinhai <lixinhai.lxh@xxxxxxxxx> · Sat, 2 Jan 2021 20:24:50 +0800

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@xxxxxxxxxx>
Cc: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
Signed-off-by: Li Xinhai <lixinhai.lxh@xxxxxxxxx>
---
  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;









