Re: [PATCH] Fix stacksize returned for main process thread when RLIMIT_STACKis relevant

[KOSAKI Motohiro <kosaki dot motohiro at gmail dot com>]

(6/14/12 3:29 PM), Siddhesh Poyarekar wrote:
> Hi,
>
> This patch is a follow-up to the discussion:
>
> http://sourceware.org/ml/libc-alpha/2012-06/msg00310.html
>
> The stacksize returned by pthread_getattr_np is incorrect if the
> stacksize computed is based on rlimit since it should be adjusted
> against the pages we skip above __libc_stack_end. Further, if the
> stacksize is based on rlimit, it should be truncated to align to page
> size because the rlimit may not necessarily be page aligned.
>
> I have added a test case that verifies that the stack can extend up to
> the returned stackaddr for the main thread. Once the thread stack size
> issue is fixed, an analogous check can be added in the same test case
> for thread stacks too. I have verified that there are no regressions in
> the testsuite resulting from this change.

Thanks!
I have a few comment/questions.


> @@ -120,8 +120,15 @@ pthread_getattr_np (thread_id, attr)
>  		      && (uintptr_t) __libc_stack_end < to)
>  		    {
>  		      /* Found the entry.  Now we have the info we need.  */
> -		      iattr->stacksize = rl.rlim_cur;
>  		      iattr->stackaddr = stack_end;
> +		      iattr->stacksize =
> +		        rl.rlim_cur - (size_t) (to - (uintptr_t) stack_end);
> +
> +		      /* Cut it down to align it to page size since otherwise we
> +		         risk going beyond rlimit when the kernel rounds up the
> +		         stack extension request.  */
> +		      iattr->stacksize = (iattr->stacksize
> +					  & -(intptr_t) GLRO(dl_pagesize));

Hmm..
This comment is unclear to me. Which round up are you afraid? kernel always account
vma size. And, when unaligned stacksize is happen? Both "to" and "stack_end" seems
always page size aligned.


> +  /* Reduce the rlimit to a page less that what is currently being returned so
> +     that we ensure that pthread_getattr_np uses rlimit.  The figure is
> +     intentionally unaligned so to verify that pthread_getattr_np returns an
> +     aligned stacksize that correctly fits into the rlimit.  We don't bother
> +     about the case where the stack is limited by the vma below it and not by
> +     the rlimit because the stacksize returned in that case is computed from
> +     the end of that vma and is hence safe.  */
> +  if (stack_limit.rlim_cur > stacksize)
> +    {
> +      stack_limit.rlim_cur = stacksize - 4095;
> +      printf ("Adjusting RLIMIT_STACK to %zu¥n", stack_limit.rlim_cur);
> +      if ((ret = setrlimit (RLIMIT_STACK, &stack_limit)))

This test is also unclear to me.

Why "if (stack_limit.rlim_cur > stacksize)" check is necessary? It is always true
because we have no way to create a process w/o auxv.

And if I understand correctly, this setrlimit() never change a stacksize because
current pthread_getattr_np() is always returned page aligned size.
i.e. roundup(N*pagesize - 4095) == N*pagesize.

I'm not sure why do we need to bother unaligned size case.

Finally, if we should care "the stack is limited by the vma below" case, we should
reduce a stack pagesize(), I think allocate_and_test() assume there is at least 1
unmapped (or PROT_NONE) region at neighbor of a stack.