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

[Pavel Emelyanov]

>>> +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.

> 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.
>>>
>>
> 
> .
>