[CRIU] [PATCH 2/2] mem: Don't assume guard page is returned in procfs with new kernels

[Cyrill Gorcunov]

On Tue, Jun 20, 2017 at 03:02:47PM +0200, Oleg Nesterov wrote:
> Add CC's, we will need to fix criu for rhel...
> 
> 
> On 06/20, Cyrill Gorcunov wrote:
> >
> > If the guard page is not reported in show_map_vma we should
> > not ajust vma address neither we should call unmap_guard_pages
> > in restorer.
> > 
> > https://github.com/xemul/criu/issues/322
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> > ---
> >  criu/cr-restore.c | 3 ++-
> >  criu/proc_parse.c | 6 ++++--
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> > index 298fb693a09e..d44e723cc180 100644
> > --- a/criu/cr-restore.c
> > +++ b/criu/cr-restore.c
> > @@ -1812,7 +1812,8 @@ static int restore_task_with_children(void *_arg)
> >  
> >  	timing_stop(TIME_FORK);
> >  
> > -	if (unmap_guard_pages(current))
> > +	if (kdat.mm_guard_page_maps &&
> > +	    unmap_guard_pages(current))
> 
> can't resist... I feel that unmap_guard_pages() logic is not really right and
> this helper can be simply removed even with stack-guard... but I can be easily
> wrong and this is off-topic right now.

You know, we're running tests now and found that not calling unmap_guard_pages
on new kernel left some pages on restored process. They are came from other
restore code which assumes that on growsdown flag page-size need to added/removed.
So as a fast fix unmap_guard_pages still should be called.

> >  
> >  	restore_pgid();
> > diff --git a/criu/proc_parse.c b/criu/proc_parse.c
> > index 041d4512413d..4cb4b9dbf288 100644
> > --- a/criu/proc_parse.c
> > +++ b/criu/proc_parse.c
> > @@ -638,8 +638,10 @@ static int vma_list_add(struct vma_area *vma_area,
> >  
> >  	/* Add a guard page only if here is enough space for it */
> >  	if ((vma_area->e->flags & MAP_GROWSDOWN) &&
> > -	    *prev_end < vma_area->e->start)
> > -		vma_area->e->start -= PAGE_SIZE; /* Guard page */
> > +	    *prev_end < vma_area->e->start) {
> > +		if (kdat.mm_guard_page_maps)
> > +			vma_area->e->start -= PAGE_SIZE; /* Guard page */
> > +	}
> 
> OK, but what about MAP_GROWSDOWN checks in criu/mem.c ? At first glance it
> seems that most of them (if not all) should be updated too, no?

Exactly. I need to revisit all over the places, sigh.

> Perhaps you can make a helper? something like
> 
> 	bool vma_has_guard(vma_area *vma)
> 	{
> 		return kdat.mm_guard_page_maps && (vma->e->flags & MAP_GROWSDOWN);
> 	}
> 
> then it would be simpler to change the code whenever this is needed.
> 
> And. This can also help us (redhat) ;) more on this later, but most probably
> we will need to disable this logic unconditionally (iow, without the 1st patch)
> for the short term fix.

Why? The patches gonna be in vanilla repo I think for you it should be
easlier to simply cherry-pick.