Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c82b6466-9c2a-777e-eaae-388b8bae4318@amd.com>
Date:   Tue, 1 Nov 2022 22:19:12 -0400
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] drm/amdkfd: Fix error handling in criu_checkpoint
Content-Language: en-CA
To:     Felix Kuehling <Felix.Kuehling@amd.com>,
        amd-gfx@lists.freedesktop.org
Cc:     linux-kernel@vger.kernel.org, Jann Horn <jannh@google.com>
References: <20221101191559.302884-1-Felix.Kuehling@amd.com>
From:   "Bhardwaj, Rajneesh" <rajneesh.bhardwaj@amd.com>
In-Reply-To: <20221101191559.302884-1-Felix.Kuehling@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cE9ZV0MxS0RGYnk4YUFsaDRyL0NiTE9vTUhQQU1IMG5sbmN0MkZmMTZWZXho?=
 =?utf-8?B?QkhaSjFSUVAyb3oycUp4WDhHYTJ2SU4rc1RxdExSMXVSdCtFZmpOYnRVTVdK?=
 =?utf-8?B?QVZ4VXN1aTRhQ2pBY2cycjFIM2tkM2xLV2pjNkJJbERMKzR6bXpwQ1BTZE5s?=
 =?utf-8?B?VWF6UERuQUwydVhuTGh6NDhVWkU1ZE5YQ243dUtxeFpNTkdPTXNrc1ZVcUdm?=
 =?utf-8?B?TzJVSy8vOUw0SndqSkpheHJNUUkySmx0Q1BnWkU5U0dnd3VRZENOeStYeW00?=
 =?utf-8?B?VllKcnpjcGVSVFhjbVhqaXZ2OFMyRUdqVEpYcUVGNVU0bGVPdG5mWTFGMWJs?=
 =?utf-8?B?NGVvVXk0VVB2clVkejl4czYyaCtIeDR2bmJ2MmpocXF0WU9CQTQwdEtxekpH?=
 =?utf-8?B?RnVubVhDR1czS0x5b1BJbGZxV3BFMm1CREJ6VXJjOTQ0RUNabGc3elVFdW56?=
 =?utf-8?B?WlQvenpWNzZoWWQ5Z2FDY1E2cHphaGFMYXVUQU84K0dqS3dtZFphcnFyaTI5?=
 =?utf-8?B?WjBDaWU5M1I4SUhhOFdtWndhTDNISGRvajNrMTJQZWFrNll1T2JvY2J3NW9x?=
 =?utf-8?B?Lzd5TExuZlI4ZTM4WXF4eWZaUE81SWwwREFBcUp5S0hDM0l6cjNGaHFoT01E?=
 =?utf-8?B?K0FRQjk3RGdoL21pQWFLdjRZbnlaOEdoOUszNysxUitDUTllemhoWDA4R012?=
 =?utf-8?B?WTlNNlpISzgwV1VRaUttRXJ6a2Y1SnpJMkFZelc3dVp0MWxLRlFLU1RES0U2?=
 =?utf-8?B?T2R5a3g1Ukd0YmRyRXcxT0NBUjhPUkdMZHJzdC9xWkhYZlhnSXg3SVFZU2Js?=
 =?utf-8?B?c2lHV0Y3enlHdjZ0MlZiUmxvYVJWcHBDL1pvWXdIQUlWSGVvZGlEK0VJYWk0?=
 =?utf-8?B?aEVZRlVyVkZEYTUrQWFWTVpuUmFhUnZqc2NZZ1J6SjM2UzJGOENFc0dQUk5u?=
 =?utf-8?B?NE5CSGdZdzVIZ3pVVGlVa2NZei9JZytsbnI4OGdFRStHeG5SVU95OTdnU3VE?=
 =?utf-8?B?RVNvanRiek1oSTFyeVZHclZjc3FJNEZRdUNGb3NHYzBEZkhWNWUxVTNpUXJj?=
 =?utf-8?B?WjhBTEpYd1dKWjU3L3pwSWlBZ3JOUTREQ1VSOE8rT2FKR1ZpWjZ4ek5qOGVV?=
 =?utf-8?B?eWFzUUcxc1lzRDBpRG5YUWhmSTNBRHcyVmNJSDY0RlJGNXI2aitKODJ6b0JL?=
 =?utf-8?B?OEhCL0lhTFgyb2ZycWordWxVQlF2aXJVUWRGcVAwU2trR1VHdk9pSkJpQ1Q1?=
 =?utf-8?B?S1dOWnV0TXZkQXFqdklFemNJME04ajFESHREeHdWNzQxY1hkQlRQbTVUZllx?=
 =?utf-8?B?VzZWRDFRc1JFUlJMZ2QrM3RNTFdKRkZqNytrb0l0eG9pYXcyaFdKRDhYYUpt?=
 =?utf-8?B?MmJyOXNaaktTNUFwVXJnOGRqOSt6Ym8vN044eHc0bmZ0S1Z1U0JldXBsOEZZ?=
 =?utf-8?B?SUNhb2FETVFiUnRnTFQzNmorYWptZm9oakpZV0p5REdUWGVBU242MFlDN3o1?=
 =?utf-8?B?aG9HYnlXRTMybjlhTzNxcksyVS9nUGpMc1pGTzlHZEJMeHQ2eWpuYWtXQUNw?=
 =?utf-8?B?RW1KNkRTOTZZQTk5d1pzN05CTWdYb1Fmek5IdERCc3NFSlRrMk5Rb3FkMGRL?=
 =?utf-8?B?ZGg2d2FlTmE4Q2VmSnowSnJhSFFpTlJha0ZVcUdjZkp2cmFQclBqRlhTYTlH?=
 =?utf-8?B?YnMyMWU2ZUMwa25nQmVTbkJDUUNiTmNhN0h5aVdMN0R6cHRRNDBNaGU1QWhi?=
 =?utf-8?B?YVVZVFgyZjZVcVdWZ2YvV0Jjd3JuMU1CSjZFSGZMTmtaeXE3TFVhQjd4MUY1?=
 =?utf-8?B?NnFQQUdvQ25hKytUb0NOQlJRUGZsc21odFMwcHdqSWlIaW9VaWlRclplT0F6?=
 =?utf-8?B?R3ZxMWFqaEVhRkJ1SDd4NktwTjRtdlFmV1FCQ0NjOHh4V29Xc1Q5MFJkMWVv?=
 =?utf-8?B?UXNqZEcxWHJZK1R5YUM4OVd3WnorMlptVHpGMDVFSHo2ajJMWlVCYU9pRGht?=
 =?utf-8?B?K3BqVDF5U0tETVNZUUx1VDMvWW5aUGlZY2swOTZEa2RCbFdmeDE0UUJOdC9W?=
 =?utf-8?B?aFRmTlhrZW1pQ1Z6bGxIa0czTk8va3RzZUdaLzkwV21XdUxCcUFGRHp4djFq?=
 =?utf-8?Q?1SRQjpmIBF4mQTimXQhwvX7Zg?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 78226cd3-b70e-4e0d-09da-08dabc78a352
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5933.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2022 02:19:14.9912
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: m44L5a3UJBCYYqx1DydEE//pEkW79kv8qVHgR7YEJHg8drjzWRv9rPf+ttxYN15V+F219pIHJ22pcHedhktToA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4181
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 11/1/2022 3:15 PM, Felix Kuehling wrote:
> Checkpoint BOs last. That way we don't need to close dmabuf FDs if
> something else fails later. This avoids problematic access to user mode
> memory in the error handling code path.
>
> criu_checkpoint_bos has its own error handling and cleanup that does not
> depend on access to user memory.


This seems to be breaking the restore operation. I did a quick pytorch 
based test and I can confirm that restore operation fails with this 
change applied.

[  +0.000003] CR2: 000055b6726e0020 CR3: 00000001283fe000 CR4: 
00000000003 50ee0
[  +0.000002] Call Trace:
[  +0.000002]  <TASK>
[  +0.000003]  kfd_ioctl_criu+0xd4c/0x1930 [amdgpu]
[  +0.000185]  ? __might_fault+0x32/0x80
[  +0.000004]  ? lock_release+0x1fd/0x2b0
[  +0.000010]  kfd_ioctl+0x29b/0x600 [amdgpu]
[  +0.000153]  ? kfd_ioctl_get_tile_config+0x130/0x130 [amdgpu]
[  +0.000158]  __x64_sys_ioctl+0x8b/0xd0
[  +0.000003]  ? lockdep_hardirqs_on+0x79/0x100
[  +0.000007]  do_syscall_64+0x34/0x80
[  +0.000004]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  +0.000005] RIP: 0033:0x7f1c87e7f317
[  +0.000002] Code: b3 66 90 48 8b 05 71 4b 2d 00 64 c7 00 26 00 00 00 
48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 
05 <4 8> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 41 4b 2d 00 f7 d8 64 89 01 48
[  +0.000003] RSP: 002b:00007fff630af518 EFLAGS: 00000202 ORIG_RAX: 
000000 0000000010
[  +0.000003] RAX: ffffffffffffffda RBX: 00007f1c89351620 RCX: 
00007f1c87e 7f317
[  +0.000002] RDX: 00007fff630af5c0 RSI: 00000000c0384b22 RDI: 
00000000000 00005
[  +0.000002] RBP: 00007fff630af550 R08: 0000000000000000 R09: 
00007f1c87e d7c10
[  +0.000002] R10: 705f757067646d61 R11: 0000000000000202 R12: 
000055a4a05 14c60
[  +0.000002] R13: 000055a49eb81540 R14: 000055a49e90eea9 R15: 
00007fff630 b069c
[  +0.000010]  </TASK>
[  +0.000002] irq event stamp: 50181
[  +0.000002] hardirqs last  enabled at (50187): [<ffffffffb61072a2>] 
__up _console_sem+0x52/0x60
[  +0.000003] hardirqs last disabled at (50192): [<ffffffffb6107287>] 
__up _console_sem+0x37/0x60
[  +0.000003] softirqs last  enabled at (45940): [<ffffffffb6bd4103>] 
sock _setsockopt+0x223/0xfa0
[  +0.000003] softirqs last disabled at (45938): [<ffffffffb6bd3609>] 
rele ase_sock+0x19/0xa0
[  +0.000004] ---[ end trace 0000000000000000 ]---
[  +0.000002] amdgpu: Could not allocate idr
[  +0.000245] amdgpu: Failed to restore CRIU ret:-12
[Nov 1 22:11] loop0: detected capacity change from 0 to 8

https://github.com/checkpoint-restore/criu/blob/criu-dev/plugins/amdgpu/amdgpu_plugin.c 
:


(00.093977)     11: Added GPU mapping [0xC093 -> 0xC093]
(00.093982)     11: ===Maps===============
(00.093987)     11: GPU: 0xC093 -> 0xC093
(00.093992)     11: CPU: 00 -> 00
(00.093997)     11: ======================
(00.094002)     11: Matched destination node 0xC093
(00.094007)     11: All nodes mapped successfully
(00.094012)     11: Matched nodes 0xC093 and after
(00.094017)     11: Maps after all nodes matched
(00.094022)     11: ===Maps===============
(00.094027)     11: GPU: 0xC093 -> 0xC093
(00.094032)     11: CPU: 00 -> 00
(00.094037)     11: ======================
(00.094041)     11: amdgpu_plugin: Restoring 1 devices
(00.094319)     11: amdgpu_plugin: amdgpu_plugin: passing drm render fd 
= 10 to driver
(00.094326)     11: amdgpu_plugin: Restore devices Ok (ret:0)
(00.094331)     11: amdgpu_plugin: Restoring 184 BOs
(00.094349)     11: amdgpu_plugin: Restore BOs Ok
(00.095791)     11: Error (amdgpu_plugin.c:1830): amdgpu_plugin: Restore 
ioctl failed: Cannot allocate memory
(00.095916)     11: Error (amdgpu_plugin.c:1850): amdgpu_plugin: 
amdgpu_plugin: Failed to restore (ret:-1)
(00.095951)     11: Error (criu/files-ext.c:53): Unable to restore 0x143
(00.095961)     11: Error (criu/files.c:1213): Unable to open fd=4 id=0x143
(00.096078) Unlink remap /dev/shm/fvKoKz.cr.1.ghost
(00.096152) Error (criu/cr-restore.c:2531): Restoring FAILED.
(00.096181) amdgpu_plugin: amdgpu_plugin: finished  amdgpu_plugin 
(AMDGPU/KFD)
"restore.log" 4194L, 201090C

>
> Fixes: be072b06c739 ("drm/amdkfd: CRIU export BOs as prime dmabuf objects")
> Reported-by: Jann Horn <jannh@google.com>
> CC: Rajneesh Bhardwaj <Rajneesh.Bhardwaj@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 27 +++++++-----------------
>   1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 5feaba6a77de..aabab9010812 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1994,38 +1994,27 @@ static int criu_checkpoint(struct file *filep,
>   	if (ret)
>   		goto exit_unlock;
>   
> -	ret = criu_checkpoint_bos(p, num_bos, (uint8_t __user *)args->bos,
> -			    (uint8_t __user *)args->priv_data, &priv_offset);
> -	if (ret)
> -		goto exit_unlock;
> -
>   	if (num_objects) {
>   		ret = kfd_criu_checkpoint_queues(p, (uint8_t __user *)args->priv_data,
>   						 &priv_offset);
>   		if (ret)
> -			goto close_bo_fds;
> +			goto exit_unlock;
>   
>   		ret = kfd_criu_checkpoint_events(p, (uint8_t __user *)args->priv_data,
>   						 &priv_offset);
>   		if (ret)
> -			goto close_bo_fds;
> +			goto exit_unlock;
>   
>   		ret = kfd_criu_checkpoint_svm(p, (uint8_t __user *)args->priv_data, &priv_offset);
>   		if (ret)
> -			goto close_bo_fds;
> +			goto exit_unlock;
>   	}
>   
> -close_bo_fds:
> -	if (ret) {
> -		/* If IOCTL returns err, user assumes all FDs opened in criu_dump_bos are closed */
> -		uint32_t i;
> -		struct kfd_criu_bo_bucket *bo_buckets = (struct kfd_criu_bo_bucket *) args->bos;
> -
> -		for (i = 0; i < num_bos; i++) {
> -			if (bo_buckets[i].alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM)
> -				close_fd(bo_buckets[i].dmabuf_fd);
> -		}
> -	}
> +	/* This must be the last thing in this function that can fail.
> +	 * Otherwise we leak dmabuf file descriptors.
> +	 */
> +	ret = criu_checkpoint_bos(p, num_bos, (uint8_t __user *)args->bos,
> +			   (uint8_t __user *)args->priv_data, &priv_offset);
>   
>   exit_unlock:
>   	mutex_unlock(&p->mutex);