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

Groups > linux.kernel > #1680619 > unrolled thread

Re: [PATCH] mm: larger stack guard gap, between vmas

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-04 02:00 +0200
    Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 02:10 +0200
      Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 10:50 +0200
        Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 11:40 +0200
          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 11:50 +0200
            Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 12:50 +0200
              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-04 13:40 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 14:10 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 14:20 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-04 14:30 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 14:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Ximin Luo <infinity0@debian.org> - 2017-07-04 16:30 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 16:50 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 18:00 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 19:30 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 20:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 20:50 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 21:10 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 20:50 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 18:20 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas John Haxby <john.haxby@oracle.com> - 2017-07-04 18:30 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 19:10 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 19:20 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 01:10 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 01:40 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 08:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 10:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 10:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 11:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 16:00 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 16:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 18:10 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-06 09:40 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 16:30 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 17:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 18:10 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 19:00 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 19:10 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 19:30 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 19:20 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-06 01:40 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-06 02:00 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-06 10:30 +0200
                                    Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-06 12:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-05 18:20 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 18:30 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-05 19:30 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 21:40 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 22:50 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@amacapital.net> - 2017-07-05 23:00 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-06 02:00 +0200
                                    Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-06 02:30 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Kees Cook <keescook@chromium.org> - 2017-07-06 02:00 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-06 02:00 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-06 02:40 +0200
                                    Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-06 02:50 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-06 02:30 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Kees Cook <keescook@chromium.org> - 2017-07-06 04:50 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-06 07:30 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Kevin Easton <kevin@guarana.org> - 2017-07-06 07:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 18:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 21:10 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 21:20 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 21:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 21:20 +0200
                  Re: [vs-plain] Re: [PATCH] mm: larger stack guard gap, between vmas "kseifried@redhat.com" <kseifried@redhat.com> - 2017-07-05 03:20 +0200
                    Re: [vs-plain] Re: [PATCH] mm: larger stack guard gap, between vmas Solar Designer <solar@openwall.com> - 2017-07-05 16:20 +0200
          Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 12:50 +0200
            Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 13:00 +0200
    Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-04 02:30 +0200
    Re: [vs-plain] Re: [PATCH] mm: larger stack guard gap, between vmas John Haxby <john.haxby@oracle.com> - 2017-07-04 14:30 +0200

Page 1 of 4  [1] 2 3 4  Next page →

#1680619 — Re: [PATCH] mm: larger stack guard gap, between vmas

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2017-06-21 at 11:47 +0100, Ben Hutchings wrote:
> On Wed, 2017-06-21 at 11:24 +0200, Michal Hocko wrote:
> > On Wed 21-06-17 02:38:21, Ben Hutchings wrote:
> > > On Mon, 2017-06-19 at 16:23 +0200, Willy Tarreau wrote:
> > > > On Mon, Jun 19, 2017 at 08:44:24PM +0800, Linus Torvalds wrote:
> > > > > The distros are in a different situation and don't have that
> > > > > two-week
> > > > > window until a release, and presumably would not want to cut
> > > > > over to
> > > > > something new and fairly untested on such short notice.
> > > > > 
> > > > > The timing for this all sucks, but if somebody has some final
> > > > > comments, please speak up now..
> > > > 
> > > > What do you suggest the stable maintainers do here ? I've just
> > > > backported
> > > > this patch back to 3.10 and could boot it on i386 where it
> > > > apparently
> > > > works. But we may need more tests. On the other hand we benefit
> > > > from the
> > > > automated tests on tens of platforms when we push the queues so
> > > > at least
> > > > we'll quickly know if it builds and boots. I just don't feel
> > > > confident in
> > > > my work just because it builds and boots, you know.
> > > > 
> > > > I'm appending the patches I currently have if anyone wants to
> > > > have a
> > > > glance. Ben, 3.2 requires much more changes than 3.10 and I'm
> > > > pretty
> > > > sure you won't change your patches at the last minute so I gave
> > > > up.
> > > 
> > > Well I'm now dealing with fall-out from the Debian stable updates,
> > > which used a backport of Michal's patch series.  That unfortunately
> > > seems to break programs running Java code in the main thread (the
> > > 'java' command doesn't do this, but e.g. 'jsvc' does).
> > 
> > Could you share more details please?
> 
> https://bugs.debian.org/865303
> https://bugs.debian.org/865311
> https://bugs.debian.org/865343

Unfortunately these regressions have not been completely fixed by
switching to Hugh's fix.

Firstly, some Rust programs are crashing on ppc64el with 64 KiB pages. 
Apparently Rust maps its own guard page at the lower limit of the stack
(determined using pthread_getattr_np() and pthread_attr_getstack()).  I
don't think this ever actually worked for the main thread stack, but it
now also blocks expansion as the default stack size of 8 MiB is smaller
than the stack gap of 16 MiB.  Would it make sense to skip over
PROT_NONE mappings when checking whether it's safe to expand?

Secondly, LibreOffice is crashing on i386 when running components
implemented in Java.  I don't have a diagnosis for this yet.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

[toc] | [next] | [standalone]

#1680621

On Mon, Jul 3, 2017 at 4:55 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>
> Firstly, some Rust programs are crashing on ppc64el with 64 KiB pages.
> Apparently Rust maps its own guard page at the lower limit of the stack
> (determined using pthread_getattr_np() and pthread_attr_getstack()).  I
> don't think this ever actually worked for the main thread stack, but it
> now also blocks expansion as the default stack size of 8 MiB is smaller
> than the stack gap of 16 MiB.  Would it make sense to skip over
> PROT_NONE mappings when checking whether it's safe to expand?

Hmm. Maybe.

Also, the whole notion that the gap should be relative to the page
size never made sense to me. So I think we could/should just make the
default gap size be one megabyte, not that "256 pages" abortion.

> Secondly, LibreOffice is crashing on i386 when running components
> implemented in Java.  I don't have a diagnosis for this yet.

Ugh. Nobody seeing this inside SuSe/Red Hat? I don't think I've heard
about this..

                Linus

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

#1680776

On Mon 03-07-17 17:05:27, Linus Torvalds wrote:
> On Mon, Jul 3, 2017 at 4:55 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> >
> > Firstly, some Rust programs are crashing on ppc64el with 64 KiB pages.
> > Apparently Rust maps its own guard page at the lower limit of the stack
> > (determined using pthread_getattr_np() and pthread_attr_getstack()).  I
> > don't think this ever actually worked for the main thread stack, but it
> > now also blocks expansion as the default stack size of 8 MiB is smaller
> > than the stack gap of 16 MiB.  Would it make sense to skip over
> > PROT_NONE mappings when checking whether it's safe to expand?

This is what my workaround for the older patch was doing, actually. We
have deployed that as a follow up fix on our older code bases. And this
has fixed verious issues with Java which was doing the similar thing.

> Hmm. Maybe.
> 
> Also, the whole notion that the gap should be relative to the page
> size never made sense to me. So I think we could/should just make the
> default gap size be one megabyte, not that "256 pages" abortion.

The reason for having this in page units was that MAX_ARG_STRLEN is in
page units as well. And this is used as an on stack variable quite
often. 1MB wouldn't be sufficient for that to cover - we could go with a
larger gap but who knows how many other traps are there.

> > Secondly, LibreOffice is crashing on i386 when running components
> > implemented in Java.  I don't have a diagnosis for this yet.
> 
> Ugh. Nobody seeing this inside SuSe/Red Hat? I don't think I've heard
> about this..

No reports yet but we do not support 32b kernels on newer kernels which
had the upstream fix.

-- 
Michal Hocko
SUSE Labs

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

#1680806

On Tue 04-07-17 10:41:22, Michal Hocko wrote:
> On Mon 03-07-17 17:05:27, Linus Torvalds wrote:
> > On Mon, Jul 3, 2017 at 4:55 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > >
> > > Firstly, some Rust programs are crashing on ppc64el with 64 KiB pages.
> > > Apparently Rust maps its own guard page at the lower limit of the stack
> > > (determined using pthread_getattr_np() and pthread_attr_getstack()).  I
> > > don't think this ever actually worked for the main thread stack, but it
> > > now also blocks expansion as the default stack size of 8 MiB is smaller
> > > than the stack gap of 16 MiB.  Would it make sense to skip over
> > > PROT_NONE mappings when checking whether it's safe to expand?
> 
> This is what my workaround for the older patch was doing, actually. We
> have deployed that as a follow up fix on our older code bases. And this
> has fixed verious issues with Java which was doing the similar thing.

Here is a forward port (on top of the current Linus tree) of my earlier
patch. I have dropped a note about java stack trace because this would
most likely be not the case with the Hugh's patch. The problem is the
same in principle though. Note I didn't get to test this properly yet
but it should be pretty much obvious.
---
From d9f6faccf2c286ed81fbc860c9b0b7fe23ef0836 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 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

-- 
Michal Hocko
SUSE Labs

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

#1680819

On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
> On Tue 04-07-17 10:41:22, Michal Hocko wrote:
> > On Mon 03-07-17 17:05:27, Linus Torvalds wrote:
> > > On Mon, Jul 3, 2017 at 4:55 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > > >
> > > > Firstly, some Rust programs are crashing on ppc64el with 64 KiB pages.
> > > > Apparently Rust maps its own guard page at the lower limit of the stack
> > > > (determined using pthread_getattr_np() and pthread_attr_getstack()).  I
> > > > don't think this ever actually worked for the main thread stack, but it
> > > > now also blocks expansion as the default stack size of 8 MiB is smaller
> > > > than the stack gap of 16 MiB.  Would it make sense to skip over
> > > > PROT_NONE mappings when checking whether it's safe to expand?
> > 
> > This is what my workaround for the older patch was doing, actually. We
> > have deployed that as a follow up fix on our older code bases. And this
> > has fixed verious issues with Java which was doing the similar thing.
> 
> Here is a forward port (on top of the current Linus tree) of my earlier
> patch. I have dropped a note about java stack trace because this would
> most likely be not the case with the Hugh's patch. The problem is the
> same in principle though. Note I didn't get to test this properly yet
> but it should be pretty much obvious.
> ---
> >From d9f6faccf2c286ed81fbc860c9b0b7fe23ef0836 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 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;
>  	}

But wouldn't this completely disable the check in case such a guard page
is installed, and possibly continue to allow the collision when the stack
allocation is large enough to skip this guard page ? Shouldn't we instead
"skip" such a vma and look for the next one ?

I was thinking about something more like :

	prev = vma->vm_prev;
+	/* Don't consider a possible user-space stack guard page */
+	if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
+	    !(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC)))
+		prev = prev->vm_prev;
+
       /* Check that both stack segments have the same anon_vma? */

Willy

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

#1680859

On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
> On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
> > On Tue 04-07-17 10:41:22, Michal Hocko wrote:
> > > On Mon 03-07-17 17:05:27, Linus Torvalds wrote:
> > > > On Mon, Jul 3, 2017 at 4:55 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > > > >
> > > > > Firstly, some Rust programs are crashing on ppc64el with 64 KiB pages.
> > > > > Apparently Rust maps its own guard page at the lower limit of the stack
> > > > > (determined using pthread_getattr_np() and pthread_attr_getstack()).  I
> > > > > don't think this ever actually worked for the main thread stack, but it
> > > > > now also blocks expansion as the default stack size of 8 MiB is smaller
> > > > > than the stack gap of 16 MiB.  Would it make sense to skip over
> > > > > PROT_NONE mappings when checking whether it's safe to expand?
> > > 
> > > This is what my workaround for the older patch was doing, actually. We
> > > have deployed that as a follow up fix on our older code bases. And this
> > > has fixed verious issues with Java which was doing the similar thing.
> > 
> > Here is a forward port (on top of the current Linus tree) of my earlier
> > patch. I have dropped a note about java stack trace because this would
> > most likely be not the case with the Hugh's patch. The problem is the
> > same in principle though. Note I didn't get to test this properly yet
> > but it should be pretty much obvious.
> > ---
> > >From d9f6faccf2c286ed81fbc860c9b0b7fe23ef0836 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 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;
> >  	}
> 
> But wouldn't this completely disable the check in case such a guard page
> is installed, and possibly continue to allow the collision when the stack
> allocation is large enough to skip this guard page ?

Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
that userspace does the right thing.

> Shouldn't we instead
> "skip" such a vma and look for the next one ?

Yeah, that would be possible, I am not sure it is worth it though. The
gap as it is implemented now prevents regular mappings to get close to
the stack. So we only care about those with MAP_FIXED and those can
screw things already so we really have to rely on userspace doing some
semi reasonable.

> I was thinking about something more like :
> 
> 	prev = vma->vm_prev;
> +	/* Don't consider a possible user-space stack guard page */
> +	if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
> +	    !(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC)))
> +		prev = prev->vm_prev;
> +

If anywhing this would require to have a loop over all PROT_NONE
mappings to not hit into other weird usecases.

>        /* Check that both stack segments have the same anon_vma? */
> 
> Willy

-- 
Michal Hocko
SUSE Labs

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

#1680889

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2017-07-04 at 12:42 +0200, Michal Hocko wrote:
> On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
> > On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
[...]
> > But wouldn't this completely disable the check in case such a guard page
> > is installed, and possibly continue to allow the collision when the stack
> > allocation is large enough to skip this guard page ?
> 
> Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
> that userspace does the right thing.

It may well not be large enough, because of the same wrong assumptions
that resulted in the kernel's guard page not being large enough.  We
should count it as part of the guard gap but not a substitute.

> > Shouldn't we instead
> > "skip" such a vma and look for the next one ?
> 
> Yeah, that would be possible, I am not sure it is worth it though. The
> gap as it is implemented now prevents regular mappings to get close to
> the stack. So we only care about those with MAP_FIXED and those can
> screw things already so we really have to rely on userspace doing some
> semi reasonable.
> 
> > I was thinking about something more like :
> > 
> > 	prev = vma->vm_prev;
> > +	/* Don't consider a possible user-space stack guard page */
> > +	if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
> > +	    !(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC)))
> > +		prev = prev->vm_prev;
> > +
> 
> If anywhing this would require to have a loop over all PROT_NONE
> mappings to not hit into other weird usecases.

That's what I was thinking of.  Tried the following patch:

Subject: mmap: Ignore VM_NONE mappings when checking for space to
 expand the stack

Some user-space run-times (in particular, Java and Rust) allocate
their own guard pages in the main stack.  This didn't work well
before, but it can now block stack expansion where it is safe and would
previously have been allowed.  Ignore such mappings when checking the
size of the gap before expanding.

Reported-by: Ximin Luo <infinity0@debian.org>
References: https://bugs.debian.org/865416
Fixes: 1be7107fbe18 ("mm: larger stack guard gap, between vmas")
Cc: stable@vger.kernel.org
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 mm/mmap.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index a5e3dcd75e79..19f3ce04f24f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2243,7 +2243,14 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 	if (gap_addr < address || gap_addr > TASK_SIZE)
 		gap_addr = TASK_SIZE;
 
-	next = vma->vm_next;
+	/*
+	 * Allow VM_NONE mappings in the gap as some applications try
+	 * to make their own stack guards
+	 */
+	for (next = vma->vm_next;
+	     next && !(next->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
+	     next = next->vm_next)
+		;
 	if (next && next->vm_start < gap_addr) {
 		if (!(next->vm_flags & VM_GROWSUP))
 			return -ENOMEM;
@@ -2323,11 +2330,17 @@ int expand_downwards(struct vm_area_struct *vma,
 	if (error)
 		return error;
 
-	/* Enforce stack_guard_gap */
+	/*
+	 * Enforce stack_guard_gap, but allow VM_NONE mappings in the gap
+	 * as some applications try to make their own stack guards
+	 */
 	gap_addr = address - stack_guard_gap;
 	if (gap_addr > address)
 		return -ENOMEM;
-	prev = vma->vm_prev;
+	for (prev = vma->vm_prev;
+	     prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
+	     prev = prev->vm_prev)
+		;
 	if (prev && prev->vm_end > gap_addr) {
 		if (!(prev->vm_flags & VM_GROWSDOWN))
 			return -ENOMEM;
--- END ---

I don't have a ppc64el machine where I can change the kernel, but I
tried this on x86_64 with the stack limit reduced to 1 MiB and Rust
is able to expand its stack where previously it would crash.

This *doesn't* fix the LibreOffice regression on i386.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

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

#1680901

On Tue 04-07-17 12:36:11, Ben Hutchings wrote:
> On Tue, 2017-07-04 at 12:42 +0200, Michal Hocko wrote:
> > On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
> > > On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
> [...]
> > > But wouldn't this completely disable the check in case such a guard page
> > > is installed, and possibly continue to allow the collision when the stack
> > > allocation is large enough to skip this guard page ?
> > 
> > Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
> > that userspace does the right thing.
> 
> It may well not be large enough, because of the same wrong assumptions
> that resulted in the kernel's guard page not being large enough.  We
> should count it as part of the guard gap but not a substitute.

yes, you are right of course. But isn't this a bug on their side
considering they are managing their _own_ stack gap? Our stack gap
management is a best effort thing and two such approaches competing will
always lead to weird cornercases. That was my assumption when saying
that I am not sure this is really _worth_ it. We should definitely try
to workaround clashes but that's about it. If others think that we
should do everything to prevent even those issues I will not oppose
of course. It just adds more cycles to something that is a weird case
already.

[...]

> This *doesn't* fix the LibreOffice regression on i386.

Are there any details about this regression?

-- 
Michal Hocko
SUSE Labs

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

#1680908

On Tue 04-07-17 13:59:59, Michal Hocko wrote:
> On Tue 04-07-17 12:36:11, Ben Hutchings wrote:
> > On Tue, 2017-07-04 at 12:42 +0200, Michal Hocko wrote:
> > > On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
> > > > On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
> > [...]
> > > > But wouldn't this completely disable the check in case such a guard page
> > > > is installed, and possibly continue to allow the collision when the stack
> > > > allocation is large enough to skip this guard page ?
> > > 
> > > Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
> > > that userspace does the right thing.
> > 
> > It may well not be large enough, because of the same wrong assumptions
> > that resulted in the kernel's guard page not being large enough.  We
> > should count it as part of the guard gap but not a substitute.
> 
> yes, you are right of course. But isn't this a bug on their side
> considering they are managing their _own_ stack gap? Our stack gap
> management is a best effort thing and two such approaches competing will
> always lead to weird cornercases. That was my assumption when saying
> that I am not sure this is really _worth_ it. We should definitely try
> to workaround clashes but that's about it. If others think that we
> should do everything to prevent even those issues I will not oppose
> of course. It just adds more cycles to something that is a weird case
> already.

Forgot to mention another point. Currently we do not check other
previous vmas if prev->vm_flags & VM_GROWSDOWN. Consider that the stack
gap is implemented by mprotect. This wouldn't change the VM_GROWSDOWN
flag and we are back to square 1 because the gap might be too small. Do
we want/need to handle those cases. Are they too different from
MAP_FIXED gaps? I am not so sure but I would be inclined to say no.

-- 
Michal Hocko
SUSE Labs

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

#1680912

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2017-07-04 at 14:00 +0200, Michal Hocko wrote:
> On Tue 04-07-17 12:36:11, Ben Hutchings wrote:
> > On Tue, 2017-07-04 at 12:42 +0200, Michal Hocko wrote:
> > > On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
> > > > On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
> > 
> > [...]
> > > > But wouldn't this completely disable the check in case such a guard page
> > > > is installed, and possibly continue to allow the collision when the stack
> > > > allocation is large enough to skip this guard page ?
> > > 
> > > Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
> > > that userspace does the right thing.
> > 
> > It may well not be large enough, because of the same wrong assumptions
> > that resulted in the kernel's guard page not being large enough.  We
> > should count it as part of the guard gap but not a substitute.
> 
> yes, you are right of course. But isn't this a bug on their side
> considering they are managing their _own_ stack gap?

Yes it's their bug, but you know the rule - don't break user-space.

> Our stack gap
> management is a best effort thing and two such approaches competing will
> always lead to weird cornercases. That was my assumption when saying
> that I am not sure this is really _worth_ it. We should definitely try
> to workaround clashes but that's about it. If others think that we
> should do everything to prevent even those issues I will not oppose
> of course. It just adds more cycles to something that is a weird case
> already.

I don't want odd behaviour to weaken the stack guard.

> [...]
> 
> > This *doesn't* fix the LibreOffice regression on i386.
> 
> Are there any details about this regression?

Here:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=865303#170

I haven't reproduced it in Writer, but if I use Base to create a new
HSQLDB database it reliably crashes (HSQLDB is implemented in Java).

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

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

#1680918

On Tue 04-07-17 13:21:02, Ben Hutchings wrote:
> On Tue, 2017-07-04 at 14:00 +0200, Michal Hocko wrote:
> > On Tue 04-07-17 12:36:11, Ben Hutchings wrote:
> > > On Tue, 2017-07-04 at 12:42 +0200, Michal Hocko wrote:
> > > > On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
> > > > > On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
> > > 
> > > [...]
> > > > > But wouldn't this completely disable the check in case such a guard page
> > > > > is installed, and possibly continue to allow the collision when the stack
> > > > > allocation is large enough to skip this guard page ?
> > > > 
> > > > Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
> > > > that userspace does the right thing.
> > > 
> > > It may well not be large enough, because of the same wrong assumptions
> > > that resulted in the kernel's guard page not being large enough.  We
> > > should count it as part of the guard gap but not a substitute.
> > 
> > yes, you are right of course. But isn't this a bug on their side
> > considering they are managing their _own_ stack gap?
> 
> Yes it's their bug, but you know the rule - don't break user-space.

Absolutely, that is why I belive we should consider the prev VMA but
doing anything more just risks for new regressions. Or why do you think
that not-checking them would cause a regression?

> > Our stack gap
> > management is a best effort thing and two such approaches competing will
> > always lead to weird cornercases. That was my assumption when saying
> > that I am not sure this is really _worth_ it. We should definitely try
> > to workaround clashes but that's about it. If others think that we
> > should do everything to prevent even those issues I will not oppose
> > of course. It just adds more cycles to something that is a weird case
> > already.
> 
> I don't want odd behaviour to weaken the stack guard.
> 
> > [...]
> > 
> > > This *doesn't* fix the LibreOffice regression on i386.
> > 
> > Are there any details about this regression?
> 
> Here:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=865303#170
> 
> I haven't reproduced it in Writer, but if I use Base to create a new
> HSQLDB database it reliably crashes (HSQLDB is implemented in Java).

I haven't read through previous 169 comments but I do not see any stack
trace. Ideally with info proc mapping that would tell us the memory
layout.

-- 
Michal Hocko
SUSE Labs

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

#1681020

Michal Hocko:
> On Tue 04-07-17 13:21:02, Ben Hutchings wrote:
>> On Tue, 2017-07-04 at 14:00 +0200, Michal Hocko wrote:
>>> On Tue 04-07-17 12:36:11, Ben Hutchings wrote:
>>>> On Tue, 2017-07-04 at 12:42 +0200, Michal Hocko wrote:
>>>>> On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
>>>>>> On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
>>>>
>>>> [...]
>>>>>> But wouldn't this completely disable the check in case such a guard page
>>>>>> is installed, and possibly continue to allow the collision when the stack
>>>>>> allocation is large enough to skip this guard page ?
>>>>>
>>>>> Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
>>>>> that userspace does the right thing.
>>>>
>>>> It may well not be large enough, because of the same wrong assumptions
>>>> that resulted in the kernel's guard page not being large enough.  We
>>>> should count it as part of the guard gap but not a substitute.
>>>
>>> yes, you are right of course. But isn't this a bug on their side
>>> considering they are managing their _own_ stack gap?
>>
>> Yes it's their bug, but you know the rule - don't break user-space.
> 
> Absolutely, that is why I belive we should consider the prev VMA but
> doing anything more just risks for new regressions. Or why do you think
> that not-checking them would cause a regression?
> 
>>> Our stack gap
>>> management is a best effort thing and two such approaches competing will
>>> always lead to weird cornercases. That was my assumption when saying
>>> that I am not sure this is really _worth_ it. We should definitely try
>>> to workaround clashes but that's about it. If others think that we
>>> should do everything to prevent even those issues I will not oppose
>>> of course. It just adds more cycles to something that is a weird case
>>> already.
>>
>> I don't want odd behaviour to weaken the stack guard.
>>
>>> [...]
>>>
>>>> This *doesn't* fix the LibreOffice regression on i386.
>>>
>>> Are there any details about this regression?
>>
>> Here:
>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=865303#170
>>
>> I haven't reproduced it in Writer, but if I use Base to create a new
>> HSQLDB database it reliably crashes (HSQLDB is implemented in Java).
> 
> I haven't read through previous 169 comments but I do not see any stack
> trace. Ideally with info proc mapping that would tell us the memory
> layout.
> 

I've written up an explanation of what happens in the Rust case here:

https://github.com/rust-lang/rust/issues/43052

Hopefully I got the details about Linux correct - I only had them explained to me last night - please reply on that page if not.

X

-- 
GPG: ed25519/56034877E1F87C35
GPG: rsa4096/1318EFAC5FBBDBCE
https://github.com/infinity0/pubkeys.git

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

#1681028

On Tue 04-07-17 14:19:00, Ximin Luo wrote:
[...]
> I've written up an explanation of what happens in the Rust case here:
> 
> https://github.com/rust-lang/rust/issues/43052

The most important part is https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/thread.rs#L248
        // Rellocate the last page of the stack.
        // This ensures SIGBUS will be raised on
        // stack overflow.
        let result = mmap(stackaddr, psize, PROT_NONE, MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0);

so this is basically the same thing Java does. Except that Java doesn't
do that on main thread usually. Only some JNI runtimes do that.
pthread_attr_getstack() usage on the main thread sounds like a real bug
in rust to me.

Thanks for the writeup!
-- 
Michal Hocko
SUSE Labs

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

#1681070

On Tue, Jul 04, 2017 at 12:36:11PM +0100, Ben Hutchings wrote:
> > If anywhing this would require to have a loop over all PROT_NONE
> > mappings to not hit into other weird usecases.
> 
> That's what I was thinking of.  Tried the following patch:
(...)
> -	next = vma->vm_next;
> +	/*
> +	 * Allow VM_NONE mappings in the gap as some applications try
> +	 * to make their own stack guards
> +	 */
> +	for (next = vma->vm_next;
> +	     next && !(next->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
> +	     next = next->vm_next)
> +		;

That's what I wanted to propose but I feared someone would scream at me
for this loop :-)

+1 for me!

Willy

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

#1681102

On Tue 04-07-17 17:51:40, Willy Tarreau wrote:
> On Tue, Jul 04, 2017 at 12:36:11PM +0100, Ben Hutchings wrote:
> > > If anywhing this would require to have a loop over all PROT_NONE
> > > mappings to not hit into other weird usecases.
> > 
> > That's what I was thinking of.  Tried the following patch:
> (...)
> > -	next = vma->vm_next;
> > +	/*
> > +	 * Allow VM_NONE mappings in the gap as some applications try
> > +	 * to make their own stack guards
> > +	 */
> > +	for (next = vma->vm_next;
> > +	     next && !(next->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
> > +	     next = next->vm_next)
> > +		;
> 
> That's what I wanted to propose but I feared someone would scream at me
> for this loop :-)

Well, I've been thinking about this some more and the more I think about
it the less I am convinced we should try to be clever here. Why? Because
as soon as somebody tries to manage stacks explicitly you cannot simply
assume anything about the previous mapping. Say some interpret uses
[ mngmnt data][red zone]                 <--[- MAP_GROWSDOWN ]

Now if we consider the red zone's (PROT_NONE) prev mapping we would fail
the expansion even though we haven't hit the red zone and that is
essentially what the Java and rust bugs are about. So we just risk yet
another regression.

Now let's say another example
<--[- MAP_GROWSDOWN][red zone]   <--[- MAP_GROWSDOWN]
   thread 1                            thread 2

Does the more clever code prevent from smashing over unrelated stack?
No because of our VM_GROWS{DOWN,UP} checks which are needed for other
cases. Well we could special case those as well but...

That being said, I am not really convinced that mixing 2 different gap
implemetantions is sane. I guess it should be reasonable to assume that
a PROT_NONE mapping close to the stack is meant to be a red zone and at
this moment we should rather back off and rely on the userspace rather
than risk more weird cornercases and regressions.

-- 
Michal Hocko
SUSE Labs

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

#1681123

On Tue, Jul 4, 2017 at 10:22 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> Well, I've been thinking about this some more and the more I think about
> it the less I am convinced we should try to be clever here. Why? Because
> as soon as somebody tries to manage stacks explicitly you cannot simply
> assume anything about the previous mapping. Say some interpret uses
> [ mngmnt data][red zone]                 <--[- MAP_GROWSDOWN ]
>
> Now if we consider the red zone's (PROT_NONE) prev mapping we would fail
> the expansion even though we haven't hit the red zone and that is
> essentially what the Java and rust bugs are about. So we just risk yet
> another regression.

Ack.

Let's make the initial version at least only check the first vma.

The long-term fix for this is to have the binaries do proper stack
expansion probing anyway, and it's quite possible that people who do
their own stack redzoning by adding a PROT_NONE thing already do that
proper fix (eg the Java stack may simply not *have* those big crazy
structures on it in the first place).

            Linus

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

#1681126

On Tue, Jul 4, 2017 at 11:39 AM, Willy Tarreau <w@1wt.eu> wrote:
>
> But what is wrong with stopping the loop as soon as the distance gets
> larger than the stack_guard_gap ?

Absolutely nothing. But that's not the problem with the loop. Let's
say that you are using lots of threads, so that you know your stack
space is limited. What you do is to use MAP_FIXED a lot, and you lay
out your stacks fairly densely (with each other, but also possibly
with other mappings), with that PROT_NONE redzoning mapping in between
the "dense" allocations.

So when the kernel wants to grow the stack, it finds the PROT_NONE
redzone mapping - but there's possibly other maps right under it, so
the stack_guard_gap still hits other mappings.

And the fact that this seems to trigger with
 (a) 32-bit x86
 (b) Java
actually makes sense in the above scenario: that's _exactly_ when
you'd have dense mappings. Java is very thread-happy, and in a 32-bit
VM, the virtual address space allocation for stacks is a primary issue
with lots of threads.

Of course, the downside to this theory is that apparently the Java
problem is not confirmed to actually be due to this (Ben root-caused
the rust thing on ppc64), but it still sounds like quite a reasonable
thing to do.

The problem with the Java issue may be that they do that "dense stack
mappings in VM space" (for all the usual "lots of threads, limited VM"
reasons), but they may *not* have that PROT_NONE redzoning at all.

So the patch under discussion works for Rust exactly *because* it does
its redzone to show "this is where I expect the stack to end". The
i386 java load may simply not have that marker for us to use..

               Linus

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

#1681135

On Tue, Jul 04, 2017 at 11:47:37AM -0700, Linus Torvalds wrote:
> Let's
> say that you are using lots of threads, so that you know your stack
> space is limited. What you do is to use MAP_FIXED a lot, and you lay
> out your stacks fairly densely (with each other, but also possibly
> with other mappings), with that PROT_NONE redzoning mapping in between
> the "dense" allocations.
> 
> So when the kernel wants to grow the stack, it finds the PROT_NONE
> redzone mapping - but there's possibly other maps right under it, so
> the stack_guard_gap still hits other mappings.
(...)

OK I didn't get that use case, that totally makes sense indeed! So
now we use PROT_NONE not as something that must be skipped to find
the unmapped area but as a hint that the application apparently
wants the stack to stop here.

Thanks for this clear explanation!

Willy

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

#1681127

On Tue, Jul 04, 2017 at 11:37:15AM -0700, Linus Torvalds wrote:
> On Tue, Jul 4, 2017 at 10:22 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > Well, I've been thinking about this some more and the more I think about
> > it the less I am convinced we should try to be clever here. Why? Because
> > as soon as somebody tries to manage stacks explicitly you cannot simply
> > assume anything about the previous mapping. Say some interpret uses
> > [ mngmnt data][red zone]                 <--[- MAP_GROWSDOWN ]
> >
> > Now if we consider the red zone's (PROT_NONE) prev mapping we would fail
> > the expansion even though we haven't hit the red zone and that is
> > essentially what the Java and rust bugs are about. So we just risk yet
> > another regression.
> 
> Ack.
> 
> Let's make the initial version at least only check the first vma.
> 
> The long-term fix for this is to have the binaries do proper stack
> expansion probing anyway, and it's quite possible that people who do
> their own stack redzoning by adding a PROT_NONE thing already do that
> proper fix (eg the Java stack may simply not *have* those big crazy
> structures on it in the first place).

But what is wrong with stopping the loop as soon as the distance gets
larger than the stack_guard_gap ?

Willy

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

#1681079

On Tue, Jul 4, 2017 at 4:36 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>
> That's what I was thinking of.  Tried the following patch:
>
> Subject: mmap: Ignore VM_NONE mappings when checking for space to
>  expand the stack

This looks sane to me.

I'm going to ignore it in this thread, and assume that it gets sent as
a patch separately, ok?

It would be good to have more acks on it.

Also, separately, John Haxby kind of implied that the LibreOffice
regression on i386 is already fixed by commit f4cb767d76cf ("mm: fix
new crash in unmapped_area_topdown()").

Or was that a separate issue?

               Linus

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

Page 1 of 4  [1] 2 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web