[bug report] drm/msm: Add VM_BIND ioctl

[Rob Clark]

On Sat, Aug 2, 2025 at 12:49 AM Dan Carpenter <dan.carpenter at linaro.org> wrote:
>
> Hello Rob Clark,
>
> Commit 2e6a8a1fe2b2 ("drm/msm: Add VM_BIND ioctl") from Jun 29, 2025
> (linux-next), leads to the following Smatch static checker warning:
>
>         drivers/gpu/drm/msm/msm_gem_vma.c:596 msm_gem_vm_sm_step_remap()
>         error: we previously assumed 'vm_bo' could be null (see line 564)
>
> drivers/gpu/drm/msm/msm_gem_vma.c
>     521 static int
>     522 msm_gem_vm_sm_step_remap(struct drm_gpuva_op *op, void *arg)
>     523 {
>     524         struct msm_vm_bind_job *job = ((struct op_arg *)arg)->job;
>     525         struct drm_gpuvm *vm = job->vm;
>     526         struct drm_gpuva *orig_vma = op->remap.unmap->va;
>     527         struct drm_gpuva *prev_vma = NULL, *next_vma = NULL;
>     528         struct drm_gpuvm_bo *vm_bo = orig_vma->vm_bo;
>     529         bool mapped = to_msm_vma(orig_vma)->mapped;
>     530         unsigned flags;
>     531
>     532         vm_dbg("orig_vma: %p:%p:%p: %016llx %016llx", vm, orig_vma,
>     533                orig_vma->gem.obj, orig_vma->va.addr, orig_vma->va.range);
>     534
>     535         if (mapped) {
>     536                 uint64_t unmap_start, unmap_range;
>     537
>     538                 drm_gpuva_op_remap_to_unmap_range(&op->remap, &unmap_start, &unmap_range);
>     539
>     540                 vm_op_enqueue(arg, (struct msm_vm_op){
>     541                         .op = MSM_VM_OP_UNMAP,
>     542                         .unmap = {
>     543                                 .iova = unmap_start,
>     544                                 .range = unmap_range,
>     545                                 .queue_id = job->queue->id,
>     546                         },
>     547                         .obj = orig_vma->gem.obj,
>     548                 });
>     549
>     550                 /*
>     551                  * Part of this GEM obj is still mapped, but we're going to kill the
>     552                  * existing VMA and replace it with one or two new ones (ie. two if
>     553                  * the unmapped range is in the middle of the existing (unmap) VMA).
>     554                  * So just set the state to unmapped:
>     555                  */
>     556                 to_msm_vma(orig_vma)->mapped = false;
>     557         }
>     558
>     559         /*
>     560          * Hold a ref to the vm_bo between the msm_gem_vma_close() and the
>     561          * creation of the new prev/next vma's, in case the vm_bo is tracked
>     562          * in the VM's evict list:
>     563          */
>     564         if (vm_bo)
>                 ^^^^^^^^^^
> NULL check
>
>     565                 drm_gpuvm_bo_get(vm_bo);
>     566
>     567         /*
>     568          * The prev_vma and/or next_vma are replacing the unmapped vma, and
>     569          * therefore should preserve it's flags:
>     570          */
>     571         flags = orig_vma->flags;
>     572
>     573         msm_gem_vma_close(orig_vma);
>     574
>     575         if (op->remap.prev) {
>     576                 prev_vma = vma_from_op(arg, op->remap.prev);
>     577                 if (WARN_ON(IS_ERR(prev_vma)))
>     578                         return PTR_ERR(prev_vma);
>     579
>     580                 vm_dbg("prev_vma: %p:%p: %016llx %016llx", vm, prev_vma, prev_vma->va.addr, prev_vma->va.range);
>     581                 to_msm_vma(prev_vma)->mapped = mapped;
>     582                 prev_vma->flags = flags;
>     583         }
>     584
>     585         if (op->remap.next) {
>     586                 next_vma = vma_from_op(arg, op->remap.next);
>     587                 if (WARN_ON(IS_ERR(next_vma)))
>     588                         return PTR_ERR(next_vma);
>     589
>     590                 vm_dbg("next_vma: %p:%p: %016llx %016llx", vm, next_vma, next_vma->va.addr, next_vma->va.range);
>     591                 to_msm_vma(next_vma)->mapped = mapped;
>     592                 next_vma->flags = flags;
>     593         }
>     594
>     595         if (!mapped)
> --> 596                 drm_gpuvm_bo_evict(vm_bo, true);
>                                            ^^^^^
> Unchecked dereference.  Possibly if we're not mapped then it's non-NULL?
> If so then just ignore this warning.

Correct, the !mapped case will only happen when the shrinker evicts
BOs.  The case where the BO (and therefore vm_bo) is NULL, is MAP_NULL
mappings which are backed by the PRR page, not a BO that can be
evicted.

Would adding a WARN_ON(!vm_bo) convey to smatch that this case should
not happen unless something somewhere else was rather screwed up?

BR,
-R

>     597
>     598         /* Drop the previous ref: */
>     599         drm_gpuvm_bo_put(vm_bo);
>     600
>     601         return 0;
>     602 }
>
> regards,
> dan carpenter