Patchwork Restrict stack space reservation to rlimit

login
register
mail settings
Submitter Michael Neuling
Date Feb. 8, 2010, 5:37 a.m.
Message ID <9729.1265607469@neuling.org>
Download mbox | patch
Permalink /patch/44757/
State Not Applicable
Headers show

Comments

Michael Neuling - Feb. 8, 2010, 5:37 a.m.
> >  
> > Hi,
> > 
> > > Why do we need page size independent stack size? It seems to have
> > > compatibility breaking risk.
> > 
> > I don't think so. The current behaviour is clearly wrong, we dont need a
> > 16x larger stack just because you went from a 4kB to a 64kB base page
> > size. The user application stack usage is the same in both cases.
> 
> I didn't discuss which behavior is better. Michael said he want to apply
> his patch to 2.6.32 & 2.6.33. stable tree never accept the breaking
> compatibility patch.
> 
> Your answer doesn't explain why can't we wait it until next merge window.
> 
> btw, personally, I like page size indepent stack size. but I'm not sure
> why making stack size independency is related to bug fix.

I tend to agree.  

Below is just the bug fix to limit the reservation size based rlimit.
We still reserve different stack sizes based on the page size as
before (unless we hit rlimit of course).

Mikey

Restrict stack space reservation to rlimit

When reserving stack space for a new process, make sure we're not
attempting to allocate more than rlimit allows.

This fixes a bug cause by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba 
"mm: variable length argument support" and unmasked by
fc63cf237078c86214abcb2ee9926d8ad289da9b 
"exec: setup_arg_pages() fails to return errors".

Signed-off-by: Michael Neuling <mikey@neuling.org>
Cc: Anton Blanchard <anton@samba.org>
Cc: stable@kernel.org
---
 fs/exec.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
KOSAKI Motohiro - Feb. 8, 2010, 6:05 a.m.
> > >  
> > > Hi,
> > > 
> > > > Why do we need page size independent stack size? It seems to have
> > > > compatibility breaking risk.
> > > 
> > > I don't think so. The current behaviour is clearly wrong, we dont need a
> > > 16x larger stack just because you went from a 4kB to a 64kB base page
> > > size. The user application stack usage is the same in both cases.
> > 
> > I didn't discuss which behavior is better. Michael said he want to apply
> > his patch to 2.6.32 & 2.6.33. stable tree never accept the breaking
> > compatibility patch.
> > 
> > Your answer doesn't explain why can't we wait it until next merge window.
> > 
> > btw, personally, I like page size indepent stack size. but I'm not sure
> > why making stack size independency is related to bug fix.
> 
> I tend to agree.  
> 
> Below is just the bug fix to limit the reservation size based rlimit.
> We still reserve different stack sizes based on the page size as
> before (unless we hit rlimit of course).

Thanks.

I agree your patch in almost part. but I have very few requests.


> Mikey
> 
> Restrict stack space reservation to rlimit
> 
> When reserving stack space for a new process, make sure we're not
> attempting to allocate more than rlimit allows.
> 
> This fixes a bug cause by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba 
> "mm: variable length argument support" and unmasked by
> fc63cf237078c86214abcb2ee9926d8ad289da9b 
> "exec: setup_arg_pages() fails to return errors".

Your initial mail have following problem use-case. please append it
into the patch description.

	On recent ppc64 kernels, limiting the stack (using 'ulimit -s blah') is
	now more restrictive than it was before.  On 2.6.31 with 4k pages I
	could run 'ulimit -s 16; /usr/bin/test' without a problem.  Now with
	mainline, even 'ulimit -s 64; /usr/bin/test' gets killed.


> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Cc: Anton Blanchard <anton@samba.org>
> Cc: stable@kernel.org
> ---
>  fs/exec.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6-ozlabs/fs/exec.c
> ===================================================================
> --- linux-2.6-ozlabs.orig/fs/exec.c
> +++ linux-2.6-ozlabs/fs/exec.c
> @@ -627,10 +627,13 @@ int setup_arg_pages(struct linux_binprm 
>  			goto out_unlock;
>  	}
>  
> +	stack_base = min(EXTRA_STACK_VM_PAGES * PAGE_SIZE,
> +			 current->signal->rlim[RLIMIT_STACK].rlim_cur -
> +			   PAGE_SIZE);

This line is a bit unclear why "- PAGE_SIZE" is necessary.
personally, I like following likes explicit comments.

	stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
	stack_lim = ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur);

	/* Initial stack must not cause stack overflow. */
	if (stack_expand + PAGE_SIZE > stack_lim)
		stack_expand = stack_lim - PAGE_SIZE;

note: accessing rlim_cur require ACCESS_ONCE.


Thought?


>  #ifdef CONFIG_STACK_GROWSUP
> -	stack_base = vma->vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> +	stack_base = vma->vm_end + stack_base;
>  #else
> -	stack_base = vma->vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> +	stack_base = vma->vm_start - stack_base;
>  #endif
>  	ret = expand_stack(vma, stack_base);
>  	if (ret)
>
WANG Cong - Feb. 8, 2010, 7:07 a.m.
On Mon, Feb 8, 2010 at 2:05 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> --- linux-2.6-ozlabs.orig/fs/exec.c
>> +++ linux-2.6-ozlabs/fs/exec.c
>> @@ -627,10 +627,13 @@ int setup_arg_pages(struct linux_binprm
>>                       goto out_unlock;
>>       }
>>
>> +     stack_base = min(EXTRA_STACK_VM_PAGES * PAGE_SIZE,
>> +                      current->signal->rlim[RLIMIT_STACK].rlim_cur -
>> +                        PAGE_SIZE);
>
> This line is a bit unclear why "- PAGE_SIZE" is necessary.
> personally, I like following likes explicit comments.
>
>        stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
>        stack_lim = ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur);
>
>        /* Initial stack must not cause stack overflow. */
>        if (stack_expand + PAGE_SIZE > stack_lim)
>                stack_expand = stack_lim - PAGE_SIZE;
>
> note: accessing rlim_cur require ACCESS_ONCE.
>
>
> Thought?

It's better to use the helper function: rlimit().
KOSAKI Motohiro - Feb. 8, 2010, 7:11 a.m.
> On Mon, Feb 8, 2010 at 2:05 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> --- linux-2.6-ozlabs.orig/fs/exec.c
> >> +++ linux-2.6-ozlabs/fs/exec.c
> >> @@ -627,10 +627,13 @@ int setup_arg_pages(struct linux_binprm
> >>                       goto out_unlock;
> >>       }
> >>
> >> +     stack_base = min(EXTRA_STACK_VM_PAGES * PAGE_SIZE,
> >> +                      current->signal->rlim[RLIMIT_STACK].rlim_cur -
> >> +                        PAGE_SIZE);
> >
> > This line is a bit unclear why "- PAGE_SIZE" is necessary.
> > personally, I like following likes explicit comments.
> >
> >        stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> >        stack_lim = ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur);
> >
> >        /* Initial stack must not cause stack overflow. */
> >        if (stack_expand + PAGE_SIZE > stack_lim)
> >                stack_expand = stack_lim - PAGE_SIZE;
> >
> > note: accessing rlim_cur require ACCESS_ONCE.
> >
> >
> > Thought?
> 
> It's better to use the helper function: rlimit().

AFAIK, stable tree doesn't have rlimit(). but yes, making two patch
(for mainline and for stable) is good opinion.
Michael Neuling - Feb. 8, 2010, 10:45 a.m.
In message <20100208145240.FB58.A69D9226@jp.fujitsu.com> you wrote:
> > > >  
> > > > Hi,
> > > > 
> > > > > Why do we need page size independent stack size? It seems to have
> > > > > compatibility breaking risk.
> > > > 
> > > > I don't think so. The current behaviour is clearly wrong, we dont need 
a
> > > > 16x larger stack just because you went from a 4kB to a 64kB base page
> > > > size. The user application stack usage is the same in both cases.
> > > 
> > > I didn't discuss which behavior is better. Michael said he want to apply
> > > his patch to 2.6.32 & 2.6.33. stable tree never accept the breaking
> > > compatibility patch.
> > > 
> > > Your answer doesn't explain why can't we wait it until next merge window.
> > > 
> > > btw, personally, I like page size indepent stack size. but I'm not sure
> > > why making stack size independency is related to bug fix.
> > 
> > I tend to agree.  
> > 
> > Below is just the bug fix to limit the reservation size based rlimit.
> > We still reserve different stack sizes based on the page size as
> > before (unless we hit rlimit of course).
> 
> Thanks.
> 
> I agree your patch in almost part. but I have very few requests.
> 
> 
> > Mikey
> > 
> > Restrict stack space reservation to rlimit
> > 
> > When reserving stack space for a new process, make sure we're not
> > attempting to allocate more than rlimit allows.
> > 
> > This fixes a bug cause by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba 
> > "mm: variable length argument support" and unmasked by
> > fc63cf237078c86214abcb2ee9926d8ad289da9b 
> > "exec: setup_arg_pages() fails to return errors".
> 
> Your initial mail have following problem use-case. please append it
> into the patch description.
> 
> 	On recent ppc64 kernels, limiting the stack (using 'ulimit -s blah') is
> 	now more restrictive than it was before.  On 2.6.31 with 4k pages I
> 	could run 'ulimit -s 16; /usr/bin/test' without a problem.  Now with
> 	mainline, even 'ulimit -s 64; /usr/bin/test' gets killed.

Ok,  I'll add this info in.  

> 
> > 
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Cc: Anton Blanchard <anton@samba.org>
> > Cc: stable@kernel.org
> > ---
> >  fs/exec.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > Index: linux-2.6-ozlabs/fs/exec.c
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/fs/exec.c
> > +++ linux-2.6-ozlabs/fs/exec.c
> > @@ -627,10 +627,13 @@ int setup_arg_pages(struct linux_binprm 
> >  			goto out_unlock;
> >  	}
> >  
> > +	stack_base = min(EXTRA_STACK_VM_PAGES * PAGE_SIZE,
> > +			 current->signal->rlim[RLIMIT_STACK].rlim_cur -
> > +			   PAGE_SIZE);
> 
> This line is a bit unclear why "- PAGE_SIZE" is necessary.

This is because the stack is already 1 page in size.  I'm going to
change that code to make it clearer...  hopefully :-)

> personally, I like following likes explicit comments.
> 
> 	stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> 	stack_lim = ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur);
> 
> 	/* Initial stack must not cause stack overflow. */
> 	if (stack_expand + PAGE_SIZE > stack_lim)
> 		stack_expand = stack_lim - PAGE_SIZE;
> 
> note: accessing rlim_cur require ACCESS_ONCE.
> 
> 
> Thought?

Thanks, looks better/clearer to me too.  I'll change, new patch coming....

Mikey

> 
> 
> >  #ifdef CONFIG_STACK_GROWSUP
> > -	stack_base = vma->vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> > +	stack_base = vma->vm_end + stack_base;
> >  #else
> > -	stack_base = vma->vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> > +	stack_base = vma->vm_start - stack_base;
> >  #endif
> >  	ret = expand_stack(vma, stack_base);
> >  	if (ret)
> > 
> 
> 
>

Patch

Index: linux-2.6-ozlabs/fs/exec.c
===================================================================
--- linux-2.6-ozlabs.orig/fs/exec.c
+++ linux-2.6-ozlabs/fs/exec.c
@@ -627,10 +627,13 @@  int setup_arg_pages(struct linux_binprm 
 			goto out_unlock;
 	}
 
+	stack_base = min(EXTRA_STACK_VM_PAGES * PAGE_SIZE,
+			 current->signal->rlim[RLIMIT_STACK].rlim_cur -
+			   PAGE_SIZE);
 #ifdef CONFIG_STACK_GROWSUP
-	stack_base = vma->vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
+	stack_base = vma->vm_end + stack_base;
 #else
-	stack_base = vma->vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
+	stack_base = vma->vm_start - stack_base;
 #endif
 	ret = expand_stack(vma, stack_base);
 	if (ret)