* [bug report] drm/msm: Add VM_BIND ioctl
@ 2025-08-02 7:49 Dan Carpenter
2025-08-02 13:12 ` Rob Clark
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2025-08-02 7:49 UTC (permalink / raw)
To: Rob Clark
Cc: Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
linux-arm-msm, dri-devel, freedreno
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.
597
598 /* Drop the previous ref: */
599 drm_gpuvm_bo_put(vm_bo);
600
601 return 0;
602 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] drm/msm: Add VM_BIND ioctl
2025-08-02 7:49 [bug report] drm/msm: Add VM_BIND ioctl Dan Carpenter
@ 2025-08-02 13:12 ` Rob Clark
2025-08-04 7:12 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Rob Clark @ 2025-08-02 13:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
linux-arm-msm, dri-devel, freedreno
On Sat, Aug 2, 2025 at 12:49 AM Dan Carpenter <dan.carpenter@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] drm/msm: Add VM_BIND ioctl
2025-08-02 13:12 ` Rob Clark
@ 2025-08-04 7:12 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-08-04 7:12 UTC (permalink / raw)
To: Rob Clark
Cc: Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
linux-arm-msm, dri-devel, freedreno
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@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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-04 7:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-02 7:49 [bug report] drm/msm: Add VM_BIND ioctl Dan Carpenter
2025-08-02 13:12 ` Rob Clark
2025-08-04 7:12 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox