Re: [PATCH v3 1/3] drm/msm: Release the pins and ops of a killed VM_BIND job

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/msm: race between msm_vma_job_free() and
  msm_vma_job_run() on shared VM
--

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

drm/msm: Release the pins and ops of a killed VM_BIND job

This commit applies queued unmap operations, drops GEM object references,
and unpins BOs when the DRM scheduler frees a killed VM_BIND job without
running it.

> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 8b702422030af..0b9f2e8916b7a 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c

[ ... ]

> @@ -740,6 +740,22 @@ msm_vma_job_run(struct drm_sched_job *_job)
>  	vm->mmu->prealloc = NULL;
>  	mutex_unlock(&vm->mmu_lock);
>  
> +	return ret;
> +}
> +
> +static struct dma_fence *
> +msm_vma_job_run(struct drm_sched_job *_job)
> +{
> +	struct msm_vm_bind_job *job = to_msm_vm_bind_job(_job);
> +	struct drm_device *dev = job->vm->drm;
> +	struct msm_gem_vm *vm = to_msm_vm(job->vm);
> +	struct drm_gem_object *obj;
> +	int ret;
> +
> +	vm_dbg("");
> +
> +	ret = vm_bind_job_apply_ops(job, vm->unusable ? -EINVAL : 0);

[ ... ]

> @@ -791,6 +826,29 @@ msm_vma_job_free(struct drm_sched_job *_job)
>  	kfree(job);
>  }
>  
> +static void
> +msm_vma_job_free(struct drm_sched_job *_job)
> +{
> +	struct msm_vm_bind_job *job = to_msm_vm_bind_job(_job);
> +
> +	/*
> +	 * A job that the scheduler frees without having run it was killed,
> +	 * e.g. because its entity was destroyed.  Its queued updates were
> +	 * already applied to the VM's address space, but not to its pgtables.
> +	 * The scheduler has waited for the job's dependencies, and for the job
> +	 * before it on the entity to run or be killed, so apply its unmaps now,
> +	 * before the references to the unmapped objects are dropped, and give
> +	 * up on the VM, as its maps are skipped.  Killed jobs only unmap, so
> +	 * the order in which they are freed does not matter.
> +	 */
> +	if (!list_empty(&job->vm_ops)) {
> +		vm_bind_job_apply_ops(job, -ECANCELED);
> +		msm_gem_vm_unusable(job->vm);
> +	}

[Severity: Medium]
Can a dependent VM_BIND job on another entity of the same VM run before
msm_vma_job_free() applies a killed job's unmaps and marks the VM
unusable?

Multiple MSM_SUBMITQUEUE_VM_BIND queues share the same msm_gem_vm and
vm->sched.  When queue 1 is destroyed while job1 (which unmaps an IOVA
range) is waiting on an in-fence, drm_sched_entity_kill_jobs_work()
signals job1's finished fence before calling free_job().  If job2 on
queue 2 depends on job1's out-fence and maps the same IOVA range,
signaling job1's fence wakes queue 2 on vm->sched.submit_wq while
msm_vma_job_free(job1) runs on system_wq:

system_wq (queue 1 killed)        vm->sched.submit_wq (queue 2)
drm_sched_entity_kill_jobs_work()
  drm_sched_fence_finished(job1)
    ...
      drm_sched_entity_wakeup()
                                  drm_sched_run_job_work()
                                    msm_vma_job_run(job2)
                                      vm->unusable is false
                                      vm_bind_job_apply_ops(job2, 0)
                                        vm_map_op()
  msm_vma_job_free(job1)
    vm_bind_job_apply_ops(job1, -ECANCELED)
      vm_unmap_op()               <-- unmaps job2's new PTEs
    msm_gem_vm_unusable(vm)

job2's vm_map_op() either collides with PTEs that job1 has not unmapped
yet, or succeeds before job1's delayed vm_unmap_op() tears down the PTEs
that job2 just installed.

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