Messages in this thread | | | Date | Fri, 18 Feb 2022 13:35:41 -0500 | Subject | Re: [PATCH] drm/amdkfd: rework criu_restore_bos error handling | From | Felix Kuehling <> |
| |
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;
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; > } >
| |