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?
BR,
-R
> 597
> 598 /* Drop the previous ref: */
> 599 drm_gpuvm_bo_put(vm_bo);
> 600
> 601 return 0;
> 602 }
>
> regards,
> dan carpenter
