[bug report] drm/msm: Add VM_BIND ioctl

[Dan Carpenter]

On Sat, Aug 02, 2025 at 06:12:56AM -0700, Rob Clark wrote:
> 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?

No.  WARN_ON() doesn't mean something can't happen.  Just ignore it.
Old warnings are always false positives and if people have questions
they can find this thread on lore.

regards,
dan carpenter