Prev: Congratulations
Next: [PATCH] security/integrity/ima/ima_iint.c: Remove goto out int ima_inode_alloc()
From: KOSAKI Motohiro on 8 Feb 2010 00:10
Hi

> apkm, linus: this or something like it needs to go into 2.6.33 (& 32) to
> fix 'ulimit -s'.

"fix ulimit -s" is too cool explanation ;-)
we are not ESPer. please consider to provide what bug is exist.


> Mikey
>
> [PATCH] 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.
>
> Also, reserve the same stack size independent of page size.

Why do we need page size independent stack size? It seems to have
compatibility breaking risk.


>
> This fixes a bug unmasked by fc63cf237078c86214abcb2ee9926d8ad289da9b
>
> Signed-off-by: Michael Neuling <mikey(a)neuling.org>
> Cc: Anton Blanchard <anton(a)samba.org>
> Cc: stable(a)kernel.org
> ---
> fs/exec.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> Index: clone1/fs/exec.c
> ===================================================================
> --- clone1.orig/fs/exec.c
> +++ clone1/fs/exec.c
> @@ -554,7 +554,7 @@ static int shift_arg_pages(struct vm_are
> return 0;
> }
>
> -#define EXTRA_STACK_VM_PAGES 20 /* random */
> +#define EXTRA_STACK_VM_SIZE 81920UL /* randomly 20 4K pages */
>
> /*
> * Finalizes the stack vm_area_struct. The flags and permissions are updated,
> @@ -627,10 +627,13 @@ int setup_arg_pages(struct linux_binprm
> goto out_unlock;
> }
>
> + stack_base = min(EXTRA_STACK_VM_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)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html



--
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: Anton Blanchard on 8 Feb 2010 00:20

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.

Anton
--
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 00:30
>
> 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.



--
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: Anton Blanchard on 8 Feb 2010 00:40

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

Anton
--
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 01:10
> > >
> > > 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(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.
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)
>



--
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/
 |  Next  |  Last
Pages: 1 2
Prev: Congratulations
Next: [PATCH] security/integrity/ima/ima_iint.c: Remove goto out int ima_inode_alloc()