Re: [PATCH v3 2/3] drm/msm: Destroy the VM_BIND entity when its queue is closed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/msm: out-of-order VM_BIND ops when killing entity in
  close_vm_bind()
--

commit 85e072e9d1eb16c143e9ba2cbe9ba1651898e394
Author: Jiwoo Kim <arpe1618@gmail.com>

drm/msm: Destroy the VM_BIND entity when its queue is closed

This commit destroys a VM_BIND submitqueue's scheduler entity when the queue
is closed via MSM_SUBMITQUEUE_CLOSE or file close rather than on the final
kref put, and rejects new VM_BIND jobs on closed queues.

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index 8d550d281704e..2dbe42551bd8f 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
[ ... ]
> @@ -107,6 +104,25 @@ struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_context *ctx,
>  	return NULL;
>  }
>  
> +/*
> + * The VM_BIND scheduler is torn down by msm_gem_vm_close() when the file is
> + * closed, but a VM_BIND queue can outlive that, as long as a job still holds
> + * a reference to it.  So destroy the VM_BIND entity here, when userspace
> + * closes the queue, rather than on the final put.  Both ways a queue is taken
> + * off ctx->submitqueues come through here.
> + */
> +static void msm_submitqueue_close_vm_bind(struct msm_gpu_submitqueue *queue)
> +{
> +	if (queue->entity != &queue->_vm_bind_entity[0])
> +		return;
> +
> +	mutex_lock(&queue->lock);
> +	queue->vm_bind_closed = true;
> +	mutex_unlock(&queue->lock);
> +
> +	drm_sched_entity_destroy(queue->entity);
> +}

[Severity: High]
Can destroying a single VM_BIND entity here cause page table updates on a
sibling VM_BIND queue of the same VM to run out of order with a killed job's
cleanup?

All VM_BIND queues in a context share ctx->vm and synchronously update its
drm_gpuvm state during msm_ioctl_vm_bind(), while deferring the actual IOMMU
page table updates to the scheduler.  When drm_sched_entity_destroy() is
interrupted by a fatal signal in msm_submitqueue_remove() (or times out or
is killed in msm_submitqueue_close()), drm_sched_entity_fini() kills the
entity's remaining jobs while sibling VM_BIND queues on the same VM can still
run jobs on vm->sched.

Because msm_gem_vm_unusable() is only called inside msm_vma_job_free() after
a killed job's in-fences signal and after vm_bind_job_apply_ops(job,
-ECANCELED) finishes, vm->unusable stays false in the meantime.  Even when a
job J2 on queue Q2 depends on the out-fence of job J1 on queue Q1,
drm_sched_entity_kill_jobs_work() signals J1's scheduled and finished fences
before calling free_job():

CPU0 (system_wq)                  CPU1 (vm->sched.submit_wq)
drm_sched_entity_kill_jobs_work()
  drm_sched_fence_scheduled(J1)
  drm_sched_fence_finished(J1)
                                  msm_vma_job_run(J2)
                                    /* vm->unusable is still false */
                                    vm_bind_job_apply_ops(J2, 0)
                                      vm_map_op(iova, BO_B)
  msm_vma_job_free(J1)
    vm_bind_job_apply_ops(J1, -ECANCELED)
      vm_unmap_op(iova, BO_0)     <-- unmaps BO_B's live mapping
    msm_gem_vm_unusable(job->vm)

Conversely, if J1 on Q2 maps BO_B at iova and J2 on Q1 unmaps it (removing
the VMA from drm_gpuvm in vm_bind_job_prepare()), killing Q1 lets
msm_vma_job_free(J2) run vm_unmap_op() before msm_vma_job_run(J1) acquires
vm->mmu_lock (having already read vm->unusable as false):

drivers/gpu/drm/msm/msm_gem_vma.c:msm_vma_job_run() {
    ...
	ret = vm_bind_job_apply_ops(job, vm->unusable ? -EINVAL : 0);
}

J1 then maps BO_B into the IOMMU page tables after J2's unmap has already
executed and after the VMA and GEM object references have been dropped,
leaving freed physical pages mapped in the GPU IOMMU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114141851.134290-1-arpe1618@gmail.com?part=2