[PATCH 1/2] mremap should return -ENOMEM when __vm_enough_memory fail

[PATCH 1/2] mremap should return -ENOMEM when __vm_enough_memory fail

[denc716]

Recently I straced bash behavior in this dd zero pipe to read test,
in part of testing under vm.overcommit_memory=2 (OVERCOMMIT_NEVER mode):
    # dd if=/dev/zero | read x

The bash sub shell is calling mremap to reallocate more and more memory
untill it finally failed -ENOMEM (I expect), or to be killed by system
OOM killer (which should not happen under OVERCOMMIT_NEVER mode);
But the mremap system call actually failed of -EFAULT, which is a
surprise to me, I think it's supposed to be -ENOMEM? then I wrote this
piece of C code testing confirmed it:
https://gist.github.com/crquan/326bde37e1ddda8effe5

    $ ./remap
    allocated one page @0x7f686bf71000, (PAGE_SIZE: 4096)
    grabbed 7680512000 bytes of memory (1875125 pages) @ 00007f6690993000.
    mremap failed Bad address (14).

The -EFAULT comes from the branch of security_vm_enough_memory_mm
failure, underlyingly it calls __vm_enough_memory which returns only
0 for success or -ENOMEM; So why vma_to_resize needs to return
-EFAULT in this case? this sounds like a mistake to me.

Some more digging into git history:
1) Before commit 119f657c7 in May 1 2005 (pre 2.6.12 days) it was
   returning -ENOMEM for this failure;
2) but commit 119f657c7 changed it accidentally, to what ever is
   preserved in local ret, which happened to be -EFAULT, in a previous assignment;
3) then in commit 54f5de709 code refactoring, it's explicitly returning
   -EFAULT, should be wrong.

Signed-off-by: Derek Che <crquan@ymail.com>
Acked-by: "Kirill A. Shutemov" <kirill@shutemov.name>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 57dadc0..5da81cb 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -375,7 +375,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (vma->vm_flags & VM_ACCOUNT) {
 		unsigned long charged = (new_len - old_len) >> PAGE_SHIFT;
 		if (security_vm_enough_memory_mm(mm, charged))
-			goto Efault;
+			goto Enomem;
 		*p = charged;
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] clean up to just return ERR_PTR

[denc716]

Signed-off-by: Derek Che <crquan@ymail.com>
Acked-by: "Kirill A. Shutemov" <kirill@shutemov.name>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/mremap.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 5da81cb..afa3ab7 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -339,25 +339,25 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	struct vm_area_struct *vma = find_vma(mm, addr);
 
 	if (!vma || vma->vm_start > addr)
-		goto Efault;
+		return ERR_PTR(-EFAULT);
 
 	if (is_vm_hugetlb_page(vma))
-		goto Einval;
+		return ERR_PTR(-EINVAL);
 
 	/* We can't remap across vm area boundaries */
 	if (old_len > vma->vm_end - addr)
-		goto Efault;
+		return ERR_PTR(-EFAULT);
 
 	/* Need to be careful about a growing mapping */
 	if (new_len > old_len) {
 		unsigned long pgoff;
 
 		if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
-			goto Efault;
+			return ERR_PTR(-EFAULT);
 		pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
 		pgoff += vma->vm_pgoff;
 		if (pgoff + (new_len >> PAGE_SHIFT) < pgoff)
-			goto Einval;
+			return ERR_PTR(-EINVAL);
 	}
 
 	if (vma->vm_flags & VM_LOCKED) {
@@ -366,29 +366,20 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 		lock_limit = rlimit(RLIMIT_MEMLOCK);
 		locked += new_len - old_len;
 		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
-			goto Eagain;
+			return ERR_PTR(-EAGAIN);
 	}
 
 	if (!may_expand_vm(mm, (new_len - old_len) >> PAGE_SHIFT))
-		goto Enomem;
+		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))
-			goto Enomem;
+			return ERR_PTR(-ENOMEM);
 		*p = charged;
 	}
 
 	return vma;
-
-Efault:	/* very odd choice for most of the cases, but... */
-	return ERR_PTR(-EFAULT);
-Einval:
-	return ERR_PTR(-EINVAL);
-Enomem:
-	return ERR_PTR(-ENOMEM);
-Eagain:
-	return ERR_PTR(-EAGAIN);
 }
 
 static unsigned long mremap_to(unsigned long addr, unsigned long old_len,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mremap should return -ENOMEM when __vm_enough_memory fail

[David Rientjes]

On Tue, 17 Mar 2015, denc716@gmail.com wrote:

> Recently I straced bash behavior in this dd zero pipe to read test,
> in part of testing under vm.overcommit_memory=2 (OVERCOMMIT_NEVER mode):
>     # dd if=/dev/zero | read x
> 
> The bash sub shell is calling mremap to reallocate more and more memory
> untill it finally failed -ENOMEM (I expect), or to be killed by system
> OOM killer (which should not happen under OVERCOMMIT_NEVER mode);
> But the mremap system call actually failed of -EFAULT, which is a
> surprise to me, I think it's supposed to be -ENOMEM? then I wrote this
> piece of C code testing confirmed it:
> https://gist.github.com/crquan/326bde37e1ddda8effe5
> 
>     $ ./remap
>     allocated one page @0x7f686bf71000, (PAGE_SIZE: 4096)
>     grabbed 7680512000 bytes of memory (1875125 pages) @ 00007f6690993000.
>     mremap failed Bad address (14).
> 
> The -EFAULT comes from the branch of security_vm_enough_memory_mm
> failure, underlyingly it calls __vm_enough_memory which returns only
> 0 for success or -ENOMEM; So why vma_to_resize needs to return
> -EFAULT in this case? this sounds like a mistake to me.
> 
> Some more digging into git history:
> 1) Before commit 119f657c7 in May 1 2005 (pre 2.6.12 days) it was
>    returning -ENOMEM for this failure;
> 2) but commit 119f657c7 changed it accidentally, to what ever is
>    preserved in local ret, which happened to be -EFAULT, in a previous assignment;
> 3) then in commit 54f5de709 code refactoring, it's explicitly returning
>    -EFAULT, should be wrong.
> 
> Signed-off-by: Derek Che <crquan@ymail.com>
> Acked-by: "Kirill A. Shutemov" <kirill@shutemov.name>
> Acked-by: David Rientjes <rientjes@google.com>

Did Kirill ack this patch?

> ---
>  mm/mremap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 57dadc0..5da81cb 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -375,7 +375,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>  	if (vma->vm_flags & VM_ACCOUNT) {
>  		unsigned long charged = (new_len - old_len) >> PAGE_SHIFT;
>  		if (security_vm_enough_memory_mm(mm, charged))
> -			goto Efault;
> +			goto Enomem;
>  		*p = charged;
>  	}

The patch is corrupted and won't apply because there aren't three lines 
after the changed line.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] clean up to just return ERR_PTR

[David Rientjes]

On Tue, 17 Mar 2015, denc716@gmail.com wrote:

> Signed-off-by: Derek Che <crquan@ymail.com>
> Acked-by: "Kirill A. Shutemov" <kirill@shutemov.name>
> Acked-by: David Rientjes <rientjes@google.com>

I don't believe Kirill ever acked this patch, you need to wait for the 
person to respond to your email with the line themself before you can add 
it to your commit message.  Please see Documentation/SubmittingPatches.

He also suggested that this patch be done, so you need to add

Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>

You need a proper subject line for the patch, in this case it would be 
something like "mm, mremap: avoid unnecessary gotos in vma_to_resize()" 
and you also need a proper commit message such as:

	The "goto"s in vma_to_resize() aren't necessary since they just
	return a specific value.  Switch these statements to explicit
	return statements.

My acked-by was never given before this, but it can be added because it 
looks good:

Acked-by: David Rientjes <rientjes@google.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] mremap should return -ENOMEM when __vm_enough_memory fail

[Derek]

Recently I straced bash behavior in this dd zero pipe to read test,
in part of testing under vm.overcommit_memory=2 (OVERCOMMIT_NEVER mode):
    # dd if=/dev/zero | read x

The bash sub shell is calling mremap to reallocate more and more memory
untill it finally failed -ENOMEM (I expect), or to be killed by system
OOM killer (which should not happen under OVERCOMMIT_NEVER mode);
But the mremap system call actually failed of -EFAULT, which is a
surprise to me, I think it's supposed to be -ENOMEM? then I wrote this
piece of C code testing confirmed it:
https://gist.github.com/crquan/326bde37e1ddda8effe5

    $ ./remap
    allocated one page @0x7f686bf71000, (PAGE_SIZE: 4096)
    grabbed 7680512000 bytes of memory (1875125 pages) @ 00007f6690993000.
    mremap failed Bad address (14).

The -EFAULT comes from the branch of security_vm_enough_memory_mm
failure, underlyingly it calls __vm_enough_memory which returns only
0 for success or -ENOMEM; So why vma_to_resize needs to return
-EFAULT in this case? this sounds like a mistake to me.

Some more digging into git history:
1) Before commit 119f657c7 in May 1 2005 (pre 2.6.12 days) it was
   returning -ENOMEM for this failure;
2) but commit 119f657c7 changed it accidentally, to what ever is
   preserved in local ret, which happened to be -EFAULT, in a previous assignment;
3) then in commit 54f5de709 code refactoring, it's explicitly returning
   -EFAULT, should be wrong.

Signed-off-by: Derek Che <crquan@ymail.com>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 57dadc0..5da81cb 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -375,7 +375,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (vma->vm_flags & VM_ACCOUNT) {
 		unsigned long charged = (new_len - old_len) >> PAGE_SHIFT;
 		if (security_vm_enough_memory_mm(mm, charged))
-			goto Efault;
+			goto Enomem;
 		*p = charged;
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>