Re: [PATCH] parisc,metag: Do not hardcode maximum userspace stack size

[Helge Deller]

Hi James,

On 05/13/2014 01:18 PM, James Hogan wrote:
> On 04/05/14 08:28, Helge Deller wrote:
>> On 05/02/2014 04:48 PM, James Bottomley wrote:
>>> On Fri, 2014-05-02 at 12:54 +0100, James Hogan wrote:
>>>> On 01/05/14 18:50, James Bottomley wrote:
>>>>>
>>>>>> +config MAX_STACK_SIZE_MB
>>>>>> +	int "Maximum user stack size (MB)"
>>>>>> +	default 80
>>>>>> +	range 8 256 if METAG
>>>>>> +	range 8 2048
>>>>>> +	depends on STACK_GROWSUP
>>>>>> +	help
>>>>>> +	  This is the maximum stack size in Megabytes in the VM layout of user
>>>>>> +	  processes when the stack grows upwards (currently only on parisc and
>>>>>> +	  metag arch). The stack will be located at the highest memory address
>>>>>> +	  minus the given value, unless the RLIMIT_STACK hard limit is changed
>>>>>> +	  to a smaller value in which case that is used.
>>>>>> +
>>>>>> +	  A sane initial value is 80 MB.
>>>>>
>>>>> There's one final issue with this: placement of the stack only really
>>>>> matters on 32 bits.  We have three expanding memory areas: stack, heap
>>>>> and maps.  On 64 bits these are placed well separated from each other on
>>>>> 64 bits, so an artificial limit like this doesn't matter.
>>>>
>>>> Does the following fixup diff look reasonable? It forces
>>>> MAX_STACK_SIZE_MB to 1024 and hides the Kconfig option for 64BIT,
>>>> effectively leaving the behaviour unchanged in that case.
>>>>
>>>> diff --git a/mm/Kconfig b/mm/Kconfig
>>>> index e80075979530..b0307f737bd7 100644
>>>> --- a/mm/Kconfig
>>>> +++ b/mm/Kconfig
>>>> @@ -583,7 +583,8 @@ config GENERIC_EARLY_IOREMAP
>>>>  	bool
>>>>
>>>>  config MAX_STACK_SIZE_MB
>>>> -	int "Maximum user stack size (MB)"
>>>> +	int "Maximum user stack size (MB)" if !64BIT
>>>> +	default 1024 if 64BIT
>>>>  	default 80
>>>>  	range 8 256 if METAG
>>>>  	range 8 2048
>>>
>>> Yes, I think that's probably correct ... 
>>
>> No, it's not correct.
>> It will then choose then a 1GB stack for compat tasks on 64bit kernel.
> 
> Sorry for the delay (I had most of last week off sick and still catching
> up).

No problem.
 
> That's a good point. It makes me think the best way to handle it is in a
> new definition in asm/processor.h, maybe STACK_SIZE_MAX. Does something
> like this look better? This patch isn't getting any cleaner
> unfortunately.


Yes, it's correct now.
Just tested it. Thanks!

Another problem:
I think you wanted to get it backported into older kernels?
It seems the changes to mm/Kconfig will not apply cleanly to 3.14 or lower,
and the changes to arch/parisc/kernel/sys_parisc.c will not apply to 3.13 or lower...

Maybe cleaning up the patch to apply cleanly to 3.14 would make sense
and ignore older kernels?

Helge


> From 6ecb0392a3b670c4bf1641a6ec56f22427ca8b57 Mon Sep 17 00:00:00 2001
> From: Helge Deller <deller@xxxxxx>
> Date: Wed, 30 Apr 2014 23:26:02 +0200
> Subject: [PATCH 1/1] parisc,metag: Do not hardcode maximum userspace stack
>  size
> 
> This patch affects only architectures where the stack grows upwards
> (currently parisc and metag only). On those do not hardcode the maximum
> initial stack size to 1GB for 32-bit processes, but make it configurable
> via a config option.
> 
> The main problem with the hardcoded stack size is, that we have two
> memory regions which grow upwards: stack and heap. To keep most of the
> memory available for heap in a flexmap memoy layout, it makes no sense
> to hard allocate up to 1GB of the memory for stack which can't be used
> as heap then.
> 
> This patch makes the stack size configurable and uses 80MB as default
> value which has been in use during the last few years on parisc and
> which didn't showed any problems yet.
> 
> This also fixes a BUG on metag if the RLIMIT_STACK hard limit is
> increased beyond a safe value by root. E.g. when starting a process
> after running "ulimit -H -s unlimited" it will then attempt to use a
> stack size of the maximum 1GB which is far too big for metag's limited
> user virtual address space (stack_top is usually 0x3ffff000):
> BUG: failure at fs/exec.c:589/shift_arg_pages()!
> 
> Signed-off-by: Helge Deller <deller@xxxxxx>
> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> Cc: linux-parisc@xxxxxxxxxxxxxxx
> Cc: linux-metag@xxxxxxxxxxxxxxx
> Cc: John David Anglin <dave.anglin@xxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # only needed for >= v3.9 (arch/metag)
> ---
> v3 (James Hogan):
>  - fix so that 64-bit parisc processes still use the 1GB limit.
>    CONFIG_STACK_GROWSUP arches should provide a STACK_SIZE_MAX in their
>    asm/processor.h, and for parisc it depends on USER_WIDE_MODE (whether
>    the current process is 64-bit).
> v2 (James Hogan):
>  - updated description to mention BUG on metag.
>  - added custom range limit for METAG.
>  - moved Kconfig symbol to mm/Kconfig and reworded.
>  - fixed "matag" typo.
> ---
>  arch/metag/include/asm/processor.h  |  2 ++
>  arch/parisc/include/asm/processor.h |  5 +++++
>  arch/parisc/kernel/sys_parisc.c     |  6 +++---
>  fs/exec.c                           |  6 +++---
>  mm/Kconfig                          | 15 +++++++++++++++
>  5 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/metag/include/asm/processor.h b/arch/metag/include/asm/processor.h
> index f16477d1f571..a8a37477c66e 100644
> --- a/arch/metag/include/asm/processor.h
> +++ b/arch/metag/include/asm/processor.h
> @@ -22,6 +22,8 @@
>  /* Add an extra page of padding at the top of the stack for the guard page. */
>  #define STACK_TOP	(TASK_SIZE - PAGE_SIZE)
>  #define STACK_TOP_MAX	STACK_TOP
> +/* Maximum virtual space for stack */
> +#define STACK_SIZE_MAX	(CONFIG_MAX_STACK_SIZE_MB*1024*1024)
> 
>  /* This decides where the kernel will search for a free chunk of vm
>   * space during mmap's.
> diff --git a/arch/parisc/include/asm/processor.h b/arch/parisc/include/asm/processor.h
> index 198a86feb574..d951c9681ab3 100644
> --- a/arch/parisc/include/asm/processor.h
> +++ b/arch/parisc/include/asm/processor.h
> @@ -55,6 +55,11 @@
>  #define STACK_TOP	TASK_SIZE
>  #define STACK_TOP_MAX	DEFAULT_TASK_SIZE
> 
> +/* Allow bigger stacks for 64-bit processes */
> +#define STACK_SIZE_MAX	(USER_WIDE_MODE					\
> +			 ? (1 << 30)	/* 1 GB */			\
> +			 : (CONFIG_MAX_STACK_SIZE_MB*1024*1024))
> +
>  #endif
> 
>  #ifndef __ASSEMBLY__
> diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c
> index 31ffa9b55322..e1ffea2f9a0b 100644
> --- a/arch/parisc/kernel/sys_parisc.c
> +++ b/arch/parisc/kernel/sys_parisc.c
> @@ -72,10 +72,10 @@ static unsigned long mmap_upper_limit(void)
>  {
>  	unsigned long stack_base;
> 
> -	/* Limit stack size to 1GB - see setup_arg_pages() in fs/exec.c */
> +	/* Limit stack size - see setup_arg_pages() in fs/exec.c */
>  	stack_base = rlimit_max(RLIMIT_STACK);
> -	if (stack_base > (1 << 30))
> -		stack_base = 1 << 30;
> +	if (stack_base > STACK_SIZE_MAX)
> +		stack_base = STACK_SIZE_MAX;
> 
>  	return PAGE_ALIGN(STACK_TOP - stack_base);
>  }
> diff --git a/fs/exec.c b/fs/exec.c
> index 476f3ebf437e..238b7aa26f68 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -657,10 +657,10 @@ int setup_arg_pages(struct linux_binprm *bprm,
>  	unsigned long rlim_stack;
> 
>  #ifdef CONFIG_STACK_GROWSUP
> -	/* Limit stack size to 1GB */
> +	/* Limit stack size */
>  	stack_base = rlimit_max(RLIMIT_STACK);
> -	if (stack_base > (1 << 30))
> -		stack_base = 1 << 30;
> +	if (stack_base > STACK_SIZE_MAX)
> +		stack_base = STACK_SIZE_MAX;
> 
>  	/* Make sure we didn't let the argument array grow too large. */
>  	if (vma->vm_end - vma->vm_start > stack_base)
> diff --git a/mm/Kconfig b/mm/Kconfig
> index ebe5880c29d6..1b5a95f0fa01 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -581,3 +581,18 @@ config PGTABLE_MAPPING
> 
>  config GENERIC_EARLY_IOREMAP
>  	bool
> +
> +config MAX_STACK_SIZE_MB
> +	int "Maximum user stack size for 32-bit processes (MB)"
> +	default 80
> +	range 8 256 if METAG
> +	range 8 2048
> +	depends on STACK_GROWSUP && (!64BIT || COMPAT)
> +	help
> +	  This is the maximum stack size in Megabytes in the VM layout of 32-bit
> +	  user processes when the stack grows upwards (currently only on parisc
> +	  and metag arch). The stack will be located at the highest memory
> +	  address minus the given value, unless the RLIMIT_STACK hard limit is
> +	  changed to a smaller value in which case that is used.
> +
> +	  A sane initial value is 80 MB.
> 

--
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/