[Intel-gfx] [bug report] drm/i915: Use vma resources for async unbinding

From: Dan Carpenter <dan.carpenter@oracle.com>
To: thomas.hellstrom@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [bug report] drm/i915: Use vma resources for async unbinding
Date: Mon, 28 Feb 2022 10:23:15 +0300	[thread overview]
Message-ID: <20220228072315.GA11695@kili> (raw)

Hello Thomas Hellström,

The patch 2f6b90da9192: "drm/i915: Use vma resources for async
unbinding" from Jan 10, 2022, leads to the following Smatch static
checker warning:

	drivers/gpu/drm/i915/i915_vma.c:515 i915_vma_bind()
	error: we previously assumed 'work->vma_res' could be null (see line 487)

drivers/gpu/drm/i915/i915_vma.c
    417 int i915_vma_bind(struct i915_vma *vma,
    418                   enum i915_cache_level cache_level,
    419                   u32 flags,
    420                   struct i915_vma_work *work,
    421                   struct i915_vma_resource *vma_res)
    422 {
    423         u32 bind_flags;
    424         u32 vma_flags;
    425         int ret;
    426 
    427         lockdep_assert_held(&vma->vm->mutex);
    428         GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
    429         GEM_BUG_ON(vma->size > vma->node.size);
    430 
    431         if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start,
    432                                               vma->node.size,
    433                                               vma->vm->total))) {
    434                 i915_vma_resource_free(vma_res);
    435                 return -ENODEV;
    436         }
    437 
    438         if (GEM_DEBUG_WARN_ON(!flags)) {
    439                 i915_vma_resource_free(vma_res);
    440                 return -EINVAL;
    441         }
    442 
    443         bind_flags = flags;
    444         bind_flags &= I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
    445 
    446         vma_flags = atomic_read(&vma->flags);
    447         vma_flags &= I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
    448 
    449         bind_flags &= ~vma_flags;
    450         if (bind_flags == 0) {
    451                 i915_vma_resource_free(vma_res);
    452                 return 0;
    453         }
    454 
    455         GEM_BUG_ON(!atomic_read(&vma->pages_count));
    456 
    457         /* Wait for or await async unbinds touching our range */
    458         if (work && bind_flags & vma->vm->bind_async_flags)
    459                 ret = i915_vma_resource_bind_dep_await(vma->vm,
    460                                                        &work->base.chain,
    461                                                        vma->node.start,
    462                                                        vma->node.size,
    463                                                        true,
    464                                                        GFP_NOWAIT |
    465                                                        __GFP_RETRY_MAYFAIL |
    466                                                        __GFP_NOWARN);
    467         else
    468                 ret = i915_vma_resource_bind_dep_sync(vma->vm, vma->node.start,
    469                                                       vma->node.size, true);
    470         if (ret) {
    471                 i915_vma_resource_free(vma_res);
    472                 return ret;
    473         }
    474 
    475         if (vma->resource || !vma_res) {
                                     ^^^^^^^^
Let's assume vma->resource is NULL and vma_res is also NULL.

    476                 /* Rebinding with an additional I915_VMA_*_BIND */
    477                 GEM_WARN_ON(!vma_flags);
    478                 i915_vma_resource_free(vma_res);
    479         } else {
    480                 i915_vma_resource_init_from_vma(vma_res, vma);
    481                 vma->resource = vma_res;
    482         }
    483         trace_i915_vma_bind(vma, bind_flags);
    484         if (work && bind_flags & vma->vm->bind_async_flags) {
    485                 struct dma_fence *prev;
    486 
    487                 work->vma_res = i915_vma_resource_get(vma->resource);

If "vma->resource" is NULL here then it leads to a crash in i915_vma_resource_get().

    488                 work->cache_level = cache_level;
    489                 work->flags = bind_flags;
    490 

regards,
dan carpenter