Re: [PATCH] mm: mmap: Check all failures before set values

Re: [PATCH] mm: mmap: Check all failures before set values

[Michal Hocko]

On Mon 24-08-15 00:59:39, gang.chen.5i5j@qq.com wrote:
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> When failure occurs and return, vma->vm_pgoff is already set, which is
> not a good idea.

Why? The vma is not inserted anywhere and the failure path is supposed
to simply free the vma.

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  mm/mmap.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 8e0366e..b5a6f09 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2878,6 +2878,13 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
>  	struct vm_area_struct *prev;
>  	struct rb_node **rb_link, *rb_parent;
>  
> +	if (find_vma_links(mm, vma->vm_start, vma->vm_end,
> +			   &prev, &rb_link, &rb_parent))
> +		return -ENOMEM;
> +	if ((vma->vm_flags & VM_ACCOUNT) &&
> +	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
> +		return -ENOMEM;
> +
>  	/*
>  	 * The vm_pgoff of a purely anonymous vma should be irrelevant
>  	 * until its first write fault, when page's anon_vma and index
> @@ -2894,12 +2901,6 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
>  		BUG_ON(vma->anon_vma);
>  		vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT;
>  	}
> -	if (find_vma_links(mm, vma->vm_start, vma->vm_end,
> -			   &prev, &rb_link, &rb_parent))
> -		return -ENOMEM;
> -	if ((vma->vm_flags & VM_ACCOUNT) &&
> -	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
> -		return -ENOMEM;
>  
>  	vma_link(mm, vma, prev, rb_link, rb_parent);
>  	return 0;
> -- 
> 1.9.3

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: mmap: Check all failures before set values

[Chen Gang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 960 bytes --]

On 8/24/15 19:32, Michal Hocko wrote:
> On Mon 24-08-15 00:59:39, gang.chen.5i5j@qq.com wrote:
>>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>>>
>>> When failure occurs and return, vma->vm_pgoff is already set, which is
>>> not a good idea.
> Why? The vma is not inserted anywhere and the failure path is supposed
> to simply free the vma.
>

It can save several insns when failure occurs.

It is always a little better to let the external function suppose fewer
callers' behalf.

It can save the code readers' (especially new readers') time resource
to avoid to analyze why set 'vma->vm_pgoff' before checking '-ENOMEM'
(may it cause issue? or is 'vm_pgoff' related with the next checking?).


Thanks.
--
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mm: mmap: Check all failures before set values

[Michal Hocko]

On Mon 24-08-15 21:34:25, Chen Gang wrote:
> On 8/24/15 19:32, Michal Hocko wrote:
> > On Mon 24-08-15 00:59:39, gang.chen.5i5j@qq.com wrote:
> >>> From: Chen Gang <gang.chen.5i5j@gmail.com>
> >>>
> >>> When failure occurs and return, vma->vm_pgoff is already set, which is
> >>> not a good idea.
> > Why? The vma is not inserted anywhere and the failure path is supposed
> > to simply free the vma.
> >
> 
> It can save several insns when failure occurs.

The failure is quite unlikely, though.

> It is always a little better to let the external function suppose fewer
> callers' behalf.

I am sorry but I do not understand what you are saying here.

> It can save the code readers' (especially new readers') time resource
> to avoid to analyze why set 'vma->vm_pgoff' before checking '-ENOMEM'
> (may it cause issue? or is 'vm_pgoff' related with the next checking?).

Then your changelog should be specific about these reasons. "not a good
idea" is definitely not a good justification for a patch. I am not
saying the patch is incorrect I just do not sure it is worth it. The
code is marginally better. But others might think otherwise. The
changelog needs some more work for sure.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: mmap: Check all failures before set values

[Michal Hocko]

On Tue 25-08-15 05:54:00, Chen Gang wrote:
> On 8/24/15 21:57, Michal Hocko wrote:
> > On Mon 24-08-15 21:34:25, Chen Gang wrote:
> 
> [...]
> 
> 
> >> It is always a little better to let the external function suppose fewer
> >> callers' behalf.
> > 
> > I am sorry but I do not understand what you are saying here.
> > 
> 
> Execuse me, my English maybe be still not quite well, my meaning is:
> 
>  - For the external functions (e.g. insert_vm_struct in our case), as a
>    callee, it may have to supose something from the caller.
> 
>  - If we can keep callee's functional contents no touch, a little fewer
>    supposing will let callee a little more independent from caller.
> 
>  - If can keep functional contens no touch, the lower dependency between
>    caller and callee is always better.

OK, I guess I understand what you mean. You are certainly right that a
partial initialization for the failure case is not nice in general. I
was just objecting that the callers are supposed to free the vma in
the failure case so any partial initialization doesn't matter in this
particular case.

Your patch would be more sensible if the failure case was more
likely. But this function is used for special mappings (vdso, temporary
vdso stack) which are created early in the process life time so both
failure paths are highly unlikely. If this was a part of a larger
changes where the function would be used elsewhere I wouldn't object at
all.

The reason I am skeptical about such changes in general is that
the effect is very marginal while it increases chances of the code
conflicts.

But as I've said, if others feel this is worthwhile I will not object.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: mmap: Check all failures before set values

[Chen Gang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1229 bytes --]

On 8/25/15 19:35, Michal Hocko wrote:
>
> OK, I guess I understand what you mean. You are certainly right that a
> partial initialization for the failure case is not nice in general. I
> was just objecting that the callers are supposed to free the vma in
> the failure case so any partial initialization doesn't matter in this
> particular case.
>
> Your patch would be more sensible if the failure case was more
> likely. But this function is used for special mappings (vdso, temporary
> vdso stack) which are created early in the process life time so both
> failure paths are highly unlikely. If this was a part of a larger
> changes where the function would be used elsewhere I wouldn't object at
> all.
>

OK.

> The reason I am skeptical about such changes in general is that
> the effect is very marginal while it increases chances of the code
> conflicts.
>
> But as I've said, if others feel this is worthwhile I will not object.
>

OK, I can understand.


Thanks.
--
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mm: mmap: Check all failures before set values

[Andrew Morton]

On Mon, 24 Aug 2015 13:32:13 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> On Mon 24-08-15 00:59:39, gang.chen.5i5j@qq.com wrote:
> > From: Chen Gang <gang.chen.5i5j@gmail.com>
> > 
> > When failure occurs and return, vma->vm_pgoff is already set, which is
> > not a good idea.
> 
> Why? The vma is not inserted anywhere and the failure path is supposed
> to simply free the vma.

Yes, it's pretty marginal but I suppose the code is a bit better with
the patch than without.  I did this:


From: Chen Gang <gang.chen.5i5j@gmail.com>
Subject: mm/mmap.c:insert_vm_struct(): check for failure before setting values

There's no point in initializing vma->vm_pgoff if the insertion attempt
will be failing anyway.  Run the checks before performing the initialization.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
Cc: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/mmap.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff -puN mm/mmap.c~mm-mmap-check-all-failures-before-set-values mm/mmap.c
--- a/mm/mmap.c~mm-mmap-check-all-failures-before-set-values
+++ a/mm/mmap.c
@@ -2859,6 +2859,13 @@ int insert_vm_struct(struct mm_struct *m
 	struct vm_area_struct *prev;
 	struct rb_node **rb_link, *rb_parent;
 
+	if (find_vma_links(mm, vma->vm_start, vma->vm_end,
+			   &prev, &rb_link, &rb_parent))
+		return -ENOMEM;
+	if ((vma->vm_flags & VM_ACCOUNT) &&
+	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
+		return -ENOMEM;
+
 	/*
 	 * The vm_pgoff of a purely anonymous vma should be irrelevant
 	 * until its first write fault, when page's anon_vma and index
@@ -2875,12 +2882,6 @@ int insert_vm_struct(struct mm_struct *m
 		BUG_ON(vma->anon_vma);
 		vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT;
 	}
-	if (find_vma_links(mm, vma->vm_start, vma->vm_end,
-			   &prev, &rb_link, &rb_parent))
-		return -ENOMEM;
-	if ((vma->vm_flags & VM_ACCOUNT) &&
-	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
-		return -ENOMEM;
 
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 	return 0;
_

Re: [PATCH] mm: mmap: Check all failures before set values

[Chen Gang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 897 bytes --]

On 8/25/15 05:25, Andrew Morton wrote:
> On Mon, 24 Aug 2015 13:32:13 +0200 Michal Hocko <mhocko@kernel.org> wrote:
>
>> On Mon 24-08-15 00:59:39, gang.chen.5i5j@qq.com wrote:
>>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>>>
>>> When failure occurs and return, vma->vm_pgoff is already set, which is
>>> not a good idea.
>>
>> Why? The vma is not inserted anywhere and the failure path is supposed
>> to simply free the vma.
>
> Yes, it's pretty marginal but I suppose the code is a bit better with
> the patch than without. I did this:
>

OK, thanks. The comments really need to be improved, just like Michal
Hocko said before.


Thanks.
--
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH] mm: mmap: Check all failures before set values

[Chen Gang]

When failure occurs and return, vma->vm_pgoff is already set, which is
not a good idea.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 mm/mmap.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 8e0366e..b5a6f09 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2878,6 +2878,13 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
 	struct vm_area_struct *prev;
 	struct rb_node **rb_link, *rb_parent;
 
+	if (find_vma_links(mm, vma->vm_start, vma->vm_end,
+			   &prev, &rb_link, &rb_parent))
+		return -ENOMEM;
+	if ((vma->vm_flags & VM_ACCOUNT) &&
+	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
+		return -ENOMEM;
+
 	/*
 	 * The vm_pgoff of a purely anonymous vma should be irrelevant
 	 * until its first write fault, when page's anon_vma and index
@@ -2894,12 +2901,6 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
 		BUG_ON(vma->anon_vma);
 		vma->vm_pgoff = vma->vm_start>> PAGE_SHIFT;
 	}
-	if (find_vma_links(mm, vma->vm_start, vma->vm_end,
-			   &prev, &rb_link, &rb_parent))
-		return -ENOMEM;
-	if ((vma->vm_flags & VM_ACCOUNT) &&
-	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
-		return -ENOMEM;
 
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 	return 0;
-- 
1.9.3