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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Cc: KOSAKI Motohiro<kosaki.motohiro@xxxxxxxxxxxxxx>
> > Cc: Americo Wang<xiyou.wangcong@xxxxxxxxx>
> > Cc: Anton Blanchard<anton@xxxxxxxxx>
> > Cc: Oleg Nesterov<oleg@xxxxxxxxxx>
> > Cc: James Morris<jmorris@xxxxxxxxx>
> > Cc: Ingo Molnar<mingo@xxxxxxx>
> > Cc: Serge Hallyn<serue@xxxxxxxxxx>
> > Cc: Benjamin Herrenschmidt<benh@xxxxxxxxxxxxxxxxxxx>
> > Cc:<stable@xxxxxxxxxx>
> >
> >   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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/