guard page for stacks that grow upwards
Subsystems (3)
PATCH 1/1 guard page for stacks that grow upwards
pa-risc and ia64 have stacks that grow upwards. Check that they do not run into other mappings.
---
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.
include/linux/mm.h +1 -1
@@ -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,
mm/memory.c +30 -16
59 lines not on this page · raw message
mm/mmap.c +0 -3
@@ -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;
Quoted 2 lines · Tony
Updated to match the new code - still not tested on pa-risc.
But the ia64 grows-up case is tested?
Quoted 2 lines
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
Quoted 1 line
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).
Quoted 8 lines
Quoted 2 lines
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_GROWSUPcrap entirely in code, and instead just make theVM_GROWSUP#define be 0 for architectures that don't want it. The compiler should then just automatically remove all the code that says
Quoted 4 lines
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
#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;
}Hmm. Looking at the patch a bit more..
Quoted 5 lines · Tony
+#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
Quoted 6 lines
- 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".
Quoted 4 lines
- The "address =
PAGE_ALIGN(address + 1);" thing is just ugly. Wouldn't it be nicer to just move the earlieraddress &=
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.
Quoted 8 lines
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