| Message ID | 20100209132529.bfc455b7.akpm@linux-foundation.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
> > > 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 <mikey@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 <mikey@neuling.org> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> > Cc: Americo Wang <xiyou.wangcong@gmail.com> > Cc: Anton Blanchard <anton@samba.org> > Cc: Oleg Nesterov <oleg@redhat.com> > Cc: James Morris <jmorris@namei.org> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Serge Hallyn <serue@us.ibm.com> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: <stable@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) > _ >
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: root@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: root@c3000:~# ulimit -s 15; ls Killed -> correct. With your patch: root@c3000:~# ulimit -s 15; ls Killed _or_: root@c3000:~# ulimit -s 15; ls Segmentation fault -> ?? Any idea? Helge >> From: Michael Neuling<mikey@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<mikey@neuling.org> >> Cc: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com> >> Cc: Americo Wang<xiyou.wangcong@gmail.com> >> Cc: Anton Blanchard<anton@samba.org> >> Cc: Oleg Nesterov<oleg@redhat.com> >> Cc: James Morris<jmorris@namei.org> >> Cc: Ingo Molnar<mingo@elte.hu> >> Cc: Serge Hallyn<serue@us.ibm.com> >> Cc: Benjamin Herrenschmidt<benh@kernel.crashing.org> >> Cc:<stable@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)
> 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: > root@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: > root@c3000:~# ulimit -s 15; ls > Killed > -> correct. > > With your patch: > root@c3000:~# ulimit -s 15; ls > Killed > _or_: > root@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.
In message <20100210141016.4D18.A69D9226@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: > > root@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: > > root@c3000:~# ulimit -s 15; ls > > Killed > > -> correct. > > > > With your patch: > > root@c3000:~# ulimit -s 15; ls > > Killed > > _or_: > > root@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
In message <20100210141016.4D18.A69D9226@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: > > root@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: > > root@c3000:~# ulimit -s 15; ls > > Killed > > -> correct. > > > > With your patch: > > root@c3000:~# ulimit -s 15; ls > > Killed > > _or_: > > root@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
On 02/10/2010 06:31 AM, Michael Neuling wrote: > In message<20100210141016.4D18.A69D9226@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: >>> root@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: >>> root@c3000:~# ulimit -s 15; ls >>> Killed >>> -> correct. >>> >>> With your patch: >>> root@c3000:~# ulimit -s 15; ls >>> Killed >>> _or_: >>> root@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
In message <4B7481A6.7080300@gmx.de> you wrote: > On 02/10/2010 06:31 AM, Michael Neuling wrote: > > In message<20100210141016.4D18.A69D9226@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: > >>> root@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: > >>> root@c3000:~# ulimit -s 15; ls > >>> Killed > >>> -> correct. > >>> > >>> With your patch: > >>> root@c3000:~# ulimit -s 15; ls > >>> Killed > >>> _or_: > >>> root@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
--- 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 <mikey@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 <mikey@neuling.org> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> Cc: Americo Wang <xiyou.wangcong@gmail.com> Cc: Anton Blanchard <anton@samba.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Ingo Molnar <mingo@elte.hu> Cc: Serge Hallyn <serue@us.ibm.com> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: <stable@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)