Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681658 > unrolled thread

[PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack Michal Hocko <mhocko@kernel.org> - 2017-07-05 19:00 +0200
    Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in  the stack Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 19:50 +0200
      Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes  in the stack Michal Hocko <mhocko@kernel.org> - 2017-07-05 20:30 +0200
        Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in  the stack Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 20:40 +0200
          Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes  in the stack Michal Hocko <mhocko@kernel.org> - 2017-07-05 21:00 +0200
            Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in  the stack Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 21:20 +0200
              Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes  in the stack Willy Tarreau <w@1wt.eu> - 2017-07-05 21:20 +0200
            Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes  in the stack Michal Hocko <mhocko@kernel.org> - 2017-07-05 21:20 +0200
        Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes  in the stack Andrew Morton <akpm@linux-foundation.org> - 2017-07-05 23:20 +0200
          Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in  the stack Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 23:50 +0200
          Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes  in the stack Michal Hocko <mhocko@kernel.org> - 2017-07-06 08:50 +0200

#1681658 — [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

From: Michal Hocko <mhocko@suse.com>

"mm: enlarge stack guard gap" has introduced a regression in some rust
and Java environments which are trying to implement their own stack
guard page.  They are punching a new MAP_FIXED mapping inside the
existing stack Vma.

This will confuse expand_{downwards,upwards} into thinking that the stack
expansion would in fact get us too close to an existing non-stack vma
which is a correct behavior wrt. safety. It is a real regression on
the other hand. Let's work around the problem by considering PROT_NONE
mapping as a part of the stack. This is a gros hack but overflowing to
such a mapping would trap anyway an we only can hope that usespace
knows what it is doing and handle it propely.

Fixes: d4d2d35e6ef9 ("mm: larger stack guard gap, between vmas")
Debugged-by: Vlastimil Babka <vbabka@suse.cz>
Cc: stable
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Hi,
the original thread [1] has grown quite large and also a bit confusing.
At least the rust part should be fixed by this patch. 32b java will
probably need something more on top of this. Btw. JNI environments rely
on MAP_FIXED PROT_NONE as well they were just lucky to not hit the issue
yet I guess.

[1] http://lkml.kernel.org/r/1499126133.2707.20.camel@decadent.org.uk
 mm/mmap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index f60a8bc2869c..2e996cbf4ff3 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2244,7 +2244,8 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 		gap_addr = TASK_SIZE;
 
 	next = vma->vm_next;
-	if (next && next->vm_start < gap_addr) {
+	if (next && next->vm_start < gap_addr &&
+			(next->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
 		if (!(next->vm_flags & VM_GROWSUP))
 			return -ENOMEM;
 		/* Check that both stack segments have the same anon_vma? */
@@ -2325,7 +2326,8 @@ int expand_downwards(struct vm_area_struct *vma,
 	/* Enforce stack_guard_gap */
 	prev = vma->vm_prev;
 	/* Check that both stack segments have the same anon_vma? */
-	if (prev && !(prev->vm_flags & VM_GROWSDOWN)) {
+	if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
+			(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
 		if (address - prev->vm_end < stack_guard_gap)
 			return -ENOMEM;
 	}
-- 
2.11.0

[toc] | [next] | [standalone]

#1681683 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed, Jul 5, 2017 at 9:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> "mm: enlarge stack guard gap" has introduced a regression in some rust
> and Java environments which are trying to implement their own stack
> guard page.  They are punching a new MAP_FIXED mapping inside the
> existing stack Vma.

Hmm. What version is this patch against? It doesn't seem to match my 4.12 tree.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1681700 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed 05-07-17 10:43:27, Linus Torvalds wrote:
> On Wed, Jul 5, 2017 at 9:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > "mm: enlarge stack guard gap" has introduced a regression in some rust
> > and Java environments which are trying to implement their own stack
> > guard page.  They are punching a new MAP_FIXED mapping inside the
> > existing stack Vma.
> 
> Hmm. What version is this patch against? It doesn't seem to match my 4.12 tree.

Dohh, that was on mmotm which has a clean up by Oleg which reorganizes
the code a bit. This is on top of the current master
---
From fd538009ac373a5f87538786412a3e6191fa6001 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 4 Jul 2017 11:27:39 +0200
Subject: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the
 stack

"mm: enlarge stack guard gap" has introduced a regression in some rust
and Java environments which are trying to implement their own stack
guard page.  They are punching a new MAP_FIXED mapping inside the
existing stack Vma.

This will confuse expand_{downwards,upwards} into thinking that the stack
expansion would in fact get us too close to an existing non-stack vma
which is a correct behavior wrt. safety. It is a real regression on
the other hand. Let's work around the problem by considering PROT_NONE
mapping as a part of the stack. This is a gros hack but overflowing to
such a mapping would trap anyway an we only can hope that usespace
knows what it is doing and handle it propely.

Fixes: d4d2d35e6ef9 ("mm: larger stack guard gap, between vmas")
Debugged-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/mmap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index a5e3dcd75e79..ece0f6d3a1b5 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2244,7 +2244,8 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 		gap_addr = TASK_SIZE;
 
 	next = vma->vm_next;
-	if (next && next->vm_start < gap_addr) {
+	if (next && next->vm_start < gap_addr &&
+			(next->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
 		if (!(next->vm_flags & VM_GROWSUP))
 			return -ENOMEM;
 		/* Check that both stack segments have the same anon_vma? */
@@ -2328,7 +2329,8 @@ int expand_downwards(struct vm_area_struct *vma,
 	if (gap_addr > address)
 		return -ENOMEM;
 	prev = vma->vm_prev;
-	if (prev && prev->vm_end > gap_addr) {
+	if (prev && prev->vm_end > gap_addr &&
+			(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
 		if (!(prev->vm_flags & VM_GROWSDOWN))
 			return -ENOMEM;
 		/* Check that both stack segments have the same anon_vma? */
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1681703 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed, Jul 5, 2017 at 11:28 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> Dohh, that was on mmotm which has a clean up by Oleg which reorganizes
> the code a bit. This is on top of the current master

Oh, ok. I think I know which patch from Oleg you're talking about.

Since I do want that patch too, and since I'd hate to cause
unnecessary merge conflicts in this area, how about we just plan on
letting your original patch (on top of Oleg's) go through Andrew and
the -mm tree? I'll get it that way, and it's not like this is
timing-critical.

Having to fix up conflicts in this area would just be annoying, and
would be nasty for back-porting too.

                Linus

[toc] | [prev] | [next] | [standalone]

#1681708 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed 05-07-17 11:35:51, Linus Torvalds wrote:
> On Wed, Jul 5, 2017 at 11:28 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > Dohh, that was on mmotm which has a clean up by Oleg which reorganizes
> > the code a bit. This is on top of the current master
> 
> Oh, ok. I think I know which patch from Oleg you're talking about.
> 
> Since I do want that patch too, and since I'd hate to cause
> unnecessary merge conflicts in this area, how about we just plan on
> letting your original patch (on top of Oleg's) go through Andrew and
> the -mm tree? I'll get it that way, and it's not like this is
> timing-critical.

That would lead to conflicts when backporting to stable trees though
which is quite annoying as well and arguably slightly more annoying than
resolving this in mmotm. I can help to rebase Oleg's patch on top of
mine which is not a stable material.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1681720 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed, Jul 5, 2017 at 11:53 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> That would lead to conflicts when backporting to stable trees though
> which is quite annoying as well and arguably slightly more annoying than
> resolving this in mmotm. I can help to rebase Oleg's patch on top of
> mine which is not a stable material.

Ok, fair enough - I was actually expecting that Oleg's patch would
just be marked for stable too just to keep differences minimal.

But yes, putting your patch in first and then Oleg's on top means that
it works regardless.

Any opinions from others?

           Linus

[toc] | [prev] | [next] | [standalone]

#1681721 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed, Jul 05, 2017 at 12:15:05PM -0700, Linus Torvalds wrote:
> On Wed, Jul 5, 2017 at 11:53 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > That would lead to conflicts when backporting to stable trees though
> > which is quite annoying as well and arguably slightly more annoying than
> > resolving this in mmotm. I can help to rebase Oleg's patch on top of
> > mine which is not a stable material.
> 
> Ok, fair enough - I was actually expecting that Oleg's patch would
> just be marked for stable too just to keep differences minimal.
> 
> But yes, putting your patch in first and then Oleg's on top means that
> it works regardless.
> 
> Any opinions from others?

No pb here, and this one is reasonably easy to backport anyway as the
test is easy to locate.

Willy

[toc] | [prev] | [next] | [standalone]

#1681723 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed 05-07-17 20:53:02, Michal Hocko wrote:
> On Wed 05-07-17 11:35:51, Linus Torvalds wrote:
> > On Wed, Jul 5, 2017 at 11:28 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > Dohh, that was on mmotm which has a clean up by Oleg which reorganizes
> > > the code a bit. This is on top of the current master
> > 
> > Oh, ok. I think I know which patch from Oleg you're talking about.
> > 
> > Since I do want that patch too, and since I'd hate to cause
> > unnecessary merge conflicts in this area, how about we just plan on
> > letting your original patch (on top of Oleg's) go through Andrew and
> > the -mm tree? I'll get it that way, and it's not like this is
> > timing-critical.
> 
> That would lead to conflicts when backporting to stable trees though
> which is quite annoying as well and arguably slightly more annoying than
> resolving this in mmotm. I can help to rebase Oleg's patch on top of
> mine which is not a stable material.

Here is the rebase of Oleg's patch.
---
From 61ff0cd972dac218390a5859b89ce386db731d1d Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Fri, 30 Jun 2017 10:19:00 +0200
Subject: [PATCH] mm/mmap.c: expand_downwards: don't require the gap if
 !vm_prev

expand_stack(vma) fails if address < stack_guard_gap even if there is no
vma->vm_prev.  I don't think this makes sense, and we didn't do this
before the recent commit 1be7107fbe18 ("mm: larger stack guard gap,
between vmas").  We do not need a gap in this case, any address is fine as
long as security_mmap_addr() doesn't object.

This also simplifies the code, we know that address >= prev->vm_end and
thus underflow is not possible.

Link: http://lkml.kernel.org/r/20170628175258.GA24881@redhat.com
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Larry Woodman <lwoodman@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/mmap.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index ece0f6d3a1b5..f30847405cab 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2316,7 +2316,6 @@ int expand_downwards(struct vm_area_struct *vma,
 {
 	struct mm_struct *mm = vma->vm_mm;
 	struct vm_area_struct *prev;
-	unsigned long gap_addr;
 	int error;
 
 	address &= PAGE_MASK;
@@ -2325,15 +2324,12 @@ int expand_downwards(struct vm_area_struct *vma,
 		return error;
 
 	/* Enforce stack_guard_gap */
-	gap_addr = address - stack_guard_gap;
-	if (gap_addr > address)
-		return -ENOMEM;
 	prev = vma->vm_prev;
-	if (prev && prev->vm_end > gap_addr &&
+	/* Check that both stack segments have the same anon_vma? */
+	if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
 			(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
-		if (!(prev->vm_flags & VM_GROWSDOWN))
+		if (address - prev->vm_end < stack_guard_gap)
 			return -ENOMEM;
-		/* Check that both stack segments have the same anon_vma? */
 	}
 
 	/* We must make sure the anon_vma is allocated. */
-- 
2.11.0

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1681808 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed, 5 Jul 2017 20:28:49 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> "mm: enlarge stack guard gap" has introduced a regression in some rust
> and Java environments which are trying to implement their own stack
> guard page.  They are punching a new MAP_FIXED mapping inside the
> existing stack Vma.
> 
> This will confuse expand_{downwards,upwards} into thinking that the stack
> expansion would in fact get us too close to an existing non-stack vma
> which is a correct behavior wrt. safety. It is a real regression on
> the other hand. Let's work around the problem by considering PROT_NONE
> mapping as a part of the stack. This is a gros hack but overflowing to
> such a mapping would trap anyway an we only can hope that usespace
> knows what it is doing and handle it propely.
> 
> Fixes: d4d2d35e6ef9 ("mm: larger stack guard gap, between vmas")

That should be 1be7107fbe18, yes?

> Debugged-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

[toc] | [prev] | [next] | [standalone]

#1681870 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed, Jul 5, 2017 at 2:18 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 5 Jul 2017 20:28:49 +0200 Michal Hocko <mhocko@kernel.org> wrote:
>>>
>> Fixes: d4d2d35e6ef9 ("mm: larger stack guard gap, between vmas")
>
> That should be 1be7107fbe18, yes?

Good catch. I assume the d4d2d35e6ef9 is one of the stable backport commits..

              Linus

[toc] | [prev] | [next] | [standalone]

#1682074 — Re: [PATCH] mm: mm, mmap: do not blow on PROT_NONE MAP_FIXED holes in the stack

On Wed 05-07-17 14:18:49, Andrew Morton wrote:
> On Wed, 5 Jul 2017 20:28:49 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > "mm: enlarge stack guard gap" has introduced a regression in some rust
> > and Java environments which are trying to implement their own stack
> > guard page.  They are punching a new MAP_FIXED mapping inside the
> > existing stack Vma.
> > 
> > This will confuse expand_{downwards,upwards} into thinking that the stack
> > expansion would in fact get us too close to an existing non-stack vma
> > which is a correct behavior wrt. safety. It is a real regression on
> > the other hand. Let's work around the problem by considering PROT_NONE
> > mapping as a part of the stack. This is a gros hack but overflowing to
> > such a mapping would trap anyway an we only can hope that usespace
> > knows what it is doing and handle it propely.
> > 
> > Fixes: d4d2d35e6ef9 ("mm: larger stack guard gap, between vmas")
> 
> That should be 1be7107fbe18, yes?

yes. d4d2d35e6ef9 was a cherry-pick into the mmotm git tree. Sorry about
that.

> > Debugged-by: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web