Re: [PATCH] Restrict initial stack space expansion to rlimit
On 02/10/2010 06:31 AM, Michael Neuling wrote: In message20100210141016.4d18.a69d9...@jp.fujitsu.com you wrote: On 02/09/2010 10:51 PM, Michael Neuling wrote: I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. There's only one CONFIG_GROWSUP arch - parisc. Could someone please test it on parisc? I did. How about doing: 'ulimit -s 15; ls' before and after the patch is applied. Before it's applied, 'ls' should be killed. After the patch is applied, 'ls' should no longer be killed. I'm suggesting a stack limit of 15KB since it's small enough to trigger 20*PAGE_SIZE. Also 15KB not a multiple of PAGE_SIZE, which is a trickier case to handle correctly with this code. 4K pages on parisc should be fine to test with. Mikey, thanks for the suggested test plan. I'm not sure if your patch does it correct for parisc/stack-grows-up-case. I tested your patch on a 4k pages kernel: r...@c3000:~# uname -a Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/Li nux Without your patch: r...@c3000:~# ulimit -s 15; ls Killed - correct. With your patch: r...@c3000:~# ulimit -s 15; ls Killed _or_: r...@c3000:~# ulimit -s 15; ls Segmentation fault - ?? Any idea? My x86_64 box also makes segmentation fault. I think ulimit -s 15 is too sm all stack for ls. ulimit -s 27; ls wroks perfectly fine. Arrh. I asked Helge offline earlier to check what use to work on parisc on 2.6.31. I guess PPC has a nice clean non-bloated ABI :-D Hi Mikey, I tested again, and it works for me with ulimit -s 27 as well (on a 4k, 32bit kernel). Still, I'm not 100% sure if your patch is correct. Anyway, it seems to work. But what makes me wonder is, why EXTRA_STACK_VM_PAGES is defined in pages at all. You wrote in your patch description: This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. Wouldn't it make sense to define and use EXTRA_STACK_VM_SIZE instead (e.g. as 20*4096 = 80k)? This extra stack reservation should IMHO be independend of the actual kernel page size. Helge ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Restrict initial stack space expansion to rlimit
In message 4b7481a6.7080...@gmx.de you wrote: On 02/10/2010 06:31 AM, Michael Neuling wrote: In message20100210141016.4d18.a69d9...@jp.fujitsu.com you wrote: On 02/09/2010 10:51 PM, Michael Neuling wrote: I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. There's only one CONFIG_GROWSUP arch - parisc. Could someone please test it on parisc? I did. How about doing: 'ulimit -s 15; ls' before and after the patch is applied. Before it's applied, 'ls' should be killed. After the patch is applied, 'ls' should no longer be killed. I'm suggesting a stack limit of 15KB since it's small enough to trigger 20*PAGE_SIZE. Also 15KB not a multiple of PAGE_SIZE, which is a trickie r case to handle correctly with this code. 4K pages on parisc should be fine to test with. Mikey, thanks for the suggested test plan. I'm not sure if your patch does it correct for parisc/stack-grows-up-case . I tested your patch on a 4k pages kernel: r...@c3000:~# uname -a Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/ Li nux Without your patch: r...@c3000:~# ulimit -s 15; ls Killed - correct. With your patch: r...@c3000:~# ulimit -s 15; ls Killed _or_: r...@c3000:~# ulimit -s 15; ls Segmentation fault - ?? Any idea? My x86_64 box also makes segmentation fault. I think ulimit -s 15 is too sm all stack for ls. ulimit -s 27; ls wroks perfectly fine. Arrh. I asked Helge offline earlier to check what use to work on parisc on 2.6.31. I guess PPC has a nice clean non-bloated ABI :-D Hi Mikey, I tested again, and it works for me with ulimit -s 27 as well (on a 4k, 32bit kernel). Still, I'm not 100% sure if your patch is correct. Thanks for retesting Did ulimit -s 27 fail before you applied? Anyway, it seems to work. But what makes me wonder is, why EXTRA_STACK_VM_PAGES is defined in pages at all. You wrote in your patch description: This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. Wouldn't it make sense to define and use EXTRA_STACK_VM_SIZE instead (e.g. as 20*4096 = 80k)? This extra stack reservation should IMHO be independend of the actual kernel page size. If you look back through this thread, that has already been noted but it's a separate issue to this bug, so that change will be deferred till 2.6.34. Mikey ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Restrict initial stack space expansion to rlimit
In message 20100209154141.03f0.a69d9...@jp.fujitsu.com you wrote: When reserving stack space for a new process, make sure we're not attempting to expand the stack by more than rlimit allows. This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba mm: variable length argument support and unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b exec: setup_arg_pages() fails to return errors. This bug means when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. Signed-off-by: Michael Neuling mi...@neuling.org Cc: sta...@kernel.org --- Attempts to answer comments from Kosaki Motohiro. Tested on PPC only, hence !CONFIG_STACK_GROWSUP. Someone should probably ACK for an arch with CONFIG_STACK_GROWSUP. As noted, stable needs the same patch, but 2.6.32 doesn't have the rlimit() helper. fs/exec.c | 21 ++--- 1 file changed, 18 insertions(+), 3 deletions(-) Index: linux-2.6-ozlabs/fs/exec.c === --- linux-2.6-ozlabs.orig/fs/exec.c +++ linux-2.6-ozlabs/fs/exec.c @@ -555,6 +555,7 @@ static int shift_arg_pages(struct vm_are } #define EXTRA_STACK_VM_PAGES 20 /* random */ +#define ALIGN_DOWN(addr,size) ((addr)(~((size)-1))) /* * Finalizes the stack vm_area_struct. The flags and permissions are updat ed, @@ -570,7 +571,7 @@ int setup_arg_pages(struct linux_binprm struct vm_area_struct *vma = bprm-vma; struct vm_area_struct *prev = NULL; unsigned long vm_flags; - unsigned long stack_base; + unsigned long stack_base, stack_expand, stack_expand_lim, stack_size; #ifdef CONFIG_STACK_GROWSUP /* Limit stack size to 1GB */ @@ -627,10 +628,24 @@ int setup_arg_pages(struct linux_binprm goto out_unlock; } + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_size = vma-vm_end - vma-vm_start; + if (rlimit(RLIMIT_STACK) stack_size) + stack_expand_lim = 0; /* don't shrick the stack */ + else + /* +* Align this down to a page boundary as expand_stack +* will align it up. +*/ + stack_expand_lim = ALIGN_DOWN(rlimit(RLIMIT_STACK) - stack_size , + PAGE_SIZE); + /* Initial stack must not cause stack overflow. */ + if (stack_expand stack_expand_lim) + stack_expand = stack_expand_lim; #ifdef CONFIG_STACK_GROWSUP - stack_base = vma-vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_base = vma-vm_end + stack_expand; #else - stack_base = vma-vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_base = vma-vm_start - stack_expand; #endif ret = expand_stack(vma, stack_base); if (ret) Umm.. It looks correct. but the nested complex if statement seems a bit ugly. Instead, How about following? I don't like the duplicated code in the #ifdef/else but I can live with it. note: it's untested. Works for me on ppc64 with 4k and 64k pages. Thanks! I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. Mikey === From: Michael Neuling mi...@neuling.org Subject: Restrict initial stack space expansion to rlimit When reserving stack space for a new process, make sure we're not attempting to expand the stack by more than rlimit allows. This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba mm: variable length argument support and unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b exec: setup_arg_pages() fails to return errors. This bug means when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. [kosaki.motoh...@jp.fujitsu.com: cleanups] Signed-off-by: Michael Neuling mi...@neuling.org Signed-off-by: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com Cc: sta...@kernel.org --- Attempts to answer comments from Kosaki Motohiro. Tested on PPC only, hence !CONFIG_STACK_GROWSUP. Someone should probably ACK for an arch with CONFIG_STACK_GROWSUP. As noted, stable needs the same patch, but 2.6.32 doesn't have the rlimit() helper. diff --git a/fs/exec.c b/fs/exec.c index 6f7fb0c..325bad4 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -573,6 +573,9 @@ int setup_arg_pages(struct linux_binprm *bprm, struct vm_area_struct *prev = NULL; unsigned long vm_flags; unsigned long stack_base; + unsigned long stack_size; + unsigned long stack_expand; + unsigned long rlim_stack; #ifdef CONFIG_STACK_GROWSUP /* Limit stack size to 1GB
Re: [PATCH] Restrict initial stack space expansion to rlimit
On Tue, 09 Feb 2010 19:59:27 +1100
Michael Neuling mi...@neuling.org wrote:
+ /* Initial stack must not cause stack overflow. */
+ if (stack_expand stack_expand_lim)
+ stack_expand = stack_expand_lim;
#ifdef CONFIG_STACK_GROWSUP
- stack_base = vma-vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
+ stack_base = vma-vm_end + stack_expand;
#else
- stack_base = vma-vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
+ stack_base = vma-vm_start - stack_expand;
#endif
ret = expand_stack(vma, stack_base);
if (ret)
Umm.. It looks correct. but the nested complex if statement seems a bit
ugly.
Instead, How about following?
I don't like the duplicated code in the #ifdef/else but I can live with it.
cleanup the cleanup:
---
a/fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit-cleanup-cleanup
+++ a/fs/exec.c
@@ -637,20 +637,17 @@ int setup_arg_pages(struct linux_binprm
* will align it up.
*/
rlim_stack = rlimit(RLIMIT_STACK) PAGE_MASK;
- if (rlim_stack stack_size)
- rlim_stack = stack_size;
+ rlim_stack = min(rlim_stack, stack_size);
#ifdef CONFIG_STACK_GROWSUP
- if (stack_size + stack_expand rlim_stack) {
+ if (stack_size + stack_expand rlim_stack)
stack_base = vma-vm_start + rlim_stack;
- } else {
+ else
stack_base = vma-vm_end + stack_expand;
- }
#else
- if (stack_size + stack_expand rlim_stack) {
+ if (stack_size + stack_expand rlim_stack)
stack_base = vma-vm_end - rlim_stack;
- } else {
+ else
stack_base = vma-vm_start - stack_expand;
- }
#endif
ret = expand_stack(vma, stack_base);
if (ret)
_
note: it's untested.
Works for me on ppc64 with 4k and 64k pages. Thanks!
I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it
as well.
There's only one CONFIG_GROWSUP arch - parisc.
Guys, here's the rolled-up patch. Could someone please test it on
parisc?
err, I'm not sure what one needs to do to test it, actually.
Presumably it involves setting an unusual `ulimit -s'. Can someone
please suggest a test plan?
From: Michael Neuling mi...@neuling.org
When reserving stack space for a new process, make sure we're not
attempting to expand the stack by more than rlimit allows.
This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba (mm:
variable length argument support) and unmasked by
fc63cf237078c86214abcb2ee9926d8ad289da9b (exec: setup_arg_pages() fails
to return errors).
This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg.
80K on 4K pages or 'ulimit -s 79') all processes will be killed before
they start. This is particularly bad with 64K pages, where a ulimit below
1280K will kill every process.
Signed-off-by: Michael Neuling mi...@neuling.org
Cc: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com
Cc: Americo Wang xiyou.wangc...@gmail.com
Cc: Anton Blanchard an...@samba.org
Cc: Oleg Nesterov o...@redhat.com
Cc: James Morris jmor...@namei.org
Cc: Ingo Molnar mi...@elte.hu
Cc: Serge Hallyn se...@us.ibm.com
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: sta...@kernel.org
fs/exec.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff -puN fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit
fs/exec.c
--- a/fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit
+++ a/fs/exec.c
@@ -571,6 +571,9 @@ int setup_arg_pages(struct linux_binprm
struct vm_area_struct *prev = NULL;
unsigned long vm_flags;
unsigned long stack_base;
+ unsigned long stack_size;
+ unsigned long stack_expand;
+ unsigned long rlim_stack;
#ifdef CONFIG_STACK_GROWSUP
/* Limit stack size to 1GB */
@@ -627,10 +630,24 @@ int setup_arg_pages(struct linux_binprm
goto out_unlock;
}
+ stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
+ stack_size = vma-vm_end - vma-vm_start;
+ /*
+* Align this down to a page boundary as expand_stack
+* will align it up.
+*/
+ rlim_stack = rlimit(RLIMIT_STACK) PAGE_MASK;
+ rlim_stack = min(rlim_stack, stack_size);
#ifdef CONFIG_STACK_GROWSUP
- stack_base = vma-vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
+ if (stack_size + stack_expand rlim_stack)
+ stack_base = vma-vm_start + rlim_stack;
+ else
+ stack_base = vma-vm_end + stack_expand;
#else
- stack_base = vma-vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
+ if (stack_size + stack_expand rlim_stack)
+ stack_base = vma-vm_end - rlim_stack;
+ else
+ stack_base = vma-vm_start - stack_expand;
#endif
ret = expand_stack(vma, stack_base);
if (ret)
_
___
Linuxppc-dev mailing list
Re: [PATCH] Restrict initial stack space expansion to rlimit
note: it's untested. Works for me on ppc64 with 4k and 64k pages. Thanks! I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. There's only one CONFIG_GROWSUP arch - parisc. Guys, here's the rolled-up patch. FYI the rolled up patch still works fine on PPC64. Thanks. Could someone please test it on parisc? err, I'm not sure what one needs to do to test it, actually. Presumably it involves setting an unusual `ulimit -s'. Can someone please suggest a test plan? How about doing: 'ulimit -s 15; ls' before and after the patch is applied. Before it's applied, 'ls' should be killed. After the patch is applied, 'ls' should no longer be killed. I'm suggesting a stack limit of 15KB since it's small enough to trigger 20*PAGE_SIZE. Also 15KB not a multiple of PAGE_SIZE, which is a trickier case to handle correctly with this code. 4K pages on parisc should be fine to test with. Mikey From: Michael Neuling mi...@neuling.org When reserving stack space for a new process, make sure we're not attempting to expand the stack by more than rlimit allows. This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba (mm: variable length argument support) and unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b (exec: setup_arg_pages() fails to return errors). This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. Signed-off-by: Michael Neuling mi...@neuling.org Cc: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com Cc: Americo Wang xiyou.wangc...@gmail.com Cc: Anton Blanchard an...@samba.org Cc: Oleg Nesterov o...@redhat.com Cc: James Morris jmor...@namei.org Cc: Ingo Molnar mi...@elte.hu Cc: Serge Hallyn se...@us.ibm.com Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: sta...@kernel.org fs/exec.c | 21 +++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff -puN fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit fs/exec.c --- a/fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit +++ a/fs/exec.c @@ -571,6 +571,9 @@ int setup_arg_pages(struct linux_binprm struct vm_area_struct *prev = NULL; unsigned long vm_flags; unsigned long stack_base; + unsigned long stack_size; + unsigned long stack_expand; + unsigned long rlim_stack; #ifdef CONFIG_STACK_GROWSUP /* Limit stack size to 1GB */ @@ -627,10 +630,24 @@ int setup_arg_pages(struct linux_binprm goto out_unlock; } + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_size = vma-vm_end - vma-vm_start; + /* + * Align this down to a page boundary as expand_stack + * will align it up. + */ + rlim_stack = rlimit(RLIMIT_STACK) PAGE_MASK; + rlim_stack = min(rlim_stack, stack_size); #ifdef CONFIG_STACK_GROWSUP - stack_base = vma-vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE; + if (stack_size + stack_expand rlim_stack) + stack_base = vma-vm_start + rlim_stack; + else + stack_base = vma-vm_end + stack_expand; #else - stack_base = vma-vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE; + if (stack_size + stack_expand rlim_stack) + stack_base = vma-vm_end - rlim_stack; + else + stack_base = vma-vm_start - stack_expand; #endif ret = expand_stack(vma, stack_base); if (ret) _ ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Restrict initial stack space expansion to rlimit
On 02/09/2010 10:51 PM, Michael Neuling wrote: I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. There's only one CONFIG_GROWSUP arch - parisc. Could someone please test it on parisc? I did. How about doing: 'ulimit -s 15; ls' before and after the patch is applied. Before it's applied, 'ls' should be killed. After the patch is applied, 'ls' should no longer be killed. I'm suggesting a stack limit of 15KB since it's small enough to trigger 20*PAGE_SIZE. Also 15KB not a multiple of PAGE_SIZE, which is a trickier case to handle correctly with this code. 4K pages on parisc should be fine to test with. Mikey, thanks for the suggested test plan. I'm not sure if your patch does it correct for parisc/stack-grows-up-case. I tested your patch on a 4k pages kernel: r...@c3000:~# uname -a Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/Linux Without your patch: r...@c3000:~# ulimit -s 15; ls Killed - correct. With your patch: r...@c3000:~# ulimit -s 15; ls Killed _or_: r...@c3000:~# ulimit -s 15; ls Segmentation fault - ?? Any idea? Helge From: Michael Neulingmi...@neuling.org When reserving stack space for a new process, make sure we're not attempting to expand the stack by more than rlimit allows. This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba (mm: variable length argument support) and unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b (exec: setup_arg_pages() fails to return errors). This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. Signed-off-by: Michael Neulingmi...@neuling.org Cc: KOSAKI Motohirokosaki.motoh...@jp.fujitsu.com Cc: Americo Wangxiyou.wangc...@gmail.com Cc: Anton Blanchardan...@samba.org Cc: Oleg Nesterovo...@redhat.com Cc: James Morrisjmor...@namei.org Cc: Ingo Molnarmi...@elte.hu Cc: Serge Hallynse...@us.ibm.com Cc: Benjamin Herrenschmidtb...@kernel.crashing.org Cc:sta...@kernel.org fs/exec.c | 21 +++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff -puN fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit fs/exec.c --- a/fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit +++ a/fs/exec.c @@ -571,6 +571,9 @@ int setup_arg_pages(struct linux_binprm struct vm_area_struct *prev = NULL; unsigned long vm_flags; unsigned long stack_base; + unsigned long stack_size; + unsigned long stack_expand; + unsigned long rlim_stack; #ifdef CONFIG_STACK_GROWSUP /* Limit stack size to 1GB */ @@ -627,10 +630,24 @@ int setup_arg_pages(struct linux_binprm goto out_unlock; } + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_size = vma-vm_end - vma-vm_start; + /* +* Align this down to a page boundary as expand_stack +* will align it up. +*/ + rlim_stack = rlimit(RLIMIT_STACK) PAGE_MASK; + rlim_stack = min(rlim_stack, stack_size); #ifdef CONFIG_STACK_GROWSUP - stack_base = vma-vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE; + if (stack_size + stack_expand rlim_stack) + stack_base = vma-vm_start + rlim_stack; + else + stack_base = vma-vm_end + stack_expand; #else - stack_base = vma-vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE; + if (stack_size + stack_expand rlim_stack) + stack_base = vma-vm_end - rlim_stack; + else + stack_base = vma-vm_start - stack_expand; #endif ret = expand_stack(vma, stack_base); if (ret) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Restrict initial stack space expansion to rlimit
On 02/09/2010 10:51 PM, Michael Neuling wrote: I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. There's only one CONFIG_GROWSUP arch - parisc. Could someone please test it on parisc? I did. How about doing: 'ulimit -s 15; ls' before and after the patch is applied. Before it's applied, 'ls' should be killed. After the patch is applied, 'ls' should no longer be killed. I'm suggesting a stack limit of 15KB since it's small enough to trigger 20*PAGE_SIZE. Also 15KB not a multiple of PAGE_SIZE, which is a trickier case to handle correctly with this code. 4K pages on parisc should be fine to test with. Mikey, thanks for the suggested test plan. I'm not sure if your patch does it correct for parisc/stack-grows-up-case. I tested your patch on a 4k pages kernel: r...@c3000:~# uname -a Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/Linux Without your patch: r...@c3000:~# ulimit -s 15; ls Killed - correct. With your patch: r...@c3000:~# ulimit -s 15; ls Killed _or_: r...@c3000:~# ulimit -s 15; ls Segmentation fault - ?? Any idea? My x86_64 box also makes segmentation fault. I think ulimit -s 15 is too small stack for ls. ulimit -s 27; ls wroks perfectly fine. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Restrict initial stack space expansion to rlimit
In message 20100210141016.4d18.a69d9...@jp.fujitsu.com you wrote: On 02/09/2010 10:51 PM, Michael Neuling wrote: I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. There's only one CONFIG_GROWSUP arch - parisc. Could someone please test it on parisc? I did. How about doing: 'ulimit -s 15; ls' before and after the patch is applied. Before it's applied, 'ls' should be killed. After the patch is applied, 'ls' should no longer be killed. I'm suggesting a stack limit of 15KB since it's small enough to trigger 20*PAGE_SIZE. Also 15KB not a multiple of PAGE_SIZE, which is a trickier case to handle correctly with this code. 4K pages on parisc should be fine to test with. Mikey, thanks for the suggested test plan. I'm not sure if your patch does it correct for parisc/stack-grows-up-case. I tested your patch on a 4k pages kernel: r...@c3000:~# uname -a Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/Li nux Without your patch: r...@c3000:~# ulimit -s 15; ls Killed - correct. With your patch: r...@c3000:~# ulimit -s 15; ls Killed _or_: r...@c3000:~# ulimit -s 15; ls Segmentation fault - ?? Any idea? My x86_64 box also makes segmentation fault. I think ulimit -s 15 is too sm all stack for ls. ulimit -s 27; ls wroks perfectly fine. Arrh. I asked Helge offline earlier to check what use to work on parisc on 2.6.31. I guess PPC has a nice clean non-bloated ABI :-D Mikey ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Restrict initial stack space expansion to rlimit
In message 20100210141016.4d18.a69d9...@jp.fujitsu.com you wrote: On 02/09/2010 10:51 PM, Michael Neuling wrote: I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it as well. There's only one CONFIG_GROWSUP arch - parisc. Could someone please test it on parisc? I did. How about doing: 'ulimit -s 15; ls' before and after the patch is applied. Before it's applied, 'ls' should be killed. After the patch is applied, 'ls' should no longer be killed. I'm suggesting a stack limit of 15KB since it's small enough to trigger 20*PAGE_SIZE. Also 15KB not a multiple of PAGE_SIZE, which is a trickier case to handle correctly with this code. 4K pages on parisc should be fine to test with. Mikey, thanks for the suggested test plan. I'm not sure if your patch does it correct for parisc/stack-grows-up-case. I tested your patch on a 4k pages kernel: r...@c3000:~# uname -a Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/Li nux Without your patch: r...@c3000:~# ulimit -s 15; ls Killed - correct. With your patch: r...@c3000:~# ulimit -s 15; ls Killed _or_: r...@c3000:~# ulimit -s 15; ls Segmentation fault - ?? Any idea? My x86_64 box also makes segmentation fault. I think ulimit -s 15 is too sm all stack for ls. ulimit -s 27; ls wroks perfectly fine. Arrh. I asked Helge offline earlier to check what use to work on parisc on 2.6.31. I guess PPC has a nice clean non-bloated ABI :-D Mikey ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] Restrict initial stack space expansion to rlimit
When reserving stack space for a new process, make sure we're not attempting to expand the stack by more than rlimit allows. This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba mm: variable length argument support and unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b exec: setup_arg_pages() fails to return errors. This bug means when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. Signed-off-by: Michael Neuling mi...@neuling.org Cc: sta...@kernel.org --- Attempts to answer comments from Kosaki Motohiro. Tested on PPC only, hence !CONFIG_STACK_GROWSUP. Someone should probably ACK for an arch with CONFIG_STACK_GROWSUP. As noted, stable needs the same patch, but 2.6.32 doesn't have the rlimit() helper. fs/exec.c | 21 ++--- 1 file changed, 18 insertions(+), 3 deletions(-) Index: linux-2.6-ozlabs/fs/exec.c === --- linux-2.6-ozlabs.orig/fs/exec.c +++ linux-2.6-ozlabs/fs/exec.c @@ -555,6 +555,7 @@ static int shift_arg_pages(struct vm_are } #define EXTRA_STACK_VM_PAGES 20 /* random */ +#define ALIGN_DOWN(addr,size) ((addr)(~((size)-1))) /* * Finalizes the stack vm_area_struct. The flags and permissions are updated, @@ -570,7 +571,7 @@ int setup_arg_pages(struct linux_binprm struct vm_area_struct *vma = bprm-vma; struct vm_area_struct *prev = NULL; unsigned long vm_flags; - unsigned long stack_base; + unsigned long stack_base, stack_expand, stack_expand_lim, stack_size; #ifdef CONFIG_STACK_GROWSUP /* Limit stack size to 1GB */ @@ -627,10 +628,24 @@ int setup_arg_pages(struct linux_binprm goto out_unlock; } + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_size = vma-vm_end - vma-vm_start; + if (rlimit(RLIMIT_STACK) stack_size) + stack_expand_lim = 0; /* don't shrick the stack */ + else + /* +* Align this down to a page boundary as expand_stack +* will align it up. +*/ + stack_expand_lim = ALIGN_DOWN(rlimit(RLIMIT_STACK) - stack_size, + PAGE_SIZE); + /* Initial stack must not cause stack overflow. */ + if (stack_expand stack_expand_lim) + stack_expand = stack_expand_lim; #ifdef CONFIG_STACK_GROWSUP - stack_base = vma-vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_base = vma-vm_end + stack_expand; #else - stack_base = vma-vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_base = vma-vm_start - stack_expand; #endif ret = expand_stack(vma, stack_base); if (ret) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Restrict initial stack space expansion to rlimit
When reserving stack space for a new process, make sure we're not attempting to expand the stack by more than rlimit allows. This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba mm: variable length argument support and unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b exec: setup_arg_pages() fails to return errors. This bug means when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. Signed-off-by: Michael Neuling mi...@neuling.org Cc: sta...@kernel.org --- Attempts to answer comments from Kosaki Motohiro. Tested on PPC only, hence !CONFIG_STACK_GROWSUP. Someone should probably ACK for an arch with CONFIG_STACK_GROWSUP. As noted, stable needs the same patch, but 2.6.32 doesn't have the rlimit() helper. fs/exec.c | 21 ++--- 1 file changed, 18 insertions(+), 3 deletions(-) Index: linux-2.6-ozlabs/fs/exec.c === --- linux-2.6-ozlabs.orig/fs/exec.c +++ linux-2.6-ozlabs/fs/exec.c @@ -555,6 +555,7 @@ static int shift_arg_pages(struct vm_are } #define EXTRA_STACK_VM_PAGES 20 /* random */ +#define ALIGN_DOWN(addr,size)((addr)(~((size)-1))) /* * Finalizes the stack vm_area_struct. The flags and permissions are updated, @@ -570,7 +571,7 @@ int setup_arg_pages(struct linux_binprm struct vm_area_struct *vma = bprm-vma; struct vm_area_struct *prev = NULL; unsigned long vm_flags; - unsigned long stack_base; + unsigned long stack_base, stack_expand, stack_expand_lim, stack_size; #ifdef CONFIG_STACK_GROWSUP /* Limit stack size to 1GB */ @@ -627,10 +628,24 @@ int setup_arg_pages(struct linux_binprm goto out_unlock; } + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_size = vma-vm_end - vma-vm_start; + if (rlimit(RLIMIT_STACK) stack_size) + stack_expand_lim = 0; /* don't shrick the stack */ + else + /* + * Align this down to a page boundary as expand_stack + * will align it up. + */ + stack_expand_lim = ALIGN_DOWN(rlimit(RLIMIT_STACK) - stack_size, + PAGE_SIZE); + /* Initial stack must not cause stack overflow. */ + if (stack_expand stack_expand_lim) + stack_expand = stack_expand_lim; #ifdef CONFIG_STACK_GROWSUP - stack_base = vma-vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_base = vma-vm_end + stack_expand; #else - stack_base = vma-vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_base = vma-vm_start - stack_expand; #endif ret = expand_stack(vma, stack_base); if (ret) Umm.. It looks correct. but the nested complex if statement seems a bit ugly. Instead, How about following? note: it's untested. === From: Michael Neuling mi...@neuling.org Subject: Restrict initial stack space expansion to rlimit When reserving stack space for a new process, make sure we're not attempting to expand the stack by more than rlimit allows. This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba mm: variable length argument support and unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b exec: setup_arg_pages() fails to return errors. This bug means when limiting the stack to less the 20*PAGE_SIZE (eg. 80K on 4K pages or 'ulimit -s 79') all processes will be killed before they start. This is particularly bad with 64K pages, where a ulimit below 1280K will kill every process. [kosaki.motoh...@jp.fujitsu.com: cleanups] Signed-off-by: Michael Neuling mi...@neuling.org Signed-off-by: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com Cc: sta...@kernel.org --- Attempts to answer comments from Kosaki Motohiro. Tested on PPC only, hence !CONFIG_STACK_GROWSUP. Someone should probably ACK for an arch with CONFIG_STACK_GROWSUP. As noted, stable needs the same patch, but 2.6.32 doesn't have the rlimit() helper. diff --git a/fs/exec.c b/fs/exec.c index 6f7fb0c..325bad4 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -573,6 +573,9 @@ int setup_arg_pages(struct linux_binprm *bprm, struct vm_area_struct *prev = NULL; unsigned long vm_flags; unsigned long stack_base; + unsigned long stack_size; + unsigned long stack_expand; + unsigned long rlim_stack; #ifdef CONFIG_STACK_GROWSUP /* Limit stack size to 1GB */ @@ -629,10 +632,27 @@ int setup_arg_pages(struct linux_binprm *bprm, goto out_unlock; } + stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE; + stack_size = vma-vm_end - vma-vm_start; + /* +* Align this down to a page boundary as expand_stack +* will align it up. +*/
