| Message ID | 20201230075657.2720522-1-lixinhai.lxh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/mremap.c: refactor finding vma and checking vma is alllowed to expand | expand |
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,
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; >>
On 1/2/21 4:24 AM, Li Xinhai wrote: > 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. This chain of reasoning doesn't work for me. First of all, the callers of vma_to_resize() already check that new_len >= old_len, right? So I don't think "this needs to be fixed". Second, if there is a bug that I'm overlooking here, then I'd like to see a fix that does not also gratuitously refactor this into an unnecessary subroutine. What is the minimum clean change that you could make to fix the bug? Here's a bit more detail, in order to guide your future work: It is true that breaking something that is long and complex into one or more subroutines can improve some situations. But in this case, vma_to_resize() is already fairly short and not too complex, and your new subroutine has a somewhat misleading name. That, plus the act of splitting it up, please the unreadable documentation, actually makes it much harder to follow. Also, spend some time trying to write up what you did and why, in the commit log. If the log is quite difficult to write, then sometimes it means that it wasn't actually a good move. :) thanks,
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;
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. 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(-)