[PATCH] guard page for stacks that grow upwards

[PATCH] guard page for stacks that grow upwards

[Luck, Tony]

pa-risc and ia64 have stacks that grow upwards. Check that
they do not run into other mappings.

Signed-off-by: Tony Luck <tony.luck@intel.com>

---

Updated to match the new code - still not tested on pa-risc.
The #ifdefs are ugly - suggestions welcome on how to make
the code prettier.

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 709f672..089d135 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1330,7 +1330,7 @@ unsigned long ra_submit(struct file_ra_state *ra,
 
 /* Do stack extension */
 extern int expand_stack(struct vm_area_struct *vma, unsigned long address);
-#ifdef CONFIG_IA64
+#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
 extern int expand_upwards(struct vm_area_struct *vma, unsigned long address);
 #endif
 extern int expand_stack_downwards(struct vm_area_struct *vma,
diff --git a/mm/memory.c b/mm/memory.c
index 2ed2267..5127b1c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2760,29 +2760,43 @@ out_release:
 }
 
 /*
- * This is like a special single-page "expand_downwards()",
- * except we must first make sure that 'address-PAGE_SIZE'
+ * This is like a special single-page "expand_{down|up}wards()",
+ * except we must first make sure that 'address{-|+}PAGE_SIZE'
  * doesn't hit another vma.
- *
- * The "find_vma()" will do the right thing even if we wrap
  */
 static inline int check_stack_guard_page(struct vm_area_struct *vma, unsigned long address)
 {
-	address &= PAGE_MASK;
-	if ((vma->vm_flags & VM_GROWSDOWN) && address = vma->vm_start) {
-		struct vm_area_struct *prev = vma->vm_prev;
+	if (vma->vm_flags & VM_GROWSDOWN) {
+		address &= PAGE_MASK;
+		if (address = vma->vm_start) {
+			struct vm_area_struct *prev = vma->vm_prev;
 
-		/*
-		 * Is there a mapping abutting this one below?
-		 *
-		 * That's only ok if it's the same stack mapping
-		 * that has gotten split..
-		 */
-		if (prev && prev->vm_end = address)
-			return prev->vm_flags & VM_GROWSDOWN ? 0 : -ENOMEM;
+			/*
+			 * Is there a mapping abutting this one below?
+			 *
+			 * That's only ok if it's the same stack mapping
+			 * that has gotten split..
+			 */
+			if (prev && prev->vm_end = address)
+				return prev->vm_flags & VM_GROWSDOWN ? 0 : -ENOMEM;
 
-		expand_stack(vma, address - PAGE_SIZE);
+			expand_stack(vma, address - PAGE_SIZE);
+		}
+	}
+#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
+	else if (vma->vm_flags & VM_GROWSUP) {
+		address = PAGE_ALIGN(address + 1);
+		if (address = vma->vm_end) {
+			struct vm_area_struct *next = vma->vm_next;
+
+			/* As VM_GROWSDOWN but s/below/above/ */
+			if (next && next->vm_start = address)
+				return next->vm_flags & VM_GROWSUP ? 0 : -ENOMEM;
+
+			expand_upwards(vma, address);
+		}
 	}
+#endif
 	return 0;
 }
 
diff --git a/mm/mmap.c b/mm/mmap.c
index 331e51a..6128dc8 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1716,9 +1716,6 @@ static int acct_stack_growth(struct vm_area_struct *vma, unsigned long size, uns
  * PA-RISC uses this for its stack; IA64 for its Register Backing Store.
  * vma is the last one with address > vma->vm_end.  Have to extend vma.
  */
-#ifndef CONFIG_IA64
-static
-#endif
 int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 {
 	int error;

Re: [PATCH] guard page for stacks that grow upwards

[Linus Torvalds]

On Tue, Aug 24, 2010 at 9:31 AM, Luck, Tony <tony.luck@intel.com> wrote:
>
> Updated to match the new code - still not tested on pa-risc.

But the ia64 grows-up case is tested?

> The #ifdefs are ugly - suggestions welcome on how to make
> the code prettier.

One thing I've considered is to get rid of the CONFIG_STACK_GROWSUP
crap entirely in code, and instead just make the VM_GROWSUP #define be
0 for architectures that don't want it. The compiler should then just
automatically remove all the code that says

   if (vma->vm_flags & VM_GROWSUP) {
     ...

and the code would look more straightforward. Hmm?

                    Linus

RE: [PATCH] guard page for stacks that grow upwards

[Luck, Tony]

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

> But the ia64 grows-up case is tested?

Yes. The attached hacky test program reports that the RSE stack
stomps over the mmap'd segment w/o this patch. With it the
program dies with a SIGBUS. Should be easy to adapt to
test on pa-risc (hint, hint to parisc people).

>> The #ifdefs are ugly - suggestions welcome on how to make
>> the code prettier.
>
> One thing I've considered is to get rid of the CONFIG_STACK_GROWSUP
> crap entirely in code, and instead just make the VM_GROWSUP #define be
> 0 for architectures that don't want it. The compiler should then just
> automatically remove all the code that says
>
>   if (vma->vm_flags & VM_GROWSUP) {
>     ...
>
> and the code would look more straightforward. Hmm?

You'd also need some stub declaration for expand_upwards().
But overall that would look cleaner.

-Tony

[-- Attachment #2: growtest.c --]
[-- Type: text/plain, Size: 771 bytes --]

#include <stdio.h>
#include <sys/types.h>
#include <sys/mman.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>

void watch(long depth, char *map)
{
	int i;

	for (i = 0; i < 0x10000; i++) {
		if (map[i]) {
			printf("Found %x at %p\n", map[i], &map[i]);
			exit(1);
		}
	}
	if (++depth % 5000 == 0)
		printf("now at stack depth %ld\n", depth);
	watch(depth, map);

	/* won't get here .. but stop compiler from doing tail recursion */
	for (i = 0; i < 0x10000; i++) {
		if (map[i]) {
			printf("Found %x at %p\n", map[i], &map[i]);
			exit(1);
		}
	}
}
	
main()
{
	char *p;

	p = mmap((void *)0x6008000000000000, 0x10000, PROT_READ|PROT_WRITE,
		MAP_ANONYMOUS|MAP_PRIVATE, -1, 0L);

	printf("%p\n", p);
	memset(p, '\0', 0x10000);

	watch(0, p);

	return 0;
}

Re: [PATCH] guard page for stacks that grow upwards

[Linus Torvalds]

Hmm. Looking at the patch a bit more..

On Tue, Aug 24, 2010 at 9:31 AM, Luck, Tony <tony.luck@intel.com> wrote:
>
> +#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
> +       else if (vma->vm_flags & VM_GROWSUP) {
> +               address = PAGE_ALIGN(address + 1);
> +               if (address = vma->vm_end) {

So I react to two things:

 - that "else" looks wrong. At least conceptually, both could happen -
the code should be entirely able to handle a segment that expands both
ways (which is something that ia64 could do: stack one way, register
spills etc other, all in just one happy vma). I guess we don't set it
up that way now, but the "else" just annoys my sense of aesthetics.
It's an extra four letters that don't add value - just takes it away.

 - The "address = PAGE_ALIGN(address + 1);" thing is just ugly.
Wouldn't it be nicer to just move the earlier

  address &= PAGE_MASK

back outside the conditionals (and keep the original conditional the
way it was - you only changed it for that bogus "else" case anyway),
and then do

  if (address + PAGE_SIZE = vma->vm_end)

rather than have "PAGE_ALIGN(address + 1)" as yet another way of
aligning the address just right.

No?

                              Linus

RE: [PATCH] guard page for stacks that grow upwards

[Luck, Tony]

>  - that "else" looks wrong. At least conceptually, both could happen -
> the code should be entirely able to handle a segment that expands both
> ways (which is something that ia64 could do: stack one way, register
> spills etc other, all in just one happy vma). I guess we don't set it
> up that way now, but the "else" just annoys my sense of aesthetics.
> It's an extra four letters that don't add value - just takes it away.

Conceptually yes we could - I don't suppose we ever will as keeping
separate vmas for stack and RSE stack defends against underflow in
a program that has totally messed up.  But I can save 4 bytes of
source code and drop the "else".

> - The "address = PAGE_ALIGN(address + 1);" thing is just ugly.
> Wouldn't it be nicer to just move the earlier
>
>  address &= PAGE_MASK

This mask might be redundant ... I didn't look at the call
sequence, but I've observed that "address" happens to be page
aligned in all my tests.

> back outside the conditionals (and keep the original conditional the
> way it was - you only changed it for that bogus "else" case anyway),
> and then do
>
>  if (address + PAGE_SIZE = vma->vm_end)
>
> rather than have "PAGE_ALIGN(address + 1)" as yet another way of
> aligning the address just right.

Yes, that works too.

Coding revised version now.

-Tony