Messages in this thread | | | Subject | Re: [PATCH] drm/amdkfd: rework criu_restore_bos error handling | From | Tom Rix <> | Date | Fri, 18 Feb 2022 18:34:41 -0800 |
| |
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.
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; >> } >
| |