Message ID | 20150918201101.GD27881@eper |
---|---|
State | New |
Headers | show |
On Fri, Sep 18, 2015 at 09:11:01PM +0100, Balazs Kezes wrote: > On 2015-09-18 15:45 -0400, Rich Felker wrote: > > On Fri, Sep 18, 2015 at 08:29:52PM +0100, Balazs Kezes wrote: > > > So here's what I think pthreads should do: First mmap with PROT_NONE > > > and only then should mprotect read/write the stack pages. > > > > > > Does that sound reasonable? > > > > Yes. > > So here's the simplest patch I could come up with: > > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index 753da61..c6065dc 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -501,12 +501,21 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp, > size += pagesize_m1 + 1; > #endif > > - mem = mmap (NULL, size, prot, > + /* Map with PROT_NONE first and only then mprotect the pages to avoid > + the kernel unnecessary reserving the pages in the case of > + mlockall(MCL_FUTURE). */ > + mem = mmap (NULL, size, PROT_NONE, > MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); > > if (__glibc_unlikely (mem == MAP_FAILED)) > return errno; > > + if (__glibc_unlikely (mprotect (mem, size, prot) != 0)) > + { > + munmap(mem, size); > + return errno; > + } > + > /* SIZE is guaranteed to be greater than zero. > So we can never get a null pointer back from mmap. */ > assert (mem != NULL); > > > I've verified in my pthreads example that pthreads doesn't waste memory > with this patch applied. That's not really a nice fix though but this > allocatestack() function looks too scary to me. :( If this works, I think it's only due to a kernel bug of failing to apply the lock after mprotect. It's also going to be considerably slower, I think. What I had in mind was switching around the existing mmap/mprotect order, not adding an extra mprotect. Rich
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index 753da61..c6065dc 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -501,12 +501,21 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp, size += pagesize_m1 + 1; #endif - mem = mmap (NULL, size, prot, + /* Map with PROT_NONE first and only then mprotect the pages to avoid + the kernel unnecessary reserving the pages in the case of + mlockall(MCL_FUTURE). */ + mem = mmap (NULL, size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); if (__glibc_unlikely (mem == MAP_FAILED)) return errno; + if (__glibc_unlikely (mprotect (mem, size, prot) != 0)) + { + munmap(mem, size); + return errno; + } + /* SIZE is guaranteed to be greater than zero. So we can never get a null pointer back from mmap. */ assert (mem != NULL);