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.carpen...@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
