[PATCH v2][BZ #22637] Fix stack guard size accounting

[PATCH v2][BZ #22637] Fix stack guard size accounting

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

v2:
- use separate bug number from the tls accounting issue.
- check for arithmetic overflows.

Previously if user requested S stack and G guard when creating a
thread, the total mapping was S and the actual available stack was
S - G - static_tls, which is not what the user requested.

This patch fixes the guard size accounting by pretending the user
requested S+G stack.  This way all later logic works out except
when reporting the user requested stack size (pthread_getattr_np)
or when computing the minimal stack size (__pthread_get_minstack).

Normally this will increase thread stack allocations by one page.
TLS accounting is not affected, that will require a separate fix.

	[BZ #22637]
	* nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize.
	* nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from
	stacksize.
	* nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise.

[-- Attachment #2: guard.diff --]
[-- Type: text/x-patch, Size: 1929 bytes --]

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 05b8ed331b..b374f4794d 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -532,6 +532,10 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
       /* Make sure the size of the stack is enough for the guard and
 	 eventually the thread descriptor.  */
       guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
+      if (guardsize < attr->guardsize || size + guardsize < guardsize)
+	/* Arithmetic overflow.  */
+	return EINVAL;
+      size += guardsize;
       if (__builtin_expect (size < ((guardsize + __static_tls_size
 				     + MINIMAL_REST_STACK + pagesize_m1)
 				    & ~pagesize_m1),
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index c237a3675a..5a4b52419f 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -472,8 +472,5 @@ strong_alias (__pthread_initialize_minimal_internal,
 size_t
 __pthread_get_minstack (const pthread_attr_t *attr)
 {
-  struct pthread_attr *iattr = (struct pthread_attr *) attr;
-
-  return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN
-	  + iattr->guardsize);
+  return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN;
 }
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index 961d711fd2..c79fd7b4ec 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -57,9 +57,10 @@ pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr)
   /* The sizes are subject to alignment.  */
   if (__glibc_likely (thread->stackblock != NULL))
     {
-      iattr->stacksize = thread->stackblock_size;
+      iattr->stacksize = thread->stackblock_size - thread->guardsize;
 #if _STACK_GROWS_DOWN
-      iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize;
+      iattr->stackaddr = (char *) thread->stackblock
+			 + thread->stackblock_size;
 #else
       iattr->stackaddr = (char *) thread->stackblock;
 #endif

Re: [PATCH v2][BZ #22637] Fix stack guard size accounting

[Carlos O'Donell]

On 01/08/2018 10:04 AM, Szabolcs Nagy wrote:
> v2:
> - use separate bug number from the tls accounting issue.
> - check for arithmetic overflows.
> 
> Previously if user requested S stack and G guard when creating a
> thread, the total mapping was S and the actual available stack was
> S - G - static_tls, which is not what the user requested.
> 
> This patch fixes the guard size accounting by pretending the user
> requested S+G stack.  This way all later logic works out except
> when reporting the user requested stack size (pthread_getattr_np)
> or when computing the minimal stack size (__pthread_get_minstack).
> 
> Normally this will increase thread stack allocations by one page.
> TLS accounting is not affected, that will require a separate fix.
> 
> 	[BZ #22637]
> 	* nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize.
> 	* nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from
> 	stacksize.
> 	* nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise.
> 
 
OK with the comments added as requested.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 05b8ed331b..b374f4794d 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -532,6 +532,10 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>        /* Make sure the size of the stack is enough for the guard and
>  	 eventually the thread descriptor.  */
>        guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
> +      if (guardsize < attr->guardsize || size + guardsize < guardsize)
> +	/* Arithmetic overflow.  */
> +	return EINVAL;
> +      size += guardsize;

OK.

>        if (__builtin_expect (size < ((guardsize + __static_tls_size
>  				     + MINIMAL_REST_STACK + pagesize_m1)
>  				    & ~pagesize_m1),
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index c237a3675a..5a4b52419f 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -472,8 +472,5 @@ strong_alias (__pthread_initialize_minimal_internal,
>  size_t
>  __pthread_get_minstack (const pthread_attr_t *attr)
>  {
> -  struct pthread_attr *iattr = (struct pthread_attr *) attr;
> -
> -  return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN
> -	  + iattr->guardsize);
> +  return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN;

OK. Happy to see this expression get simpler!

>  }
> diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
> index 961d711fd2..c79fd7b4ec 100644
> --- a/nptl/pthread_getattr_np.c
> +++ b/nptl/pthread_getattr_np.c
> @@ -57,9 +57,10 @@ pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr)
>    /* The sizes are subject to alignment.  */
>    if (__glibc_likely (thread->stackblock != NULL))
>      {

This operation needs a comment explaining why we subtract guard size.

> -      iattr->stacksize = thread->stackblock_size;
> +      iattr->stacksize = thread->stackblock_size - thread->guardsize;
>  #if _STACK_GROWS_DOWN
> -      iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize;
> +      iattr->stackaddr = (char *) thread->stackblock
> +			 + thread->stackblock_size;
>  #else
>        iattr->stackaddr = (char *) thread->stackblock;
>  #endif

You need to add a comment to nptl/descr.h to explain that stackblock_size
is the sum of the stack and guard.

-- 
Cheers,
Carlos.