Re: [PATCH 3.18 32/32] mm: larger stack guard gap, between vmas

[Willy Tarreau]

On Tue, Jun 20, 2017 at 11:10:56PM -0700, Hugh Dickins wrote:
> On Wed, 21 Jun 2017, Willy Tarreau wrote:
> > On Tue, Jun 20, 2017 at 10:49:16PM -0700, Hugh Dickins wrote:
> > > On Mon, 19 Jun 2017, Greg Kroah-Hartman wrote:
> > > 
> > > > 3.18-stable review patch.  If anyone has any objections, please let me know.
> > > > 
> > > > ------------------
> > > > 
> > > > From: Hugh Dickins <hughd@xxxxxxxxxx>
> > > > 
> > > > commit 1be7107fbe18eed3e319a6c3e83c78254b693acb upstream.
> > > 
> > > Here's a few adjustments to the 3.18 patch: no doubt you'll have
> > > already sorted out any build errors (and I have to confess that
> > > I haven't even tried to build this); and the VM_WARN_ON line (as
> > > in 4.4) only fixes a highly unlikely error; but those FOLL_MLOCK
> > > lines in mm/gup.c were mistaken, and do need to be deleted.
> > 
> > Are you sure ? The test on the FOLL_MLOCK flag remains present in
> > 4.11 and mainline :
> > 
> >  	/* mlock all present pages, but do not fault in new pages */
> >  	if ((*flags & (FOLL_POPULATE | FOLL_MLOCK)) == FOLL_MLOCK)
> >  		return -ENOENT;
> > 
> > And this test was present although different in 3.18 as well :
> > 
> > 	/* For mlock, just skip the stack guard page. */
> > 	if ((*flags & FOLL_MLOCK) &&
> > 			(stack_guard_page_start(vma, address) ||
> > 			 stack_guard_page_end(vma, address + PAGE_SIZE)))
> > 
> > So by removing it we're totally removing any test on FOLL_MLOCK. That
> > might be the correct fix, but I'm just a bit surprized since the mainline
> > patch doesn't remove it, and only removes the test on FOLL_POPULATE.
> 
> I think I'm sure :)  Please take another look, the intention of those
> two FOLL_MLOCK tests is completely different.  One of them is about
> the mlock2() syscall (I think), which wants not to fault in every
> page at syscall time; and the other is about stack guard pages
> bogusly included in the vma extents, which must not be faulted in.
> The stack guard pages are no longer included in the vma extents,
> so we can just delete those lines (note the "&&" in the condition);
> but we don't want mlock() to stop faulting its pages in.
> 
> Makes sense now, or am I tired and confused?

Your explanation sounds pretty fine to me, so I agree with you :-)

Thanks!
Willy