[3/7] Skip adding PROT_READ to non-PROT_READ mappings

Submitted by Abhishek Dubey on Aug. 25, 2019, 1:58 a.m.

Details

Message ID 1566698301-8740-4-git-send-email-dubeyabhishek777@gmail.com
State New
Series "GSoC 19: Optimizing the Pre-dump Algorithm"
Headers show

Commit Message

Abhishek Dubey Aug. 25, 2019, 1:58 a.m.
"read" mode pre-dump may fail even after
adding PROT_READ flag. Adding PROT_READ
works when dumping statically. See added
comment for details.

Signed-off-by: Abhishek Dubey <dubeyabhishek777@gmail.com>
---
 criu/mem.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/mem.c b/criu/mem.c
index 640b00a..15094f7 100644
--- a/criu/mem.c
+++ b/criu/mem.c
@@ -553,18 +553,51 @@  int parasite_dump_pages_seized(struct pstree_item *item,
 	 * able to read the memory contents.
 	 *
 	 * Afterwards -- reprotect memory back.
+	 *
+	 * This step is required for "splice" mode pre-dump and dump.
+	 * Skip this step for "read" mode pre-dump.
+	 * "read" mode pre-dump delegates processing of non-PROT_READ
+	 * regions to dump stage. Adding PROT_READ works fine for
+	 * static processing and fails for dynamic as explained below.
+	 *
+	 * Consider following sequence of instances to reason, why
+	 * not to add PROT_READ in "read" mode pre-dump ?
+	 *
+	 *    CRIU- "read" pre-dump         Target Process
+	 *
+	 *                              1. Creates mapping M
+	 *                                 without PROT_READ
+	 * 2. CRIU freezes target
+	 *    process
+	 * 3. Collect the mappings
+	 * 4. Add PROT_READ to M
+	 *    (non-PROT_READ region)
+	 * 5. CRIU unfreezes target
+	 *    process
+	 *                              6. Add flag PROT_READ
+	 *                                 to mapping M
+	 *                              7. Revoke flag PROT_READ
+	 *                                 from mapping M
+	 * 8. process_vm_readv tries
+	 *    to copy mapping M
+	 *    (believing M have
+	 *     PROT_READ flag)
+	 * 9. syscall fails to copy
+	 *    data from M
 	 */
 
-	pargs->add_prot = PROT_READ;
-	ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
-	if (ret) {
-		pr_err("Can't dump unprotect vmas with parasite\n");
-		return ret;
-	}
+	if (!mdc->pre_dump || opts.pre_dump_mode == PRE_DUMP_SPLICE) {
+		pargs->add_prot = PROT_READ;
+		ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
+		if (ret) {
+			pr_err("Can't dump unprotect vmas with parasite\n");
+			return ret;
+		}
 
-	if (fault_injected(FI_DUMP_PAGES)) {
-		pr_err("fault: Dump VMA pages failure!\n");
-		return -1;
+		if (fault_injected(FI_DUMP_PAGES)) {
+			pr_err("fault: Dump VMA pages failure!\n");
+			return -1;
+		}
 	}
 
 	ret = __parasite_dump_pages_seized(item, pargs, vma_area_list, mdc, ctl);
@@ -574,10 +607,12 @@  int parasite_dump_pages_seized(struct pstree_item *item,
 		return ret;
 	}
 
-	pargs->add_prot = 0;
-	if (compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl)) {
-		pr_err("Can't rollback unprotected vmas with parasite\n");
-		ret = -1;
+	if (!mdc->pre_dump || opts.pre_dump_mode == PRE_DUMP_SPLICE) {
+		pargs->add_prot = 0;
+		if (compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl)) {
+			pr_err("Can't rollback unprotected vmas with parasite\n");
+			ret = -1;
+		}
 	}
 
 	return ret;

Comments

Dmitry Safonov Aug. 28, 2019, 4:56 p.m.
On 8/25/19 2:58 AM, Abhishek Dubey wrote:
[..]
> -	pargs->add_prot = PROT_READ;
> -	ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
> -	if (ret) {
> -		pr_err("Can't dump unprotect vmas with parasite\n");
> -		return ret;
> -	}
> +	if (!mdc->pre_dump || opts.pre_dump_mode == PRE_DUMP_SPLICE) {
> +		pargs->add_prot = PROT_READ;
> +		ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
> +		if (ret) {
> +			pr_err("Can't dump unprotect vmas with parasite\n");
> +			return ret;
> +		}

Nit: could we separate it into:
static int parasite_mprotect_vmas(struct parasite_ctl *ctl, int prot)
{
     struct parasite_dump_pages_args *pargs;
     int ret;

     ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
     /*  ... */
}

>  
> -	if (fault_injected(FI_DUMP_PAGES)) {
> -		pr_err("fault: Dump VMA pages failure!\n");
> -		return -1;
> +		if (fault_injected(FI_DUMP_PAGES)) {
> +			pr_err("fault: Dump VMA pages failure!\n");
> +			return -1;
> +		}

Hmm, I might be mistaken, but it may make sense to leave it outside "if"?

>  	}
>  
>  	ret = __parasite_dump_pages_seized(item, pargs, vma_area_list, mdc, ctl);
> @@ -574,10 +607,12 @@ int parasite_dump_pages_seized(struct pstree_item *item,
>  		return ret;
>  	}
>  
> -	pargs->add_prot = 0;
> -	if (compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl)) {
> -		pr_err("Can't rollback unprotected vmas with parasite\n");
> -		ret = -1;
> +	if (!mdc->pre_dump || opts.pre_dump_mode == PRE_DUMP_SPLICE) {
> +		pargs->add_prot = 0;
> +		if (compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl)) {
> +			pr_err("Can't rollback unprotected vmas with parasite\n");
> +			ret = -1;
> +		}

And reuse it here?

Thanks,
          Dmitry
Abhishek Dubey Aug. 29, 2019, 3:15 a.m.
On 28/08/19 10:26 PM, Dmitry Safonov wrote:
> On 8/25/19 2:58 AM, Abhishek Dubey wrote:
> [..]
>> -	pargs->add_prot = PROT_READ;
>> -	ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
>> -	if (ret) {
>> -		pr_err("Can't dump unprotect vmas with parasite\n");
>> -		return ret;
>> -	}
>> +	if (!mdc->pre_dump || opts.pre_dump_mode == PRE_DUMP_SPLICE) {
>> +		pargs->add_prot = PROT_READ;
>> +		ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
>> +		if (ret) {
>> +			pr_err("Can't dump unprotect vmas with parasite\n");
>> +			return ret;
>> +		}
> Nit: could we separate it into:
> static int parasite_mprotect_vmas(struct parasite_ctl *ctl, int prot)
> {
>       struct parasite_dump_pages_args *pargs;
>       int ret;
>
>       ret = compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl);
>       /*  ... */
> }
>
>>   
>> -	if (fault_injected(FI_DUMP_PAGES)) {
>> -		pr_err("fault: Dump VMA pages failure!\n");
>> -		return -1;
>> +		if (fault_injected(FI_DUMP_PAGES)) {
>> +			pr_err("fault: Dump VMA pages failure!\n");
>> +			return -1;
>> +		}
> Hmm, I might be mistaken, but it may make sense to leave it outside "if"?
Yes, this is logically correct and can be done!
>
>>   	}
>>   
>>   	ret = __parasite_dump_pages_seized(item, pargs, vma_area_list, mdc, ctl);
>> @@ -574,10 +607,12 @@ int parasite_dump_pages_seized(struct pstree_item *item,
>>   		return ret;
>>   	}
>>   
>> -	pargs->add_prot = 0;
>> -	if (compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl)) {
>> -		pr_err("Can't rollback unprotected vmas with parasite\n");
>> -		ret = -1;
>> +	if (!mdc->pre_dump || opts.pre_dump_mode == PRE_DUMP_SPLICE) {
>> +		pargs->add_prot = 0;
>> +		if (compel_rpc_call_sync(PARASITE_CMD_MPROTECT_VMAS, ctl)) {
>> +			pr_err("Can't rollback unprotected vmas with parasite\n");
>> +			ret = -1;
>> +		}
> And reuse it here?
>
> Thanks,
>            Dmitry
-Abhishek