| Message ID | 9729.1265607469@neuling.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
> > > > > > 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) >
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().
> 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.
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) > > > > >
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)