* [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand
@ 2020-12-30 7:56 Li Xinhai
2020-12-30 20:52 ` John Hubbard
0 siblings, 1 reply; 4+ messages in thread
From: Li Xinhai @ 2020-12-30 7:56 UTC (permalink / raw)
To: linux-mm; +Cc: akpm, John Hubbard, Kirill A. Shutemov
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(-)
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;
--
2.18.4
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand
2020-12-30 7:56 [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand Li Xinhai
@ 2020-12-30 20:52 ` John Hubbard
2021-01-02 12:24 ` Li Xinhai
0 siblings, 1 reply; 4+ messages in thread
From: John Hubbard @ 2020-12-30 20:52 UTC (permalink / raw)
To: Li Xinhai, linux-mm; +Cc: akpm, Kirill A. Shutemov
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;
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand
2020-12-30 20:52 ` John Hubbard
@ 2021-01-02 12:24 ` Li Xinhai
2021-01-03 2:59 ` John Hubbard
0 siblings, 1 reply; 4+ messages in thread
From: Li Xinhai @ 2021-01-02 12:24 UTC (permalink / raw)
To: John Hubbard, linux-mm; +Cc: akpm, Kirill A. Shutemov
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;
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand
2021-01-02 12:24 ` Li Xinhai
@ 2021-01-03 2:59 ` John Hubbard
0 siblings, 0 replies; 4+ messages in thread
From: John Hubbard @ 2021-01-03 2:59 UTC (permalink / raw)
To: Li Xinhai, linux-mm; +Cc: akpm, Kirill A. Shutemov
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,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-03 2:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-30 7:56 [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand Li Xinhai
2020-12-30 20:52 ` John Hubbard
2021-01-02 12:24 ` Li Xinhai
2021-01-03 2:59 ` John Hubbard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox