* [PATCH] guard page for stacks that grow upwards
@ 2010-08-24 16:31 ` Luck, Tony
0 siblings, 0 replies; 11+ messages in thread
From: Luck, Tony @ 2010-08-24 16:31 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-parisc, linux-ia64
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;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] guard page for stacks that grow upwards
@ 2010-08-24 16:31 ` Luck, Tony
0 siblings, 0 replies; 11+ messages in thread
From: Luck, Tony @ 2010-08-24 16:31 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-parisc, linux-ia64
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;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] guard page for stacks that grow upwards
2010-08-24 16:31 ` Luck, Tony
@ 2010-08-24 16:53 ` Linus Torvalds
-1 siblings, 0 replies; 11+ messages in thread
From: Linus Torvalds @ 2010-08-24 16:53 UTC (permalink / raw)
To: Luck, Tony; +Cc: linux-kernel, linux-parisc, linux-ia64
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] guard page for stacks that grow upwards
@ 2010-08-24 16:53 ` Linus Torvalds
0 siblings, 0 replies; 11+ messages in thread
From: Linus Torvalds @ 2010-08-24 16:53 UTC (permalink / raw)
To: Luck, Tony; +Cc: linux-kernel, linux-parisc, linux-ia64
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] guard page for stacks that grow upwards
2010-08-24 16:53 ` Linus Torvalds
@ 2010-08-24 17:33 ` Luck, Tony
-1 siblings, 0 replies; 11+ messages in thread
From: Luck, Tony @ 2010-08-24 17:33 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-parisc, linux-ia64
[-- 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;
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] guard page for stacks that grow upwards
@ 2010-08-24 17:33 ` Luck, Tony
0 siblings, 0 replies; 11+ messages in thread
From: Luck, Tony @ 2010-08-24 17:33 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-parisc, linux-ia64
[-- 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;
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] guard page for stacks that grow upwards
2010-08-24 16:31 ` Luck, Tony
(?)
@ 2010-08-24 17:51 ` Linus Torvalds
-1 siblings, 0 replies; 11+ messages in thread
From: Linus Torvalds @ 2010-08-24 17:51 UTC (permalink / raw)
To: Luck, Tony; +Cc: linux-kernel, linux-parisc, linux-ia64
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)
> + =A0 =A0 =A0 else if (vma->vm_flags & VM_GROWSUP) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 address =3D PAGE_ALIGN(address + 1);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (address =3D=3D 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 =3D PAGE_ALIGN(address + 1);" thing is just ugly.
Wouldn't it be nicer to just move the earlier
address &=3D 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 =3D=3D vma->vm_end)
rather than have "PAGE_ALIGN(address + 1)" as yet another way of
aligning the address just right.
No?
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] guard page for stacks that grow upwards
@ 2010-08-24 17:51 ` Linus Torvalds
0 siblings, 0 replies; 11+ messages in thread
From: Linus Torvalds @ 2010-08-24 17:51 UTC (permalink / raw)
To: Luck, Tony; +Cc: linux-kernel, linux-parisc, linux-ia64
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] guard page for stacks that grow upwards
@ 2010-08-24 17:51 ` Linus Torvalds
0 siblings, 0 replies; 11+ messages in thread
From: Linus Torvalds @ 2010-08-24 17:51 UTC (permalink / raw)
To: Luck, Tony; +Cc: linux-kernel, linux-parisc, linux-ia64
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] guard page for stacks that grow upwards
2010-08-24 17:51 ` Linus Torvalds
@ 2010-08-24 18:04 ` Luck, Tony
-1 siblings, 0 replies; 11+ messages in thread
From: Luck, Tony @ 2010-08-24 18:04 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-parisc, linux-ia64
> - 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] guard page for stacks that grow upwards
@ 2010-08-24 18:04 ` Luck, Tony
0 siblings, 0 replies; 11+ messages in thread
From: Luck, Tony @ 2010-08-24 18:04 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-parisc, linux-ia64
> - 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-08-24 18:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-24 16:31 [PATCH] guard page for stacks that grow upwards Luck, Tony
2010-08-24 16:31 ` Luck, Tony
2010-08-24 16:53 ` Linus Torvalds
2010-08-24 16:53 ` Linus Torvalds
2010-08-24 17:33 ` Luck, Tony
2010-08-24 17:33 ` Luck, Tony
2010-08-24 17:51 ` Linus Torvalds
2010-08-24 17:51 ` Linus Torvalds
2010-08-24 17:51 ` Linus Torvalds
2010-08-24 18:04 ` Luck, Tony
2010-08-24 18:04 ` Luck, Tony
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.