| Message ID | 5A53B2BA.1090500@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,BZ,#22637] Fix stack guard size accounting | expand |
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.
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