Prev: Congratulations
Next: [PATCH] security/integrity/ima/ima_iint.c: Remove goto out int ima_inode_alloc()
From: KOSAKI Motohiro on 8 Feb 2010 01:20
>
> Hi,
>
> > 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.
>
> OK sorry, I misunderstood your initial mail. I agree fixing the bit that
> regressed in 2.6.32 is the most important thing. The difference in page size is
> clearly wrong but since it isn't a regression we could probably live with it
> until 2.6.34

thanks!


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Américo Wang on 8 Feb 2010 02:10
On Mon, Feb 8, 2010 at 2:05 PM, KOSAKI Motohiro
<kosaki.motohiro(a)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().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 8 Feb 2010 02:20
> On Mon, Feb 8, 2010 at 2:05 PM, KOSAKI Motohiro
> <kosaki.motohiro(a)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.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michael Neuling on 8 Feb 2010 05:50

In message <20100208145240.FB58.A69D9226(a)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(a)neuling.org>
> > Cc: Anton Blanchard <anton(a)samba.org>
> > Cc: stable(a)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)
> >
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: Congratulations
Next: [PATCH] security/integrity/ima/ima_iint.c: Remove goto out int ima_inode_alloc()