<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 11, 2020 at 9:08 PM Cyril Hrubis <<a href="mailto:chrubis@suse.cz">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail_default" style="font-size:small">...</span><br>
> +static void allocate_stack(size_t size)<br>
> +{<br>
> + void *start;<br>
> +<br>
> + /*<br>
> + * Since the newer kernel does not allow a MAP_GROWSDOWN mapping to grow<br>
> + * closer than 'stack_guard_gap' pages away from a preceding mapping.<br>
> + * The guard then ensures that the next-highest mapped page remains more<br>
> + * than 'stack_guard_gap' below the lowest stack address, and if not then<br>
> + * it will trigger a segfault. So, here adding 256 pages memory spacing<br>
> + * for stack growing safely.<br>
> + *<br>
> + * Btw, kernel default 'stack_guard_gap' size is '256 * getpagesize()'.<br>
> + */<br>
> + long total_size = 256 * getpagesize() + size * 2;<br>
> +<br>
> + start = SAFE_MMAP(NULL, total_size, PROT_READ | PROT_WRITE,<br>
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);<br>
> + SAFE_MUNMAP(start, total_size);<br>
> +<br>
> + /* start stack<br>
> + * +-----------+---------------------+----------------------+<br>
> + * | 256 pages | unmapped (size) | mapped (size) |<br>
> + * +-----------+---------------------+----------------------+<br>
> + *<br>
> + */<br>
> + stack = SAFE_MMAP((start + total_size - size), size, PROT_READ | PROT_WRITE,<br>
> + MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS | MAP_GROWSDOWN,<br>
> + -1, 0);<br>
<br>
Well it's not wrong per se but as it is we do not use the pre-allocated<br>
part of the stack at all, we directly jump for the guard page as we use<br></blockquote><div><br></div><div class="gmail_default" style="font-size:small">Really? But I think the pthread_attr_setstack(&attr, stack, stack_size) will</div><div class="gmail_default">take use of the whole stack memory in function recursive performing.</div><div class="gmail_default">How can we say NOT use the pre-allocated stack? I fell a bit confused</div><div class="gmail_default" style="font-size:small">about your words here.</div><div class="gmail_default" style="font-size:small"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
the stack pointer as a base for the pthread stack. The actual pointer<br>
that points to the start of the region is stack - stack_size.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">If that's true, it might belong to the internal operation of stack grows,</div><div class="gmail_default" style="font-size:small">should we take care of it in the userspace case?</div></div><div class="gmail_default" style="font-size:small"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
There is no point in adding size * 2 here. We can as well reserve 256 *<br>
page_size + size. Then map() a single page at the end, which would be at<br>
start + total_size - page_size and finally return start + total_size<br>
from this function and pass that to pthread_attr_setstack().<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I guess that will be work, but sounds a bit stingy. Since the modern system</div><div class="gmail_default" style="font-size:small">does not short of such memory for testing:). And if we decide to go with this,</div></div><div class="gmail_default" style="font-size:small">the code design comments above should be all rewrite.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
That way it would look like:<br>
<br>
| 256 pages | unmapped | 1 mapped page |<br>
<br>
| - - - stack_size - - - |<br>
<br>
<br>
<span class="gmail_default" style="font-size:small">...</span><br>
> + SAFE_PTHREAD_CREATE(&test_thread, &attr, check_depth_recursive, limit);<br>
> + SAFE_PTHREAD_JOIN(test_thread, NULL);<br>
> +<br>
> + if (stack)<br>
> + SAFE_MUNMAP(stack, stack_size);<br>
<br>
I'ts a bit unexpected to unmap the stack here. I guess that unamping it<br>
in the run_test() after the grow_stack() call would be a bit cleaner but<br>
we would have to move the exit(0) there as well.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I agree with this.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + /* Test 2 */<br>
> + child_pid = SAFE_FORK();<br>
> + if (!child_pid) {<br>
> + tst_no_corefile(0);<br>
^<br>
This should go to the test setup.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Only the child_2 will get SIGSEGV, why should we move it to affect the whole test?</div></div></div><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>