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

Previous thread: [PATCH] s390: Fix prototype for execve by Jeff Mahoney on Tuesday, August 24, 2010 - 9:25 am. (3 messages)

Next thread: Notificación Importante by malin ljung on Tuesday, August 24, 2010 - 9:23 am. (1 message)
From: Luck, Tony
Date: Tuesday, August 24, 2010 - 9:31 am

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 ...
From: Linus Torvalds
Date: Tuesday, August 24, 2010 - 9:53 am

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
--

From: Luck, Tony
Date: Tuesday, August 24, 2010 - 10:33 am

> 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

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

-Tony
From: Linus Torvalds
Date: Tuesday, August 24, 2010 - 10:51 am

Hmm. Looking at the patch a bit more..


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
--

From: Luck, Tony
Date: Tuesday, August 24, 2010 - 11:04 am

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

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

Yes, that works too.

Coding revised version now.

-Tony
--

Previous thread: [PATCH] s390: Fix prototype for execve by Jeff Mahoney on Tuesday, August 24, 2010 - 9:25 am. (3 messages)

Next thread: Notificación Importante by malin ljung on Tuesday, August 24, 2010 - 9:23 am. (1 message)