Re: [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init()

On Wed, Sep 23, 2015 at 10:16 AM, Christian König christian.koenig@amd.com wrote:

On 23.09.2015 12:59, Dan Carpenter wrote:

...

The amdgpu_cs_parser_init() function doesn't clean up after itself but instead the caller uses a free everything function amdgpu_cs_parser_fini() on failure. This style of error handling is often buggy. In this example, we call "drm_free_large(parser->chunks[i].kdata);" when it is an unintialized pointer or when "parser->chunks" is NULL.

I fixed this bug by adding unwind code so that it frees everything that it allocates.

I also mode some other very minor changes:

1. Renamed "r" to "ret".
2. Moved the chunk_array allocation to the start of the function.
3. Removed some initializers which are no longer needed.

Reported-by: Ilja Van Sprundel ivansprundel@ioactive.com Signed-off-by: Dan Carpenter dan.carpenter@oracle.com

The whole set looks sane to me, patches are Reviewed-by: Christian König christian.koenig@amd.com

Applied. thanks!

Alex

Regards, Christian.

...

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 3b355ae..abb257d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -154,42 +154,41 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data) { union drm_amdgpu_cs *cs = data; uint64_t *chunk_array_user;

•   uint64_t *chunk_array = NULL;
  

•   uint64_t *chunk_array;
    struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
    unsigned size, i;
  

•   int r = 0;
  

•   int ret;
  

  • if (!cs->in.num_chunks)
    

•           goto out;
  

•   if (cs->in.num_chunks == 0)
  

•           return 0;
  

•   chunk_array = kmalloc_array(cs->in.num_chunks, sizeof(uint64_t),
  

GFP_KERNEL);

•   if (!chunk_array)
  

•           return -ENOMEM;
    p->ctx = amdgpu_ctx_get(fpriv, cs->in.ctx_id);
    if (!p->ctx) {
  

•           r = -EINVAL;
  

•           goto out;
  

•           ret = -EINVAL;
  

•           goto free_chunk;
    }
  

•   p->bo_list = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle);
    /* get chunks */
    INIT_LIST_HEAD(&p->validated);
  

•   chunk_array = kmalloc_array(cs->in.num_chunks, sizeof(uint64_t),
  

GFP_KERNEL);

•   if (chunk_array == NULL) {
  

•           r = -ENOMEM;
  

•           goto out;
  

•   }
  

•   chunk_array_user = (uint64_t __user *)(cs->in.chunks);
    if (copy_from_user(chunk_array, chunk_array_user,
                       sizeof(uint64_t)*cs->in.num_chunks)) {
  

•           r = -EFAULT;
  

•           goto out;
  

•           ret = -EFAULT;
  

•           goto put_bo_list;
    }
    p->nchunks = cs->in.num_chunks;
    p->chunks = kmalloc_array(p->nchunks, sizeof(struct
  

amdgpu_cs_chunk), GFP_KERNEL);

•   if (p->chunks == NULL) {
  

•           r = -ENOMEM;
  

•           goto out;
  

•   if (!p->chunks) {
  

•           ret = -ENOMEM;
  

•           goto put_bo_list;
    }
    for (i = 0; i < p->nchunks; i++) {
  

@@ -200,8 +199,9 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data) chunk_ptr = (void __user *)chunk_array[i]; if (copy_from_user(&user_chunk, chunk_ptr, sizeof(struct drm_amdgpu_cs_chunk))) {

•                   r = -EFAULT;
  

•                   goto out;
  

•                   ret = -EFAULT;
  

•                   i--;
  

•                   goto free_partial_kdata;
            }
            p->chunks[i].chunk_id = user_chunk.chunk_id;
            p->chunks[i].length_dw = user_chunk.length_dw;
  

@@ -212,13 +212,14 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data) p->chunks[i].kdata = drm_malloc_ab(size, sizeof(uint32_t)); if (p->chunks[i].kdata == NULL) {

•                   r = -ENOMEM;
  

•                   goto out;
  

•                   ret = -ENOMEM;
  

•                   i--;
  

•                   goto free_partial_kdata;
            }
            size *= sizeof(uint32_t);
            if (copy_from_user(p->chunks[i].kdata, cdata, size)) {
  

•                   r = -EFAULT;
  

•                   goto out;
  

•                   ret = -EFAULT;
  

•                   goto free_partial_kdata;
            }
            switch (p->chunks[i].chunk_id) {
  

@@ -238,15 +239,15 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data) gobj = drm_gem_object_lookup(p->adev->ddev, p->filp, handle); if (gobj == NULL) {

•                                   r = -EINVAL;
  

•                                   goto out;
  

•                                   ret = -EINVAL;
  

•                                   goto free_partial_kdata;
                            }
                            p->uf.bo = gem_to_amdgpu_bo(gobj);
                            p->uf.offset = fence_data->offset;
                    } else {
  

•                           r = -EINVAL;
  

•                           goto out;
  

•                           ret = -EINVAL;
  

•                           goto free_partial_kdata;
                    }
                    break;
  

  @@ -254,19 +255,35 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser

*p, void *data) break; default:

•                   r = -EINVAL;
  

•                   goto out;
  

•                   ret = -EINVAL;
  

•                   goto free_partial_kdata;
            }
    }
    p->ibs = kcalloc(p->num_ibs, sizeof(struct amdgpu_ib),
  

GFP_KERNEL);

•   if (!p->ibs)
  

•           r = -ENOMEM;
  

•   if (!p->ibs) {
  

•           ret = -ENOMEM;
  

•           goto free_all_kdata;
  

•   }
  

  -out: kfree(chunk_array);

•   return r;
  

•   return 0;
  

+free_all_kdata:

•   i = p->nchunks - 1;
  

+free_partial_kdata:

•   for (; i >= 0; i--)
  

•           drm_free_large(p->chunks[i].kdata);
  

•   kfree(p->chunks);
  

+put_bo_list:

•   if (p->bo_list)
  

•           amdgpu_bo_list_put(p->bo_list);
  

•   amdgpu_ctx_put(p->ctx);
  

+free_chunk:

•   kfree(chunk_array);
  

•   return ret;
  

  } /* Returns how many bytes TTM can move per IB.

@@ -804,7 +821,7 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) r = amdgpu_cs_parser_init(parser, data); if (r) { DRM_ERROR("Failed to initialize parser !\n");

•           amdgpu_cs_parser_fini(parser, r, false);
  

•           kfree(parser);
            up_read(&adev->exclusive_lock);
            r = amdgpu_cs_handle_lockup(adev, r);
            return r;
  

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel