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

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 3992 bytes --]

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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]