Messages in this thread | | | Date | Fri, 18 Feb 2022 22:03:18 -0500 | Subject | Re: [PATCH] drm/amdkfd: rework criu_restore_bos error handling | From | Felix Kuehling <> |
| |
Am 2022-02-18 um 21:34 schrieb Tom Rix: > > On 2/18/22 10:35 AM, Felix Kuehling wrote: >> Am 2022-02-18 um 12:39 schrieb trix@redhat.com: >>> From: Tom Rix <trix@redhat.com> >>> >>> Clang static analysis reports this problem >>> kfd_chardev.c:2327:2: warning: 1st function call argument >>> is an uninitialized value >>> kvfree(bo_privs); >>> ^~~~~~~~~~~~~~~~ >>> >>> If the copy_from_users(bo_buckets, ...) fails, there is a jump to >>> the generic error handler at exit:. The freeing of bo_privs and >>> unwinding of the dmabuf_fd loop do not need to be done. >>> >>> Add some specific labels for the early failures. >>> Reorder the frees to be the reverse of their allocs. >>> >>> Move the initialize of 'i' back to the loop. >>> The problem with the early frees predates the loop >>> unwinding problem. >> >> I think the existing error handling strategy in this function is >> fine. Having only one exit label avoids potential issues when using >> the wrong label. Freeing NULL pointers is not a problem. The loop >> becomes a noop if i==0 (this was fixed by you in a previous patch). >> The only real problem I see is that bo_privs is not initialized. So >> this should really be a one-line or maybe two-line fix: >> >> struct kfd_criu_bo_bucket *bo_buckets = NULL; >> struct kfd_criu_bo_priv_data *bo_privs = NULL; > > This is the other way I considered to fix the problem. So it will work.
OK. I have already submitted this version to amd-staging-drm-next. Thank you for reporting the problem.
Regards, Felix
> > Tom > >> >> Regards, >> Felix >> >> >>> >>> Fixes: 73fa13b6a511 ("drm/amdkfd: CRIU Implement KFD restore ioctl") >>> Signed-off-by: Tom Rix <trix@redhat.com> >>> --- >>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++++------ >>> 1 file changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> index 965af2a08bc0..1d5f41ac3832 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>> @@ -2102,7 +2102,7 @@ static int criu_restore_bos(struct kfd_process >>> *p, >>> const bool criu_resume = true; >>> bool flush_tlbs = false; >>> int ret = 0, j = 0; >>> - uint32_t i = 0; >>> + uint32_t i; >>> if (*priv_offset + (args->num_bos * sizeof(*bo_privs)) > >>> max_priv_data_size) >>> return -EINVAL; >>> @@ -2119,13 +2119,13 @@ static int criu_restore_bos(struct >>> kfd_process *p, >>> if (ret) { >>> pr_err("Failed to copy BOs information from user\n"); >>> ret = -EFAULT; >>> - goto exit; >>> + goto free_buckets; >>> } >>> bo_privs = kvmalloc_array(args->num_bos, sizeof(*bo_privs), >>> GFP_KERNEL); >>> if (!bo_privs) { >>> ret = -ENOMEM; >>> - goto exit; >>> + goto free_buckets; >>> } >>> ret = copy_from_user(bo_privs, (void __user >>> *)args->priv_data + *priv_offset, >>> @@ -2133,12 +2133,12 @@ static int criu_restore_bos(struct >>> kfd_process *p, >>> if (ret) { >>> pr_err("Failed to copy BOs information from user\n"); >>> ret = -EFAULT; >>> - goto exit; >>> + goto free_privs; >>> } >>> *priv_offset += args->num_bos * sizeof(*bo_privs); >>> /* Create and map new BOs */ >>> - for (; i < args->num_bos; i++) { >>> + for (i = 0; i < args->num_bos; i++) { >>> struct kfd_criu_bo_bucket *bo_bucket; >>> struct kfd_criu_bo_priv_data *bo_priv; >>> struct kfd_dev *dev; >>> @@ -2323,8 +2323,11 @@ static int criu_restore_bos(struct >>> kfd_process *p, >>> if (bo_buckets[i].alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM) >>> close_fd(bo_buckets[i].dmabuf_fd); >>> } >>> - kvfree(bo_buckets); >>> +free_privs: >>> kvfree(bo_privs); >>> +free_buckets: >>> + kvfree(bo_buckets); >>> + >>> return ret; >>> } >> >
| |