Re: [PATCH] Restrict initial stack space expansion to rlimit

Re: [PATCH] Restrict initial stack space expansion to rlimit

[Andrew Morton]

On Tue, 09 Feb 2010 19:59:27 +1100
Michael Neuling <mikey@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 <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)
_

Re: [PATCH] Restrict initial stack space expansion to rlimit

[Michael Neuling]

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

Re: [PATCH] Restrict initial stack space expansion to rlimit

[Helge Deller]

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)

Re: [PATCH] Restrict initial stack space expansion to rlimit

[KOSAKI Motohiro]

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

Re: [PATCH] Restrict initial stack space expansion to rlimit

[Michael Neuling]

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

Re: [PATCH] Restrict initial stack space expansion to rlimit

[Michael Neuling]

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

Re: [PATCH] Restrict initial stack space expansion to rlimit

[Helge Deller]

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

Re: [PATCH] Restrict initial stack space expansion to rlimit

[Michael Neuling]

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