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

Groups > linux.kernel > #1671413 > unrolled thread

Re: [PATCH 3.10 268/268] 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 3.10 268/268] mm: larger stack guard gap, between vmas Hugh Dickins <hughd@google.com> - 2017-06-21 09:10 +0200
    Re: [PATCH 3.10 268/268] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-06-21 09:20 +0200
      Re: [PATCH 3.10 268/268] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-06-21 18:30 +0200

#1671413 — Re: [PATCH 3.10 268/268] mm: larger stack guard gap, between vmas

On Mon, 19 Jun 2017, Willy Tarreau wrote:

> From: Hugh Dickins <hughd@google.com>
> 
> commit 1be7107fbe18eed3e319a6c3e83c78254b693acb upstream.

Some of these suggested adjustments below are just what comparing mine
and yours showed up, and I'm being anal in passing them on e.g. I do
like your blank line in mm.h, but Michal chose to leave it out, and
I think that the closer we keep these sources to each other,
the less trouble we shall have patching on top in future.

Which is particularly true in expand_upwards() and expand_downwards()
(and you're thinking of backporting Helge's TASK_SIZE enhancement
on top of that, though I don't think it's strictly necessary for a
stable tree).  Your patch is not wrong there: though odd to be trying
anon_vma_prepare() twice in expand_downwards(), and tiresome to have
to unlock at each error exit.  But I'd already decided in one of our
internal trees just to factor in some of Konstantin's change, that
made the ordering much more sensible there, and the two more like
each other; so recommend that 3.10 do the same, keeping it closer
to the final 4.12 code.  But you may have different priorities and
disagree with that: just suggesting.

And there is the possibility that we shall want another patch or
two on top there.  I've left a question as to whether we should be
comparing anon_vmas.  And there's a potential (but I think ignorable)
locking issue, in the case of an architecture that supports both
VM_GROWSUP and VM_GROWSDOWN: if they expand towards each other at the
same instant, they could gobble up the gap between them (they almost
certainly have different anon_vmas, so the anon_vma locking does not
protect against that).  When it gets to updating the vma tree, it is
careful to use page_table_lock to maintain the consistency of the
tree in such a case, but maybe we should do that earlier.

Then there's the FOLL_MLOCK thing, and the WARN_ON (phew, remembered
in time that you don't have VM_WARN_ON) - but keep in mind that I
have not even built this tree, let alone tested it.

Sorry if I'm being annoying, Willy: you must be heartily sick of
these patches by now!  Or, being a longtime longterm maintainer,
perhaps it's all joy for you ;-?

Hugh

diff -purN 310n/include/linux/mm.h 310h/include/linux/mm.h
--- 310n/include/linux/mm.h	2017-06-20 16:50:29.809546868 -0700
+++ 310h/include/linux/mm.h	2017-06-20 19:52:59.359942133 -0700
@@ -1595,7 +1595,6 @@ unsigned long ra_submit(struct file_ra_s
 			struct file *filp);
 
 extern unsigned long stack_guard_gap;
-
 /* Generic expand stack which grows the stack according to GROWS{UP,DOWN} */
 extern int expand_stack(struct vm_area_struct *vma, unsigned long address);
 
diff -purN 310n/mm/memory.c 310h/mm/memory.c
--- 310n/mm/memory.c	2017-06-20 16:50:29.809546868 -0700
+++ 310h/mm/memory.c	2017-06-20 19:57:14.537573559 -0700
@@ -1821,9 +1821,6 @@ long __get_user_pages(struct task_struct
 				int ret;
 				unsigned int fault_flags = 0;
 
-				/* mlock all present pages, but do not fault in new pages */
-				if (foll_flags & FOLL_MLOCK)
-					goto next_page;
 				if (foll_flags & FOLL_WRITE)
 					fault_flags |= FAULT_FLAG_WRITE;
 				if (nonblocking)
diff -purN 310n/mm/mmap.c 310h/mm/mmap.c
--- 310n/mm/mmap.c	2017-06-20 16:50:29.809546868 -0700
+++ 310h/mm/mmap.c	2017-06-20 20:48:08.409202485 -0700
@@ -892,7 +892,7 @@ again:			remove_next = 1 + (end > next->
 		else if (next)
 			vma_gap_update(next);
 		else
-			mm->highest_vm_end = end;
+			WARN_ON(mm->highest_vm_end != vm_end_gap(vma));
 	}
 	if (insert && file)
 		uprobe_mmap(insert);
@@ -2123,48 +2123,39 @@ int expand_upwards(struct vm_area_struct
 {
 	struct vm_area_struct *next;
 	unsigned long gap_addr;
-	int error;
+	int error = 0;
 
 	if (!(vma->vm_flags & VM_GROWSUP))
 		return -EFAULT;
 
-	/*
-	 * We must make sure the anon_vma is allocated
-	 * so that the anon_vma locking is not a noop.
-	 */
-	if (unlikely(anon_vma_prepare(vma)))
-		return -ENOMEM;
-	vma_lock_anon_vma(vma);
-
-	/*
-	 * vma->vm_start/vm_end cannot change under us because the caller
-	 * is required to hold the mmap_sem in read mode.  We need the
-	 * anon_vma lock to serialize against concurrent expand_stacks.
-	 * Also guard against wrapping around to address 0.
-	 */
+	/* Guard against wrapping around to address 0. */
 	address &= PAGE_MASK;
 	address += PAGE_SIZE;
-	if (!address) {
-		vma_unlock_anon_vma(vma);
+	if (!address)
 		return -ENOMEM;
-	}
-	error = 0;
 
 	/* Enforce stack_guard_gap */
 	gap_addr = address + stack_guard_gap;
-	if (gap_addr < address) {
-		vma_unlock_anon_vma(vma);
+	if (gap_addr < address)
 		return -ENOMEM;
-	}
 	next = vma->vm_next;
 	if (next && next->vm_start < gap_addr) {
-		if (!(next->vm_flags & VM_GROWSUP)) {
-			vma_unlock_anon_vma(vma);
+		if (!(next->vm_flags & VM_GROWSUP))
 			return -ENOMEM;
-		}
 		/* Check that both stack segments have the same anon_vma? */
 	}
 
+	/* We must make sure the anon_vma is allocated. */
+	if (unlikely(anon_vma_prepare(vma)))
+		return -ENOMEM;
+
+	/*
+	 * vma->vm_start/vm_end cannot change under us because the caller
+	 * is required to hold the mmap_sem in read mode.  We need the
+	 * anon_vma lock to serialize against concurrent expand_stacks.
+	 */
+	vma_lock_anon_vma(vma);
+
 	/* Somebody else might have raced and expanded it already */
 	if (address > vma->vm_end) {
 		unsigned long size, grow;
@@ -2218,46 +2209,32 @@ int expand_downwards(struct vm_area_stru
 	unsigned long gap_addr;
 	int error;
 
-	/*
-	 * We must make sure the anon_vma is allocated
-	 * so that the anon_vma locking is not a noop.
-	 */
-	if (unlikely(anon_vma_prepare(vma)))
-		return -ENOMEM;
-
 	address &= PAGE_MASK;
 	error = security_mmap_addr(address);
 	if (error)
 		return error;
 
-	vma_lock_anon_vma(vma);
-
 	/* Enforce stack_guard_gap */
 	gap_addr = address - stack_guard_gap;
-	if (gap_addr > address) {
-		vma_unlock_anon_vma(vma);
+	if (gap_addr > address)
 		return -ENOMEM;
-	}
 	prev = vma->vm_prev;
 	if (prev && prev->vm_end > gap_addr) {
-		if (!(prev->vm_flags & VM_GROWSDOWN)) {
-			vma_unlock_anon_vma(vma);
+		if (!(prev->vm_flags & VM_GROWSDOWN))
 			return -ENOMEM;
-		}
 		/* Check that both stack segments have the same anon_vma? */
 	}
 
 	/* We must make sure the anon_vma is allocated. */
-	if (unlikely(anon_vma_prepare(vma))) {
-		vma_unlock_anon_vma(vma);
+	if (unlikely(anon_vma_prepare(vma)))
 		return -ENOMEM;
-	}
 
 	/*
 	 * vma->vm_start/vm_end cannot change under us because the caller
 	 * is required to hold the mmap_sem in read mode.  We need the
 	 * anon_vma lock to serialize against concurrent expand_stacks.
 	 */
+	vma_lock_anon_vma(vma);
 
 	/* Somebody else might have raced and expanded it already */
 	if (address < vma->vm_start) {

[toc] | [next] | [standalone]

#1671425

On Wed, Jun 21, 2017 at 12:05:07AM -0700, Hugh Dickins wrote:
> On Mon, 19 Jun 2017, Willy Tarreau wrote:
> 
> > From: Hugh Dickins <hughd@google.com>
> > 
> > commit 1be7107fbe18eed3e319a6c3e83c78254b693acb upstream.
> 
> Some of these suggested adjustments below are just what comparing mine
> and yours showed up, and I'm being anal in passing them on e.g. I do
> like your blank line in mm.h, but Michal chose to leave it out, and
> I think that the closer we keep these sources to each other,
> the less trouble we shall have patching on top in future.

I totally agree, that's what I generally focus on as well.

> Which is particularly true in expand_upwards() and expand_downwards()
> (and you're thinking of backporting Helge's TASK_SIZE enhancement
> on top of that, though I don't think it's strictly necessary for a
> stable tree).

I thought it was a fix for a corner case on PARISC, so just in case
I'd rather stick as close as possible to mainline : at least we want
to ensure the same bugs are met everywhere so that we can benefit
from developers' help when issues are met.

> Your patch is not wrong there: though odd to be trying
> anon_vma_prepare() twice in expand_downwards(),

Ah crap, the second one is a leftover from initial code that I missed.

> and tiresome to have to unlock at each error exit.

Oh I'm seeing that you could move it later, I wasn't sure about this
one. Thanks. I think I did the same stuff in the 3.16 backport.

> But I'd already decided in one of our
> internal trees just to factor in some of Konstantin's change, that
> made the ordering much more sensible there, and the two more like
> each other; so recommend that 3.10 do the same, keeping it closer
> to the final 4.12 code.  But you may have different priorities and
> disagree with that: just suggesting.

No, I perfectly agree with you. As I mentionned, my patches were
proposals based on what I understood from the code, I'm really glad
to receive your help and fixes here!

> And there is the possibility that we shall want another patch or
> two on top there.  I've left a question as to whether we should be
> comparing anon_vmas.  And there's a potential (but I think ignorable)
> locking issue, in the case of an architecture that supports both
> VM_GROWSUP and VM_GROWSDOWN: if they expand towards each other at the
> same instant, they could gobble up the gap between them (they almost
> certainly have different anon_vmas, so the anon_vma locking does not
> protect against that).  When it gets to updating the vma tree, it is
> careful to use page_table_lock to maintain the consistency of the
> tree in such a case, but maybe we should do that earlier.

OK.

> Then there's the FOLL_MLOCK thing, and the WARN_ON (phew, remembered
> in time that you don't have VM_WARN_ON) - but keep in mind that I
> have not even built this tree, let alone tested it.

I'll take care of building it, don't worry.

> Sorry if I'm being annoying, Willy: you must be heartily sick of
> these patches by now!  Or, being a longtime longterm maintainer,
> perhaps it's all joy for you ;-?

No, rest assured it's never a full joy :-)  But it's much better
when I get help from the people who know how this stuff works than
when I have to invent the backport by myself!

Thanks a lot, I'll include your patch and will test it again. And
yes, I intend to merge Helge's fix once it lands into mainline (maybe
it is right now, I didn't check) and possibly other ones you might be
working on depending on various feedback.

Willy

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

#1671771

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

Hey Hugh,

On Wed, Jun 21, 2017 at 09:18:23AM +0200, Willy Tarreau wrote:
> Thanks a lot, I'll include your patch and will test it again. And
> yes, I intend to merge Helge's fix once it lands into mainline (maybe
> it is right now, I didn't check) and possibly other ones you might be
> working on depending on various feedback.

So here's a quick update, I've built a kernel with my initial backport
fixed by applying your patch on top of it and have run various tests
on it. All I can say is that for me it works. I've instrumented a
little bit more my test program (which I'm attaching).

With 3.10.106+ unpatched, I get this :

admin@formilux:~$ ulimit -s unlimited
admin@formilux:~$ /tmp/gap 65536 < /dev/null 
mmap() failed
stack=0x550024b0 [0x550024b0-0x7fc6d0f0] (717663296 total bytes)
heap~=(nil) [(nil)-(nil)] (0 total bytes)
anon~=0x7ffef000 [0x2aaab000-0x7ffef000] (1431584768 total bytes)
heap...stack=2143736048 bytes
heap+anon+stack=2149248064 bytes
08048000-080d6000 r-xp 00000000 00:0d 3263       /var/tmp/gap
080d6000-080d8000 rw-p 0008d000 00:0d 3263       /var/tmp/gap
080d8000-080fb000 rw-p 00000000 00:00 0          [heap]
2aaab000-5537d000 rw-p 00000000 00:00 0          [stack:1813]
55382000-7fc7f000 rw-p 00000000 00:00 0 
7fc7f000-7ffff000 rw-p 00000000 00:00 0 
ffffe000-fffff000 r-xp 00000000 00:00 0          [vdso]
rounds: 10949

The output is not obvious, it dumps its last known pointer for each
VMA and the upper and lower known as well, then dumps the contents
of /proc/self/maps either after the segfault or after a failed
alloc. Here you can see that the stack was seen equal to 0x550024b0,
which totally belongs to the anon area, which is reported as stack
in /proc/self/maps probably due to the pointers crossing each other.
The stack really was the next VMA (55382000-7fc7f000). So we had a
significant collision here, with about ~56 stack accesses being made
in the anon VMA.

With 3.10.107-rc and your latest fix I get this :

admin@formilux:~$ ulimit -s unlimited
admin@formilux:~$ /tmp/gap 65536 </dev/null  
SEGV caught
stack=0x552cb240 [0x552db250-0x7fa15960] (712222480 total bytes)
heap~=(nil) [(nil)-(nil)] (0 total bytes)
anon~=0x7fa27000 [0x2aaab000-0x7fa27000] (1425522688 total bytes)
heap...stack=2141280608 bytes
heap+anon+stack=2137745168 bytes
08048000-080d6000 r-xp 00000000 00:0d 3263       /var/tmp/gap
080d6000-080d8000 rw-p 0008d000 00:0d 3263       /var/tmp/gap
080d8000-080fb000 rw-p 00000000 00:00 0          [heap]
2aaab000-551cd000 rw-p 00000000 00:00 0 
552db000-7fa27000 rw-p 00000000 00:00 0 
7fa27000-7fa37000 rw-p 00000000 00:00 0 
ffffe000-fffff000 r-xp 00000000 00:00 0          [vdso]

There's no collision, the stack access stopped on the guard page.

The program is ugly but usable. If you pass it a negative size it will
first fill the heap for this absolute size, then switch to anon. It's
useless now but who knows.

So for me it's OK now. I'm attaching the test program. Greg, Ben, Sasha,
you need a 2 GB i386 machine to reliably test it (booting in a VM is OK).
I can provide you with a small system image offline if needed.

Cheers,
Willy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web