[CRIU] [PATCH v2 4/7] lazy-pages: fix memory corruption when combining pre-dump with lazy pages

[Mike Rapoport]

On Wed, Jun 21, 2017 at 04:52:07PM +0300, Pavel Emelyanov wrote:
> 
> >>> +static int maybe_disable_thp(struct pstree_item *t, struct page_read *pr)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	if (!(opts.lazy_pages && page_read_has_parent(pr)))
> >>
> >> Why checking for page_read_has_parent()? I mean -- what if page read doesn't
> >> have parent, why do we keep THP on?
> > 
> > There is no need to disable it if the page read doesn't have parent. In
> > this case VMA will be empty until userfaultfd_register, so there would be
> > no pages to collapse. And, once we register the VMA with uffd, khugepaged
> > will skip it.
> 
> Ah! Would you send PATCH 8/7 with this exact text as code comment? :)
> 
> >>> +		return 0;
> >>> +
> >>> +	if (!kdat.has_thp_disable)
> >>> +		pr_warn("Disabling transparent huge pages. "
> >>> +			"It may affect performance!\n");
> >>> +
> >>> +	/*
> >>> +	 * temporarily disable THP to avoid collapse of pages
> >>> +	 * in the areas that will be monitored by uffd
> >>> +	 */
> >>> +	ret = prctl(PR_GET_THP_DISABLE, 0, 0, 0, 0, 0);
> >>
> >> What is get for?
> > 
> > We re-enable THP after the restore, at least I've intended to do so :)
> 
> A-ha. Then, I guess, this bit should go into the image.

Hmm, right. Then it'll be v3 anyway.
 
> > If the process had THP disabled at the dump time we should keep it this
> > way.
> >  
> >>> +	if (unlikely(ret < 0))
> >>> +		return -1;
> >>> +	if (!ret && prctl(PR_SET_THP_DISABLE, 1, 0, 0, 0)) {
> >>> +		pr_perror("Cannot disable THP");
> >>> +		return -1;
> >>> +	}
> >>> +	rsti(t)->has_thp_enabled = !ret;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  int prepare_mappings(struct pstree_item *t)
> >>>  {
> >>>  	int ret = 0;
> >>> @@ -1055,6 +1084,9 @@ int prepare_mappings(struct pstree_item *t)
> >>>  	if (ret <= 0)
> >>>  		return -1;
> >>>  
> >>> +	if (maybe_disable_thp(t, &pr))
> >>> +		return -1;
> >>> +
> >>>  	pr.advance(&pr); /* shift to the 1st iovec */
> >>>  
> >>>  	ret = premap_priv_vmas(t, vmas, &addr, &pr);
> >>> @@ -1196,4 +1228,3 @@ int prepare_vmas(struct pstree_item *t, struct task_restore_args *ta)
> >>>  
> >>>  	return prepare_vma_ios(t, ta);
> >>>  }
> >>> -
> >>> diff --git a/criu/pie/restorer.c b/criu/pie/restorer.c
> >>> index 7b9c052..28a1129 100644
> >>> --- a/criu/pie/restorer.c
> >>> +++ b/criu/pie/restorer.c
> >>> @@ -1272,6 +1272,14 @@ long __export_restore_task(struct task_restore_args *args)
> >>>  	}
> >>>  
> >>>  	if (args->uffd > -1) {
> >>> +		/* re-enable THP if we disabled it previously */
> >>> +		if (args->has_thp_enabled) {
> >>> +			if (sys_prctl(PR_SET_THP_DISABLE, 1, 0, 0, 0)) {
> > 
> > And here should have been
> > sys_prctl(PR_SET_THP_DISABLE, 0, 0, 0, 0);
> > 
> > ;-)
> > 
> >>> +				pr_err("Cannot re-enable THP\n");
> >>> +				goto core_restore_end;
> >>> +			}
> >>> +		}
> >>> +
> >>>  		pr_debug("lazy-pages: closing uffd %d\n", args->uffd);
> >>>  		/*
> >>>  		 * All userfaultfd configuration has finished at this point.
> >>>
> >>
> > 
> > .
> > 
>